From: David Ahern <hidden> Date: 2018-03-13 15:40:18
ONLINK flag is per nexthop. For multipath routes it is specified in rtnh_flags
in struct rtnexthop versus rtm_flags for single path routes. Update
ip6_route_multipath_add to look at rtnh_flags before the call to
ip6_route_info_create.
Add multipath test cases as well.
David Ahern (2):
net/ipv6: Handle onlink flag with multipath routes
selftests: Add multipath tests for onlink flag
net/ipv6/route.c | 1 +
tools/testing/selftests/net/fib-onlink-tests.sh | 92 ++++++++++++++++++++++++-
2 files changed, 90 insertions(+), 3 deletions(-)
--
2.11.0
From: David Ahern <hidden> Date: 2018-03-13 15:40:19
For multipath routes the ONLINK flag is specified per nexthop in rtnh_flags
(as opposed to rtm_flags for unicast routes). Update ip6_route_multipath_add
to set fc_flags based on rtnh_flags.
Fixes: fc1e64e1092f ("net/ipv6: Add support for onlink flag")
Signed-off-by: David Ahern <redacted>
---
net/ipv6/route.c | 1 +
1 file changed, 1 insertion(+)
From: David Ahern <hidden> Date: 2018-03-13 15:40:21
Add multipath tests for onlink flag: one test with onlink added to
both nexthops, then tests with onlink added to only 1 nexthop.
Signed-off-by: David Ahern <redacted>
---
tools/testing/selftests/net/fib-onlink-tests.sh | 92 ++++++++++++++++++++++++-
1 file changed, 89 insertions(+), 3 deletions(-)
From: David Miller <davem@davemloft.net> Date: 2018-03-16 15:40:40
From: David Ahern <redacted>
Date: Tue, 13 Mar 2018 08:40:09 -0700
quoted hunk
For multipath routes the ONLINK flag is specified per nexthop in rtnh_flags
(as opposed to rtm_flags for unicast routes). Update ip6_route_multipath_add
to set fc_flags based on rtnh_flags.
Fixes: fc1e64e1092f ("net/ipv6: Add support for onlink flag")
Signed-off-by: David Ahern <redacted>
---
net/ipv6/route.c | 1 +
1 file changed, 1 insertion(+)
@@ -4166,6 +4166,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,r_cfg.fc_encap_type=nla_get_u16(nla);}+r_cfg.fc_flags|=(rtnh->rtnh_flags&RTNH_F_ONLINK);rt=ip6_route_info_create(&r_cfg,extack);if(IS_ERR(rt)){err=PTR_ERR(rt);
Hmmm, this actually "accumulates" the flag rather than sets it.
Have you thought about what should happen if the cfg has RTNH_F_ONLINK
set?
I think you should either change this logic to a true 'set', or adjust
your commit message to address this aspect of the new behavior.
Thank you.
From: David Ahern <hidden> Date: 2018-03-16 15:45:14
On 3/16/18 8:40 AM, David Miller wrote:
From: David Ahern <redacted>
Date: Tue, 13 Mar 2018 08:40:09 -0700
quoted
For multipath routes the ONLINK flag is specified per nexthop in rtnh_flags
(as opposed to rtm_flags for unicast routes). Update ip6_route_multipath_add
to set fc_flags based on rtnh_flags.
Fixes: fc1e64e1092f ("net/ipv6: Add support for onlink flag")
Signed-off-by: David Ahern <redacted>
---
net/ipv6/route.c | 1 +
1 file changed, 1 insertion(+)
@@ -4166,6 +4166,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,r_cfg.fc_encap_type=nla_get_u16(nla);}+r_cfg.fc_flags|=(rtnh->rtnh_flags&RTNH_F_ONLINK);rt=ip6_route_info_create(&r_cfg,extack);if(IS_ERR(rt)){err=PTR_ERR(rt);
Hmmm, this actually "accumulates" the flag rather than sets it.
Have you thought about what should happen if the cfg has RTNH_F_ONLINK
set?
yes, that's why the test script adds cases with the ONLINK flag set for
both nexthops, then one with it on the first nexthop only, and one with
the flag only on the second nexthop.
If you look at the full loop 'cfg' is the variable with the user data,
and r_cfg is the 'local loop version' so r_cfg.fc_flags gets reset for
each nexthop:
while (rtnh_ok(rtnh, remaining)) {
memcpy(&r_cfg, cfg, sizeof(*cfg));
...
r_cfg.fc_flags |= (rtnh->rtnh_flags & RTNH_F_ONLINK);
rt = ip6_route_info_create(&r_cfg, extack);
I think you should either change this logic to a true 'set', or adjust
your commit message to address this aspect of the new behavior.
From: David Miller <davem@davemloft.net> Date: 2018-03-16 16:38:50
From: David Ahern <redacted>
Date: Fri, 16 Mar 2018 08:45:10 -0700
On 3/16/18 8:40 AM, David Miller wrote:
quoted
Hmmm, this actually "accumulates" the flag rather than sets it.
Have you thought about what should happen if the cfg has RTNH_F_ONLINK
set?
yes, that's why the test script adds cases with the ONLINK flag set for
both nexthops, then one with it on the first nexthop only, and one with
the flag only on the second nexthop.
If you look at the full loop 'cfg' is the variable with the user data,
and r_cfg is the 'local loop version' so r_cfg.fc_flags gets reset for
each nexthop:
while (rtnh_ok(rtnh, remaining)) {
memcpy(&r_cfg, cfg, sizeof(*cfg));
...
r_cfg.fc_flags |= (rtnh->rtnh_flags & RTNH_F_ONLINK);
rt = ip6_route_info_create(&r_cfg, extack);
Right.
quoted
I think you should either change this logic to a true 'set', or adjust
your commit message to address this aspect of the new behavior.
From: David Ahern <hidden> Date: 2018-03-16 18:18:58
On 3/16/18 9:38 AM, David Miller wrote:
From: David Ahern <redacted>
Date: Fri, 16 Mar 2018 08:45:10 -0700
quoted
On 3/16/18 8:40 AM, David Miller wrote:
quoted
Hmmm, this actually "accumulates" the flag rather than sets it.
Have you thought about what should happen if the cfg has RTNH_F_ONLINK
set?
yes, that's why the test script adds cases with the ONLINK flag set for
both nexthops, then one with it on the first nexthop only, and one with
the flag only on the second nexthop.
If you look at the full loop 'cfg' is the variable with the user data,
and r_cfg is the 'local loop version' so r_cfg.fc_flags gets reset for
each nexthop:
while (rtnh_ok(rtnh, remaining)) {
memcpy(&r_cfg, cfg, sizeof(*cfg));
...
r_cfg.fc_flags |= (rtnh->rtnh_flags & RTNH_F_ONLINK);
rt = ip6_route_info_create(&r_cfg, extack);
Right.
quoted
quoted
I think you should either change this logic to a true 'set', or adjust
your commit message to address this aspect of the new behavior.
I can update the commit message.
Please do, thanks David.
And it looks like patch 1 needs to be in 'net'; did not realize the ipv6
ONLINK support is already in 4.16. I will send the patch adding tests
separately for net-next.