Re: [pim] draft-ietf-pim-igmp-mld-extension WGLC

["Holland, Jake" <[e-mail]>]

Hi Stig,

Thanks for the update, on the whole I think this improved the text
with respect to my previous comments and made several things better,
thanks.

However, while looking through the diffs, to me there seem some new
points of confusion raised by the new edits, so here's a few new
comments:

1.
This new paragraph in the intro seems like it might be over-reaching:

   When this extension mechanism is used, it will make use of the entire
   Additional Data section defined in IGMPv3/MLDv2 for TLVs.  The TLV
   scheme is flexible enough to provide for any future extensions.

I think this spec doesn't say anything about possible alternative uses
of other reserved bits indicating alternate semantics for the Auxiliary
Data sections (and probably shouldn't make claims about being flexible
enough for all future extensions).

However, it does specify that when the E bit is set, no other data
besides the Extension TLVs described here can appear in the Auxiliary
Data part of the packet.  This is narrower than what's written, I think.

1.a.
Sorry, I should have noticed this in my first review.  But while revisiting
this, I noticed that the MLDv2 and IGMPv3 specs call the extra data field
"Auxiliary Data", but this spec calls it "Additional Data".

This seems like a point of possible confusion, is there a reason not to call
it "Auxiliary Data" in this doc as well?


2.
In section 3 the 2nd to last paragraph says there MUST be no data
in the message after the last TLV:

   There MUST be no data in the message after the last TLV.  The TLVs
   are processed until the end of the message is reached.  When
   processing the TLVs an implementation MUST keep track of how many
   octets are remaining in the message and stop TLV processing when
   there is no room for any further TLVs.  That is, TLV processing stops
   if there are less than 4 octets remaining in the message after a TLV
   is processed since there is not enough room for an additional minimal
   TLV.  ...


This wording I think is confusing on a few points:
- "in the message" is sort of unclear (e.g. it could refer to the IP
  payload length, at least for the last group record).  Although the doc
  does refer briefly to the relevant sections of the IGMP and MLD specs
  in the 2nd to last paragraph of the intro, it's not clear here, near
  the normative requirements about parsing, what "in the message" means.
  I think these requirements mean to say that the TLVs must fit entirely
  within the Auxiliary Data field, with a minimum of leftover space.
- the requirement "no data following the last TLV in the Auxiliary Data
  field" might not be satisfiable, because the TLV length is given in
  octets but the Auxiliary Data length is given in 32-bit words, so there
  would have to be some leftover data when the sum of TLV lengths is not
  a multiple of 4.  So I think this requirement needs to be articulated
  better.
- Nothing is stated about leftover data if any (since there might be up
  to 3 octets unless there should be an alignment requirement?).  Maybe
  it's worth stating it should be set to 0 and ignored, if there are
  any extra octets at the end?

3.
The second of these paragraphs in the "Extension Length" explanation in
section 3 seems to be trying to clarify the parsing, but to me it raises
more questions than it answers:
      Extension Length: 2 octets.  This specifies the length in octets
      of the following Extension Value field.  Note that this value may
      be zero, in which case there is no Extension Value field present.

      The next type field, if any, will come immediately after this
      length field.

I think this is trying to say that in the event of a 0-length Extension
Value, the Extension Type of the next TLV (if any) follows the Extension
Length immediately.

But it made me realize nothing seems to state explicitly that the next
extension type field (if any) immediately follows the extension value
field (as opposed to padding for alignment or something, as Figure 1,
right above this, seems to imply it might).

It's a minor point, but it might make sense to tweak this to explain the
whole picture with something like:
      Extension Length: 2 octets.  This specifies the length in octets
      of the following Extension Value field.

      The next Extension Type field, if any, will come immediately after
      the following Extension Value field (or immediately after the
      Extension Length field, for cases with a 0-length Extension Value).

(And perhaps the 2nd paragraph or something like it should move into the
explanation of the Extension Value, or into a paragraph explaining about
the parsing overview, rather than being in the Extension Length explanation).

4.
I find this design a bit troubling:
   Also if a TLV has a length exceeding the remainder of the
   message, that TLV is ignored, and further TLV processing stops.

