Subject: [babel] Mandatory sub-TLVs in Next Hop and Router-ID

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 14:22:37 +0200

I don't think it's a good idea to put a Mandatory sub-TLV in Next Hop or
Router-ID, since these two TLVs are used for establishing state, and the
state must be correlated for different implementations.  However, we need
to specify the behaviour if for some reason we receive one of these with
an unknown mandatory sub-TLV.

The current text and implementation does the simple thing, and ignores
such TLVs alltogether -- so only Update carries the exception to mandatory
handling (which is unavoidable in that case if we wish to use compression
effectively).  Please shout if you see any reason for the other possible
behaviour.

-- Juliusz

---

From: Matthieu Boutier <[email-masked]>
Date: Wed, 24 May 2017 14:47:21 +0200

> handling

Next Hop and Router-ID only update state, instead of Updates which also
carry route information.  So it is possible to just update state with
two successive Next Hop or Router-ID TLVs (the first without mandatory
sub-TLV).   If an extension want to put mandatory sub-TLV to Next Hop
and Router-ID while preserving state, it can do it.  So I would say:
keep doing the simple thing.

Matthieu

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 15:02:40 +0200

> sub-TLV).

You've got a sick mind ;-)

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 24 May 2017 15:42:50 +0200

Juliusz Chroboczek <[email-masked]> writes:

> such TLVs alltogether -- so only Update carries the exception to
> mandatory handling (which is unavoidable in that case if we wish to
> use compression effectively). Please shout if you see any reason for
> the other possible behaviour.

Why is special-casing updates the simple thing? If an update is the only
thing that requires updating parser state no matter the sub-TLV, it has
to be special-cased and TLV parsing has to be interleaved with sub-TLV
parsing. Whereas if there is consistency, it can all be the same
code-path.

I.e., if the spec says to always update parser state, I can do something
like:

bool read_tlv(tlv_data[]) {
  return tlv_header_valid(tlv_data) \
     && tlv_parser[tlv_data.type].parse(tlv_data) \ // updates parser state
     && read_subtlvs(tlv_data); // return false if an unknown mandatory sub-TLV is found
}

versus:

bool read_tlv(tlv_data[]) {
  if(!tlv_header_valid(tlv_data])
    return false;

  if(tlv_data.type == TYPE_UPDATE) {
    return tlv_parser[tlv_data.type].parse(tlv_data) && read_subtlvs(tlv_data);
  } else {
    return read_subtlvs(tlv_data) && tlv_parser[tlv_data.type].parse(tlv_data);
  }
}

(in this pseudo-code example all these functions are assumed to have
side effects that does something useful with the parsed TLV information;
haven't actually gotten around to implementing sub-TLVs for real yet)

-Toke

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 15:56:27 +0200

> parsing.

I agree, it depends on the implementation.  In my implementation (that
doesn't build a parse tree, but interleaves parsing and updating data
structures), special-casing updates is the simple thing.  I suppose that
if you're building a parse tree, then the two are roughly similar.

As I've said, I don't have any strong preferences either case, so if
anyone has any usage scenarios for mandatory sub-TLVs on Next Hop or
Router-Id TLVs, I'm listening.  The one thing I think would be a mistake
would be to leave this case underspecified, so we need to make a call.

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 16:05:30 +0200

> I suppose that if you're building a parse tree, then the two are roughly
> similar.

Hmm, I'm no longer so sure about that; it might actually be significantly
simpler.  Is that your case, Toke?

David, Markus, please chime in.

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 24 May 2017 16:06:50 +0200

Juliusz Chroboczek <[email-masked]> writes:

> Router-Id TLVs, I'm listening.

Specifying source addresses for the next hop? Extending the router-ID
size? Not sure these are actually something you'd want to do, just the
only things that came to mind...

> The one thing I think would be a mistake would be to leave this case
> underspecified, so we need to make a call.

This I can agree with :)

