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

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

Hi Stig,

On 1/19/21, 10:21 AM, "Stig Venaas" <[e-mail]> wrote:
> I do believe TLVs really is flexible enough for any future extension
> and I mentioned this since people might question why we don't need
> Additional Data when the extension is used. But I can certainly remove
> this. The main point is that there is no data besides the Extension
> TLVs. I'll try to improve this text. Of course we are also able to
> extend IGMP/MLD in other ways when not setting the E-bit if we want.

I agree it's likely to be flexible enough, but I have minor reservations
about stating it as a known fact (as in -03), because it's hard to be
certain that all future needs will be fully compatible with the semantics
defined for these TLVs. (And I agree if it happens then it can be covered
by using something else other than the E-bit).

I'm not sure it needs anything pointing out the flexibility to future
needs, but if you think it will help forestall questions to include it,
even just "is flexible enough" -> "is expected to be flexible enough"
would address my reservation.

>> 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?
>
> The specs define both Additional Data and Auxiliary Data. The former
> is at the end of the IGMP/MLD messages, which we are using. The latter
> is potentially auxiliary data at the end of a group record. We are not
> using that.

Ah, thanks, my mistake.  I think the reason I missed it in my first review
was because I understood it correctly that time and then forgot.  Sorry
about that.

>> 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
...
>> 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
>
> Yes (except additional not auxiliary). Perhaps it is best if I refer
> to bytes remaining in the IP payload.

Yes, to me something like one of these would be an improvement over "in
the message":
- in the IP payload
- in the Additional Data field
- in the Additional Data field, which extends to the end of the IP payload

(please disregard the rest of my comment #2, it was based on my mistaken
reading thinking you were using Auxiliary data.)

>> 3.
...
> This sounds good. Thanks, I'll use this. It is pretty common to draw
> the figure like this even when no alignment is expected, but I think
> I'll point out that there is no alignment or padding.

Cool, thanks.  I agree the figure seems fine to me.

>> 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.)
>
> When the E-bit is set, one should be following this document, but of
> course there may be bugs or attacks. I'm fine with being strict as you
> suggest, the only downside is that one will have to validate this
> before applying any of the TLVs. Since the length does not have to be
> a multiple of 4, I think it might be best then to require that the
> last TLV ends at the end of the IP payload (no trailing bytes).

Yes, I think that's right.

I hope that's not too bad a downside, I think it sounds ok to me.  And on
the upside it's maybe slightly more likely to get length-checked properly.

It's a minor point though, if you prefer to leave it alone I won't object.
I maybe sometimes worry more than is necessary about the sad state of
buffer-overflow avoidance strategies in networking.


Thanks and regards,
Jake