Re: [PATCH net v2] ipv6: fix ndisc_is_useropt() handling for PIO

[Paolo Abeni <pabeni@redhat.com>]

On 7/30/24 02:27, Maciej Żenczykowski wrote:
> On Mon, Jul 29, 2024 at 5:17 PM Maciej Żenczykowski <maze@google.com> wrote:
>>
>> The current logic only works if the PIO is between two
>> other ND user options.  This fixes it so that the PIO
>> can also be either before or after other ND user options
>> (for example the first or last option in the RA).
>>
>> side note: there's actually Android tests verifying
>> a portion of the old broken behaviour, so:
>>    https://android-review.googlesource.com/c/kernel/tests/+/3196704
>> fixes those up.
>>
>> Cc: Jen Linkova <furry@google.com>
>> Cc: Lorenzo Colitti <lorenzo@google.com>
>> Cc: Patrick Rohr <prohr@google.com>
>> Cc: David Ahern <dsahern@kernel.org>
>> Cc: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
>> Cc: Jakub Kicinski <kuba@kernel.org>
>> Signed-off-by: Maciej Żenczykowski <maze@google.com>
>> Fixes: 048c796beb6e ("ipv6: adjust ndisc_is_useropt() to also return true for PIO")
>> ---
>>   net/ipv6/ndisc.c | 34 ++++++++++++++++++----------------
>>   1 file changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
>> index 70a0b2ad6bd7..b8eec1b6cc2c 100644
>> --- a/net/ipv6/ndisc.c
>> +++ b/net/ipv6/ndisc.c
>> @@ -227,6 +227,7 @@ struct ndisc_options *ndisc_parse_options(const struct net_device *dev,
>>                  return NULL;
>>          memset(ndopts, 0, sizeof(*ndopts));
>>          while (opt_len) {
>> +               bool unknown = false;
>>                  int l;
>>                  if (opt_len < sizeof(struct nd_opt_hdr))
>>                          return NULL;
>> @@ -262,22 +263,23 @@ struct ndisc_options *ndisc_parse_options(const struct net_device *dev,
>>                          break;
>>   #endif
>>                  default:
>> -                       if (ndisc_is_useropt(dev, nd_opt)) {
>> -                               ndopts->nd_useropts_end = nd_opt;
>> -                               if (!ndopts->nd_useropts)
>> -                                       ndopts->nd_useropts = nd_opt;
>> -                       } else {
>> -                               /*
>> -                                * Unknown options must be silently ignored,
>> -                                * to accommodate future extension to the
>> -                                * protocol.
>> -                                */
>> -                               ND_PRINTK(2, notice,
>> -                                         "%s: ignored unsupported option; type=%d, len=%d\n",
>> -                                         __func__,
>> -                                         nd_opt->nd_opt_type,
>> -                                         nd_opt->nd_opt_len);
>> -                       }
>> +                       unknown = true;
>> +               }
>> +               if (ndisc_is_useropt(dev, nd_opt)) {
>> +                       ndopts->nd_useropts_end = nd_opt;
>> +                       if (!ndopts->nd_useropts)
>> +                               ndopts->nd_useropts = nd_opt;
>> +               } else if (unknown) {
>> +                       /*
>> +                        * Unknown options must be silently ignored,
>> +                        * to accommodate future extension to the
>> +                        * protocol.
>> +                        */
>> +                       ND_PRINTK(2, notice,
>> +                                 "%s: ignored unsupported option; type=%d, len=%d\n",
>> +                                 __func__,
>> +                                 nd_opt->nd_opt_type,
>> +                                 nd_opt->nd_opt_len);
>>                  }
>>   next_opt:
>>                  opt_len -= l;
>> --
>> 2.46.0.rc1.232.g9752f9e123-goog
>>
> 
> The diff on this second version is significantly bigger (although it's
> just unindenting a block), but perhaps this is better as it is much
> harder to screw things up.

Makes sense to me.

It would be great if you could follow-up with a self-test covering this.

Thanks,

Paolo