-Toke

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 24 May 2017 16:19:50 +0200

Juliusz Chroboczek <[email-masked]> writes:

> Hmm, I'm no longer so sure about that; it might actually be
> significantly simpler. Is that your case, Toke?

I'm not building a parse tree, I'm just doing two-pass parsing (first
pass reads wire data into an internal data structure while updating
parser state, second pass does protocol handling). The pseudo-code
example I posted before is not actually that far off from the actual C
function:

static inline int
babel_read_tlv(struct babel_tlv *hdr,
               union babel_msg *msg,
               struct babel_parse_state *state)
{
  if ((hdr->type <= BABEL_TLV_PADN) ||
      (hdr->type >= BABEL_TLV_MAX) ||
      !tlv_data[hdr->type].read_tlv)
    return PARSE_IGNORE;

  if (TLV_LENGTH(hdr) < tlv_data[hdr->type].min_length)
    return PARSE_ERROR;

  memset(msg, 0, sizeof(*msg));
  return tlv_data[hdr->type].read_tlv(hdr, msg, state);
}

What I was planning to do was just add a babel_read_subtlv() function
that would be called after the last line, which could just return
PARSE_IGNORE on unknown mandatory sub-TLVs (which would cause the
first-pass parser to drop the whole TLV).

I know this is very specific to the way I implemented things, but from
a high-level PoV, I'd agree with Markus that being consistent is better
than special-casing.

-Toke

---

From: Markus Stenberg <[email-masked]>
Date: Wed, 24 May 2017 17:07:28 +0300

> David, Markus, please chime in.

On ideological level, I would prefer just to say that _regardless_ of TLV type mandatory bit in sub-TLVs works in a specific way. One might additionally note that in case of update TLVs (at least) it is a bad idea, but it would keep the design simple and easy to understand (and implement).

Cheers,

-Markus

---

From: Markus Stenberg <[email-masked]>
Date: Wed, 24 May 2017 17:09:21 +0300

On 24 May 2017, at 16.02, Juliusz Chroboczek <[email-masked]> wrote:
> You’ve got a sick mind ;-)

Haha. High five, Mathieu; I was thinking of the same example but figured I did not want to bring it up this morning.

In general I do not like constraining functionality arbitrarily, as long as not constraining has well understood outcome (and I believe that is the case here).

Cheers,

-Markus

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 16:37:23 +0200

I'm slowly starting to agree with you guys.

Markus said:

> On ideological level, I would prefer just to say that _regardless_ of
> TLV type mandatory bit in sub-TLVs works in a specific way.

So that would mean saying that all the parser state (default prefix,
router-id, next hop) are updated even for ignored sub-TLVs, whatever the
TLV?  You appear to be agreeing with Toke.

Toke said:

> Specifying source addresses for the next hop?

I'm not sure what the proper semantics would be, here: do you want the
next hop to be taken into account if the receiver doesn't understand the
extension?

> Extending the router-ID size?

I'm not sure how that would work.  You'd probably want the whole set of
updates to be dropped if the receiver cannot grok the larger router-id, so
you'd really want to put a mandatory sub-TLV on all updates.

A similar idea would be putting the source prefix in the Router-ID.  So
instead of the Matthieu's current encoding:

  router-id
  update (mandatory source-prefix=A)
  update (mandatory source-prefix=A)

you'd have the slightly more economic

  router-id (mandatory source-prefix=A)
  update
  update

Again, you'd want the whole block of updates to be ignored if the
router-id is not understood, so that idea's not gonna fly without some
very complex semantics.

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Wed, 24 May 2017 16:49:23 +0200

> I'm slowly starting to agree with you guys.

Wooh ;)

> next hop to be taken into account if the receiver doesn't understand
> the extension?

I was thinking along the lines of being able to express "this next-hop
is only available from this source prefix (due to, e.g., firewall
rules)". Something like:

