Re: [pim] AD Review draft-ietf-pim-null-register-packing-11

[Alvaro Retana <[e-mail]>]

Ping!

On December 20, 2022 at 5:55:07 PM, Alvaro Retana ([e-mail])
wrote:


Dear authors:

Thank you for the work on this document.  In general, it looks a lot better
compared to when I read it last (-05) [1].  However, I am repeating some of
the comments I made back then -- see inline.

Thanks!

Alvaro.


[1] https://mailarchive.ietf.org/arch/msg/pim/NdNMqSwjwU9pvN0GGD2oXkpIuNI/



[Line numbers from idnits.]

...
15 Abstract
...
25   This document defines a standard to send multiple Multicast source
26   and group information in a single PIM Packed Null-Register message.
27   We will refer to the new packed formats as the PIM Packed Null-
28   Register format and PIM Packed Register-Stop format throughout the
29   document.  This document also discusses interoperability between the
30   PIM routers which do not understand the PIM Packed Null-Register
31   format and routers which do understand it.

[minor] The "PIM Packed Null-Register message" is mentioned before it is
described.

s/in a single PIM Packed Null-Register message/in a single PIM message


[style nit] Please write in third person, not frst person.

s/We will refer to/This document refers to


[minor] Is it a "format" or a "message"?  I understand that the message is
formatted differently (it is new!), but the use of "format" to (apparently)
mean "message" is confusing.

s/PIM Packed Null-Register format and PIM Packed Register-Stop format/PIM
Packed Null-Register message and PIM Packed Register-Stop message



...
85 1.  Introduction

87   PIM Null-Registers are sent by the DR periodically for Multicast
88   streams to keep the states active on the RP, as long as the multicast
89   source is alive.  As the number of multicast sources increases, the
90   number of PIM Null-Register messages that are sent also increases.
91   This results in more PIM packet processing at the RP and the DR.

[] Suggestion:

s/
  PIM Null-Registers are sent by the DR periodically for Multicast streams
to
  keep the states active on the RP, as long as the multicast source is
alive.
/
  The DR periodically sends PIM Null-Registers to keep the state of
existing
  multicast sources active on the RP.



93   The control plane policing (COPP), monitors the packets that are
94   processed by the control plane.  The high rate at which Null-
95   Registers are received at the RP can lead to COPP drops of Multicast
96   PIM Null-Register messages.  This draft proposes a method to
97   efficiently pack multiple PIM Null-Registers [RFC7761] (Section 4.4)
98   and Register-Stops [RFC7761](Section 3.2) into a single message as
99   these packets anyway do not contain encapsulated data.

[minor] COPP:  Is there a reference for that?  A vendor-independent
reference would be ideal, but I don't think that this document needs to
mention COPP to justify the enhancement.


[nit] The section numbers are not needed.


[nit] s/these packets anyway do not contain encapsulated data/these packets
do not contain encapsulated data



...
118 2.  Packed Null-Register Capability

120   A router (DR) can decide to pack multiple Null-Register messages
121   based on the capability received from the RP as part of the PIM
122   Register-Stop.  This ensures compatibility with routers that do not
123   support processing of the new format.  The capability information can
124   be indicated by the RP via the PIM Register-Stop message sent to the
125   DR.  Thus a DR will switch to the new format only when it learns that
126   the RP is capable of handling the PIM Packed Null-Register messages.

128   Conversely, a DR that does not support the packed format can continue
129   generating the PIM Null-Register as defined in [RFC7761]
130   (Section 4.4).  To exchange the capability information in the
131   Register-Stop message, the "Reserved" field can be used to indicate
132   this capability in those Register-Stop messages.  One bit of the
133   Reserved field is used to indicate the "packing" capability (P bit).
134   The rest of the bits in the "Reserved" field will be retained for
135   future use.

[major] In this section, please only specify the new capability bit...the
description of the operation should be in just one place.  That description
is in §5.

Because the interoperability with routers that don't support this extension
is mentioned in the Abstract and Introduction, please add a new section to
explicitly talk about that (it can be just a few sentences long), perhaps
as a sub-section of the Operational Considerations.

Suggestion>
   This section allocates a bit in the PIM Register-Stop message
   Flag Bits field for the RP to indicate its ability to receive
   PIM Packed Null-Register messages (Section 3), and send PIM
   Packed Register-Stop messages (Section 4).


[major] The common PIM header was updated by rfc8736, so the discussion
above about the Reserved field and the figure below are out of date.

