netdev - Re: [PATCH net-next 1/2] net/ipv6: Handle onlink flag with multipath routes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3e2977f7-156e-aad9-ef6a-43733fc7ae4d@gmail.com>
Date:   Fri, 16 Mar 2018 08:45:10 -0700
From:   David Ahern <dsahern@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net/ipv6: Handle onlink flag with multipath
 routes

On 3/16/18 8:40 AM, David Miller wrote:
> From: David Ahern <dsahern@...il.com>
> Date: Tue, 13 Mar 2018 08:40:09 -0700
> 
>> 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 <dsahern@...il.com>
>> ---
>>  net/ipv6/route.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index f0ae58424c45..b223dffa8521 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -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.

I can update the commit message.