nexthop (sub-tlv: only from source-prefix A)
nexthop (sub-tlv: only from source-prefix B)

where the implementation would pick the right one, and an implementation
that didn't understand the sub-tlv would ignore both.

Not sure how useful this is in practice, but why rule it out? :)

-Toke

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 17:06:36 +0200

> that didn't understand the sub-tlv would ignore both.

Hmm... so that would require the current semantics, the one you've been
arguing against?

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 19:01:10 +0200

> I'm slowly starting to agree with you guys.

After further thought, I agree.

  https://github.com/jech/babel-drafts/commit/f216118f5751a8c95e59c8333b989f21da9da246

Both my implementations updated to comply, it's a trivial change (just
swap updating the parser state with checking for unknown mandatory sub-TLVs).

-- Juliusz

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 24 May 2017 21:32:27 +0200

> After further thought, I agree.

Another advantage is that tools like tcpdump can parse Babel without
taking mandatory bits into account.  Definitely the right thing.

-- Juliusz

---

From: Toke Høiland-Jørgensen <[email-masked]>
Date: Thu, 25 May 2017 13:44:32 +0200

Juliusz Chroboczek <[email-masked]> writes:

> Hmm... so that would require the current semantics, the one you've
> been arguing against?

Yeah, realise that. As I said, not sure if it is actually useful; more
of a thought experiment to come up with a reason. I do still think being
consistent is better :)

-Toke

---

From: Juliusz Chroboczek <[email-masked]>
Date: Thu, 25 May 2017 14:03:35 +0200

> I do still think being consistent is better :)

Consistency is the straightjacket of small minds.

The main insight your comments gave me is that there are two layers: the
parser, which happens to be stateful and implements compression, and the
rest of Babel, which is layered above the parser.  Clearly, you want the
parser to build the same parse tree whichever options are implemented, so
the parser must ignore mandatory bits.  (In particular, tcpdump and
wireshark only do parsing, and you want them to be implementable
deterministically without reference to a specific set of extensions.)

To put it differently: suppose Babel were reformulated to use the PacketBB
packet format, or XML, or JSON, or protobuf, or whatever other horror the
"let's specify first and implement someday" community has come up with
today.  Then the whole stateful parsing layer would go away, but you'd
want the handling of mandatory bits to remain unchanged.

The reason I was confused is that I've been interleaving parsing and
acting on the result in my implementation, so the distinction between the
two layers wasn't obvious to me.  I hope I've managed to make it clear in
the current version of the draft, but there's obviously room for improvement.

-- Juliusz

---

From: David Schinazi <[email-masked]>
Date: Wed, 31 May 2017 08:15:15 -0700

I'm a little late to the party here, but I think I agree with where the discussion landed,
as in treat NextHop and RouterID like any other TLV, fully ignore it in the
presence of an unknown mandatory sub TLV.

> On May 25, 2017, at 05:03, Juliusz Chroboczek <[email-masked]> wrote:
> 

I like my specifications in tight straightjackets :)

David

---

From: Juliusz Chroboczek <[email-masked]>
Date: Wed, 31 May 2017 17:32:34 +0200

> I'm a little late to the party here, but I think I agree with where the
> discussion landed, as in treat NextHop and RouterID like any other TLV,
> fully ignore it in the presence of an unknown mandatory sub TLV.

Er... no.  The conclusion is that Next Hop and Router-ID TLVs are honoured
even if ignored due to an unknown mandatory sub-TLV.

The rationale is that the current next hop and router-id are part of the
parser's state, and that the packet is parsed with no reference to
sub-TLVs.  Only after the packet is parsed are some TLVs dropped.  This
way, the produced parse tree is identical no matter which sub-TLVs are
honoured by a given implementation, which makes it possible to build tools
such as tcpdump that need to parse a packet but don't honour any sub-TLVs.

Folks, please read the relevant sections in -02, and let me know if the
current text is not clear in any way.

-- Juliusz