The new common header is this:

    0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |PIM Ver| Type  |   Flag Bits   |           Checksum            |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

...so the discussion should be about the allocation of the Flag Bits.



...
149   PIM Version, Type, Checksum, Group Address, Source Address:

151      Same as [RFC7761] (Section 4.9.4)

[major] Suggestion:

   The fields in the PIM Register-Stop message are defined in Section
   4.9.4 of [RFC7761], and the common header in [RFC8736].


153   P:

155      Capability bit (flag bit 7) used to indicate support for the
156      Packed Null-Register Capability

[minor] Suggestion:

   P: When set, it indicates the ability of the RP to receive
      PIM Packed Null-Register messages, and send PIM Packed
      Register-Stop messages.


158 3.  PIM Packed Null-Register message format

160   PIM Packed Null-Register message format includes a count to indicate
161   the number of Null-Register records in the message.

[nit] s/count/Count field


163       0                   1                   2                   3
164       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
165      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
166      |PIM Ver| Type  |Subtype|  FB   |           Checksum            |
167      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
168      |   Count       |              Reserved                         |
169      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
170      |     Group Address[1]   (Encoded-Group format)                 |
171      |     Source Address[1]  (Encoded-Unicast format)               |
172      .                                                               .
173      .                                                               .
174      .                                                               .
175      .                                                               .
176      .     Group Address[N]                                          .
177      |     Source Address[N]                                         |
178      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

180              Figure 2: PIM Packed Null-Register message format

182   PIM Version, Reserved, Checksum:

184      Same as [RFC7761] (Section 4.9.3)

[major] Again, the updated common header definitions come from rfc8736.


186   Type, SubType:

188      The new packed Null-Register Type and SubType values TBD.
189      [RFC8736]

[major] Type and SubType are not different values -- rfc8736.


191   Count:

193      The number of packed Null-Register records.  A record consists of
194      a Group Address and Source Address pair.

[major] The length of the message is not explicit in the PIM message, but
it is inferred from the IP layer.  What should the receiver do if the count
indicates a number of records that would result in a message with a length
different from that is expected?



...
200 4.  PIM Packed Register-Stop message format

[] The comments from the last section apply here too.



...
241 5.  Protocol operation

243   The following combinations exist -

[] The different combinations don't need to be mentioned.  Instead, the
specification for case 1 should be enough to cover all of them.

245   1.  DR and RP both support the PIM Packed Null-Register and PIM
246       Packed Register-Stop formats:

248       *  As specified in [RFC7761], the DR sends PIM Register messages
249          towards the RP when a new source is detected.

251       *  An RP supporting this specification MUST set the P-bit in the
252          corresponding Register-Stop messages.

[major]  Is receiving the  P-bit set in one Register-Stop message enough
for the
DR to use the new messages for all future communication...OR...should the
P-bit
first be set for specific (S,G)/(*,G) pairs before the DR can use the new
messages for them?  IOW, is the P-bit supposed to indicate per (S,G)/(*,G)
support or per RP?


[major] If setting the P-bit, does it have to be set in all messages or can
it be unset after the DR starts using the new messages?  The text above
talks about "the corresponding Register-Stop messages", so it is not clear.

The text in §8 seems to indicate that the P-bit should always be set.  ??


254       *  When a Register-Stop message with the P-bit set is received,
255          the DR SHOULD send PIM Packed Null-Register messages
256          (Section 3) to the RP instead of multiple Register messages
257          with the N-bit set [RFC7761].

[major] When it is ok for the DR to not use PIM Packed Null-Register
messages?  IOW, why is this action recommended and not required?

I assume that even if supported, the DR may decide (by configuration??)
that if doesn't want to use the new messages.  If this is the
justification, then it seems to me that a better Normative keyword would be
"MAY" (instead of SHOULD).


[major] Once the DR starts using the new messages, does it have to always
use them?  Can a mixture of packed and unpacked messages be used?


259       *  The RP, after receiving a PIM Packed Null-Register message
260          SHOULD start sending PIM Packed Register-Stop messages
261          (Section 4) to the corresponding DR instead of individual
262          Register-Stop messages.

[major] When it is ok for the RP to not use PIM Packed Register-Stop
messages?  IOW, why is this action recommended and not required?

I assume that even if supported, the RP may decide (by configuration??)
that if doesn't want to use the new messages.  If this is the
justification, then it seems to me that a better Normative keyword would be
"MAY" (instead of SHOULD).


[major] Once the RP starts using the new messages, does it have to always
use them?  Can a mixture of packed and unpacked messages be used?