As an implementor, if I see this scenario I would tend to assume that
I'm talking to an implementation that is using a different non-standard
meaning for the E bit and the Auxiliary Data (or perhaps an attack
packet), and I would think the better choice is to discard the entire
Auxiliary Data and treat it as though the E bit was not set, ignoring
all the TLVs.  (I assume this should be OK since the Auxiliary Data can
be ignored anyway by the devices that don't know about it.)


Anyway, I hope that's helpful, and thanks for the updates addressing my
prior comments.

Best regards,
Jake


On 1/15/21, 3:35 PM, "Stig Venaas" <[e-mail]> wrote:

Thanks a lot for all the WGLC responses, in particular to Ian, Lenny,
Jeffrey and Jake for great comments. Hope I didn't miss anyone.

I just posted revision 03 that I think should address your comments.
Great if you can have a look. I realize I forgot to update the
acknowledgements though, so I will do that shortly.

Regards,
Stig

On Thu, Dec 17, 2020 at 3:25 PM Stig Venaas <[e-mail]> wrote:
>
> Hi Jeffrey
>
> I appreciate your comments, please see responses inline.
>
> On Tue, Dec 15, 2020 at 6:21 PM Jeffrey (Zhaohui) Zhang
> <zzhang=[e-mail]> wrote:
> >
> > Support progressing this work.
> >
> >
> >
> > I have a couple of curiosity questions on the following:
> >
> >
> >
> >    Note that there may be additional data following this extension.  The
> >
> >    Total Extension Length field would indicate where this extension
> >
> >    ends, and the additional data starts.
> >
> >
> >
> > How would you have additional data following this extension? The TLV mechanism seems to be universal/generic to handle all possible extensions, so why would you have another extension blob after this, and how would you indicate so? By another bit in the reserved field?
> >
> > Just wondering if this additional data needs to be mentioned at all.
>
> I agree that there should be no need for additional data for future
> extensions as TLVs can provide any potential extension. If the WG and
> co-authors agree, I'm happy to remove the additional additional data.
> We can then also simplify the length check and say that the extension
> length specified must be equal to the remaining payload (subtracting
> from packet payload length).
>
> >
> >   Also, there is a possibility
> >
> >    that an implementation uses the Additional Data part of IGMP/MLD
> >
> >    messages, but not according to this extension scheme.
> >
> >
> >
> > Do you mean a non-standard existing implementation? Now that you mention that, are we supposed to define the interop procedures? Are they supposed to interop with the implementations following this draft at all?
>
> The IGMP/MLD RFCs specify how additional data may be present and how
> it should be ignored. I've seen cases where implementations
> intentionally (or maybe by accident) have sent packets with additional
> data. I think it is safest to use a reserved bit to clearly indicate
> that this extension is in place, rather than trying to parse whatever
> additional data may be present.
>
> If we receive data from an existing implementation, the reserved bit
> would not be set, and we would process it exactly as before.
> If an existing implementation receives the new IGMP/MLD message, it
> would both ignore the reserved bit, and the additional data according
> to the RFCs (although if implementations tried to use additional data
> in some way, then they might break, but I think we have to live with
> that). They were not supposed to use it.
>
> When using the extension, one must consider interoperability with
> implementations that do not support the extension mechanism, as well
> as implementations that do not support the specified types. Not
> supporting the mechanism, should be equivalent to not supporting any
> of the types.
>
> I can try to add a few more details in the draft.
> >
> >
> > Also, for the following:
> >
> >
> >
> >    The extension will be part of additional data as mentioned in
> >
> >    [RFC3810] Section 5.1.12 (resp.  [RFC3376] Section 4.1.10) for query
> >
> >    messages and [RFC3810] Section 5.2.12 (resp.  [RFC3376]
> >
> >    Section 4.2.11) for report messages.
> >
> >
> >
> > 5.2.12 should be 5.2.11.
>
> Good catch!
>
> Let me know if you have any thoughts on what I wrote above, or anything else.
>
> Thanks,
> Stig
>
> >
> >
> > Jeffrey
> >
> >
> >
> > From: pim <[e-mail]> On Behalf Of Michael McBride
> > Sent: Monday, December 7, 2020 7:24 PM
> > To: [e-mail]
> > Subject: [pim] draft-ietf-pim-igmp-mld-extension WGLC
> >
> >
> >
> > [External Email. Be cautious of content]
> >
> >
> >
> > Hello all,
> >
> >
> >
> > Today begins a two week wglc for https://urldefense.com/v3/__https://tools.ietf.org/html/draft-ietf-pim-igmp-mld-extension-02__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFvcqNJMw$ 
> >
> >
> >
> > Please review (it’s a quick read) and let us know your opinion on it’s readiness to progress to the iesg.
> >
> >
> >
> > Thanks,
> >
> > mike
> >
> >
> > Juniper Business Use Only
> >
> > _______________________________________________
> > pim mailing list
> > [e-mail]
> > https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/pim__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFK1H-G2g$ 

_______________________________________________
pim mailing list
[e-mail]
https://urldefense.com/v3/__https://www.ietf.org/mailman/listinfo/pim__;!!GjvTz_vk!F2FaK3vVVztSY86qjUoxhNkRib0nxFe4anOD2g9Pz2D1NlNvf6Bs6fMFK1H-G2g$