| Message ID | 51234fd156acbe2161e928631cdc3d74b00002a7.1644505353.git.gnault@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b9605161e7be40fdd0fa0685b5c534e6201ac04b |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ipv6: Reject routes configurations that specify dsfield (tos) | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/checkpatch | warning | WARNING: line length of 85 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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>
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
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 >
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
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!
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 }
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(+)