...
289 6.  Operational Considerations

291   In case the network manager disables the packed capability at the RP,
292   the router should not advertise the capability.  However, an
293   implementation MAY choose to still parse any packed registers if they
294   are received.  This may be particularly useful in the transitional
295   period after the network manager disables it.

[major] The specification in §5 says that "An RP supporting this
specification MUST set the P-bit...".  That is an absolute requirement that
doesn't account for configuration.

Perhaps you want add this exception to the specification above: When
enabled/configured, the RP MUST set the P-bit...  (or something like that)


297 7.  PIM Anycast RP Considerations

[] This seems to be an operational consideration.  It might be good to make
this a sub-section of §6.


299   The PIM Packed Null-Register format should be enabled only if it is
300   supported by all PIM Anycast RP [RFC4610] members in the RP set for
301   the RP address.  This consideration applies to PIM Anycast RP with
302   MSDP [RFC3446] as well.

[major] See the comments above about the (unqualified) requirement in §5 to
set the P-bit.


[major] s/by all PIM anycast RP members [RFC4610] in the RP set for the RP
address./by all the routers in the Anycast-RP set ([rfc4610]).


[major] If the DR receives the P-bit from an RP, should it assume that the
condition is met, or is there a way to verify?


[major] "should be enabled"  Is Normative language needed?   What is the
impact of a partial deployment?


304 8.  PIM RP router version downgrade

[] This seems to be an operational consideration.  It might be good to make
this a sub-section of §6.


306   Consider a PIM RP router that supports PIM Packed Null-Registers and
307   PIM Packed Register-Stops.  When this router downgrades to a software
308   version which does not support PIM Packed Null-Registers and PIM
309   Packed Register-Stops, the DR that sends the PIM Packed Null-Register
310   message will not get a PIM Register-Stop message back from the RP.
311   In such scenarios the DR can send an unpacked PIM Null-Register and
312   check the PIM Register-Stop to see if the capability bit (P-bit) for
313   PIM Packed Null-Register is set or not.  If it is not set then the DR
314   will continue sending unpacked PIM Null-Register messages.

[major] "In such scenarios the DR can..."

How does the DR detect this situation?  Is there a timer (rfc7761 ??) that
can alert the DR of the lack of support?



...
324 10.  Security Considerations

[] Most of these considerations are the same as the ones already covered in
rfc7761.  There's no need to repeat them here.  Also, additional
requirements (for example, "standard methods to prevent spoofing") should
not be suggested as they would better belong in the base specification.

Suggestion>

   The Security Considerations from [RFC7761] apply to this document.  In
   particular, the effect of forging a PIM Packed Null-Register or
   Register-Stop message would be amplified to all the records included and
   not a single source/group pair.



...
345   There is another case where a spoofed Register-Stop can be sent to
346   make it appear that is from the RP, and that the RP supports this new
347   packed capability when it does not.  This can cause Null-Registers to
348   be sent to an RP that doesnt support this packed format.  But
349   standard methods to prevent spoofing should take care of this case.
350   For example, uRPF can be used to filter out packets coming from the
351   outside from addresses that belong to routers inside.

[] This last paragraph points at a different effect of a forged packet:
indicating support where it doesn't exist.

Suggestion>

   By forging a PIM Register-Stop message and setting the P-bit,
   an attacker can trigger the use of PIM Packed Null-Register
   messages by a DR thus creating unnecessary churn in the network.

Is that the impact?


353 11.  IANA Considerations

[major] This section should contain a specific request to IANA ("IANA is
asked to assign..."), along with the specific names of the registries, and
of the new bits/messages (!).


355      This document requires the assignment of Capability bit (P-bit),
356      flag bit 7 in the PIM Register-Stop message.

[major] A value can be suggested, not requested.


358      This document requires the assignment of 2 new PIM message types
359      for the PIM Packed Null-Register and PIM Packed Register-Stop.

[major] Please include a short table that can be used my IANA to fill out
the registry information.  Note that both requests come from the same
registry.



...
395 13.2.  Informative References

397   [RFC3446]  Kim, D., Meyer, D., Kilmer, H., and D. Farinacci, "Anycast
398              Rendevous Point (RP) mechanism using Protocol Independent
399              Multicast (PIM) and Multicast Source Discovery Protocol
400              (MSDP)", RFC 3446, DOI 10.17487/RFC3446, January 2003,
401              <https://www.rfc-editor.org/info/rfc3446>.

[major] This reference should be Normative.

[EoR -11]