[PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[Guillaume Nault]

The ->rtm_tos option is normally used to route packets based on both
the destination address and the DS field. However it's ignored for
IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
is going to work only on the destination address anyway, so it won't
behave as specified.

Suggested-by: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
The same problem exists for ->rtm_scope. I'm working only on ->rtm_tos
here because IPv4 recently started to validate this option too (as part
of the DSCP/ECN clarification effort).
I'll give this patch some soak time, then send another one for
rejecting ->rtm_scope in IPv6 routes if nobody complains.

 net/ipv6/route.c                         |  6 ++++++
 tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f4884cda13b9..dd98a11fbdb6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -5009,6 +5009,12 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
 	err = -EINVAL;
 	rtm = nlmsg_data(nlh);
 
+	if (rtm->rtm_tos) {
+		NL_SET_ERR_MSG(extack,
+			       "Invalid dsfield (tos): option not available for IPv6");
+		goto errout;
+	}
+
 	*cfg = (struct fib6_config){
 		.fc_table = rtm->rtm_table,
 		.fc_dst_len = rtm->rtm_dst_len,
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index bb73235976b3..e2690cc42da3 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -988,12 +988,25 @@ ipv6_rt_replace()
 	ipv6_rt_replace_mpath
 }
 
+ipv6_rt_dsfield()
+{
+	echo
+	echo "IPv6 route with dsfield tests"
+
+	run_cmd "$IP -6 route flush 2001:db8:102::/64"
+
+	# IPv6 doesn't support routing based on dsfield
+	run_cmd "$IP -6 route add 2001:db8:102::/64 dsfield 0x04 via 2001:db8:101::2"
+	log_test $? 2 "Reject route with dsfield"
+}
+
 ipv6_route_test()
 {
 	route_setup
 
 	ipv6_rt_add
 	ipv6_rt_replace
+	ipv6_rt_dsfield
 
 	route_cleanup
 }
-- 
2.21.3

Re: [PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[David Ahern]

On 2/10/22 7:08 AM, Guillaume Nault wrote:
> The ->rtm_tos option is normally used to route packets based on both
> the destination address and the DS field. However it's ignored for
> IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
> is going to work only on the destination address anyway, so it won't
> behave as specified.
> 
> Suggested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
> The same problem exists for ->rtm_scope. I'm working only on ->rtm_tos
> here because IPv4 recently started to validate this option too (as part
> of the DSCP/ECN clarification effort).
> I'll give this patch some soak time, then send another one for
> rejecting ->rtm_scope in IPv6 routes if nobody complains.
> 
>  net/ipv6/route.c                         |  6 ++++++
>  tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 


Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[Shuah Khan]

On 2/10/22 8:08 AM, Guillaume Nault wrote:
> The ->rtm_tos option is normally used to route packets based on both
> the destination address and the DS field. However it's ignored for
> IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
> is going to work only on the destination address anyway, so it won't
> behave as specified.
> 
> Suggested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> ---
> The same problem exists for ->rtm_scope. I'm working only on ->rtm_tos
> here because IPv4 recently started to validate this option too (as part
> of the DSCP/ECN clarification effort).
> I'll give this patch some soak time, then send another one for
> rejecting ->rtm_scope in IPv6 routes if nobody complains.
> 
>   net/ipv6/route.c                         |  6 ++++++
>   tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f4884cda13b9..dd98a11fbdb6 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -5009,6 +5009,12 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
>   	err = -EINVAL;
>   	rtm = nlmsg_data(nlh);
>   
> +	if (rtm->rtm_tos) {
> +		NL_SET_ERR_MSG(extack,
> +			       "Invalid dsfield (tos): option not available for IPv6");

Is this an expected failure on ipv6, in which case should this test report
pass? Should it print "failed as expected" or is returning fail from errout
is what should happen?

> +		goto errout;
> +	}
> +
>   	*cfg = (struct fib6_config){
>   		.fc_table = rtm->rtm_table,
>   		.fc_dst_len = rtm->rtm_dst_len,
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index bb73235976b3..e2690cc42da3 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -988,12 +988,25 @@ ipv6_rt_replace()
>   	ipv6_rt_replace_mpath
>   }
>   
> +ipv6_rt_dsfield()
> +{
> +	echo
> +	echo "IPv6 route with dsfield tests"
> +
> +	run_cmd "$IP -6 route flush 2001:db8:102::/64"
> +
> +	# IPv6 doesn't support routing based on dsfield
> +	run_cmd "$IP -6 route add 2001:db8:102::/64 dsfield 0x04 via 2001:db8:101::2"
> +	log_test $? 2 "Reject route with dsfield"
> +}
> +
>   ipv6_route_test()
>   {
>   	route_setup
>   
>   	ipv6_rt_add
>   	ipv6_rt_replace
> +	ipv6_rt_dsfield
>   
>   	route_cleanup
>   }
> 

With the above comment addressed or explained.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[Guillaume Nault]

On Thu, Feb 10, 2022 at 11:23:20AM -0700, Shuah Khan wrote:
> On 2/10/22 8:08 AM, Guillaume Nault wrote:
> > The ->rtm_tos option is normally used to route packets based on both
> > the destination address and the DS field. However it's ignored for
> > IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
> > is going to work only on the destination address anyway, so it won't
> > behave as specified.
> > 
> > Suggested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> > Signed-off-by: Guillaume Nault <gnault@redhat.com>
> > ---
> > The same problem exists for ->rtm_scope. I'm working only on ->rtm_tos
> > here because IPv4 recently started to validate this option too (as part
> > of the DSCP/ECN clarification effort).
> > I'll give this patch some soak time, then send another one for
> > rejecting ->rtm_scope in IPv6 routes if nobody complains.
> > 
> >   net/ipv6/route.c                         |  6 ++++++
> >   tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
> >   2 files changed, 19 insertions(+)
> > 
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index f4884cda13b9..dd98a11fbdb6 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -5009,6 +5009,12 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
> >   	err = -EINVAL;
> >   	rtm = nlmsg_data(nlh);
> > +	if (rtm->rtm_tos) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Invalid dsfield (tos): option not available for IPv6");
> 
> Is this an expected failure on ipv6, in which case should this test report
> pass? Should it print "failed as expected" or is returning fail from errout
> is what should happen?

This is an expected failure. When ->rtm_tos is set, iproute2 fails with
error code 2 and prints
"Error: Invalid dsfield (tos): option not available for IPv6.".

The selftest redirects stderr to /dev/null by default (unless -v is
passed on the command line) and expects the command to fail and
return 2. So the default output is just:

IPv6 route with dsfield tests
    TEST: Reject route with dsfield                                     [ OK ]

Of course, on a kernel that accepts non-null ->rtm_tos, "[ OK ]"
becomes "[FAIL]", and the the failed tests couter is incremented.

> > +		goto errout;
> > +	}
> > +
> >   	*cfg = (struct fib6_config){
> >   		.fc_table = rtm->rtm_table,
> >   		.fc_dst_len = rtm->rtm_dst_len,
> > diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> > index bb73235976b3..e2690cc42da3 100755
> > --- a/tools/testing/selftests/net/fib_tests.sh
> > +++ b/tools/testing/selftests/net/fib_tests.sh
> > @@ -988,12 +988,25 @@ ipv6_rt_replace()
> >   	ipv6_rt_replace_mpath
> >   }
> > +ipv6_rt_dsfield()
> > +{
> > +	echo
> > +	echo "IPv6 route with dsfield tests"
> > +
> > +	run_cmd "$IP -6 route flush 2001:db8:102::/64"
> > +
> > +	# IPv6 doesn't support routing based on dsfield
> > +	run_cmd "$IP -6 route add 2001:db8:102::/64 dsfield 0x04 via 2001:db8:101::2"
> > +	log_test $? 2 "Reject route with dsfield"
> > +}
> > +
> >   ipv6_route_test()
> >   {
> >   	route_setup
> >   	ipv6_rt_add
> >   	ipv6_rt_replace
> > +	ipv6_rt_dsfield
> >   	route_cleanup
> >   }
> > 
> 
> With the above comment addressed or explained.
> 
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> thanks,
> -- Shuah
> 

Re: [PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[Shuah Khan]

On 2/10/22 3:05 PM, Guillaume Nault wrote:
> On Thu, Feb 10, 2022 at 11:23:20AM -0700, Shuah Khan wrote:
>> On 2/10/22 8:08 AM, Guillaume Nault wrote:
>>> The ->rtm_tos option is normally used to route packets based on both
>>> the destination address and the DS field. However it's ignored for
>>> IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
>>> is going to work only on the destination address anyway, so it won't
>>> behave as specified.
>>>
>>> Suggested-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>> Signed-off-by: Guillaume Nault <gnault@redhat.com>
>>> ---
>>> The same problem exists for ->rtm_scope. I'm working only on ->rtm_tos
>>> here because IPv4 recently started to validate this option too (as part
>>> of the DSCP/ECN clarification effort).
>>> I'll give this patch some soak time, then send another one for
>>> rejecting ->rtm_scope in IPv6 routes if nobody complains.
>>>
>>>    net/ipv6/route.c                         |  6 ++++++
>>>    tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
>>>    2 files changed, 19 insertions(+)
>>>
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index f4884cda13b9..dd98a11fbdb6 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -5009,6 +5009,12 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
>>>    	err = -EINVAL;
>>>    	rtm = nlmsg_data(nlh);
>>> +	if (rtm->rtm_tos) {
>>> +		NL_SET_ERR_MSG(extack,
>>> +			       "Invalid dsfield (tos): option not available for IPv6");
>>
>> Is this an expected failure on ipv6, in which case should this test report
>> pass? Should it print "failed as expected" or is returning fail from errout
>> is what should happen?
> 
> This is an expected failure. When ->rtm_tos is set, iproute2 fails with
> error code 2 and prints
> "Error: Invalid dsfield (tos): option not available for IPv6.".
> 
> The selftest redirects stderr to /dev/null by default (unless -v is
> passed on the command line) and expects the command to fail and
> return 2. So the default output is just:
> 
> IPv6 route with dsfield tests
>      TEST: Reject route with dsfield                                     [ OK ]
> 
> Of course, on a kernel that accepts non-null ->rtm_tos, "[ OK ]"
> becomes "[FAIL]", and the the failed tests couter is incremented.
> 

Sounds good to me.

>>
>> With the above comment addressed or explained.
>>
>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
>>

thanks,
-- Shuah

Re: [PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 10 Feb 2022 16:08:08 +0100 you wrote:
> The ->rtm_tos option is normally used to route packets based on both
> the destination address and the DS field. However it's ignored for
> IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
> is going to work only on the destination address anyway, so it won't
> behave as specified.
> 
> Suggested-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> 
> [...]

Here is the summary with links:
  - [net-next] ipv6: Reject routes configurations that specify dsfield (tos)
    https://git.kernel.org/netdev/net-next/c/b9605161e7be

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