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

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

[Maciej Żenczykowski]

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

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

[Maciej Żenczykowski]

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.

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

[Paolo Abeni]

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 29 Jul 2024 17:17:48 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [net,v2] ipv6: fix ndisc_is_useropt() handling for PIO
    https://git.kernel.org/netdev/net/c/a46c68debf3b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html