netdev - Re: About ip6_dst

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 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]

Date:   Sun, 28 Apr 2019 07:57:54 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Ahern <dsahern@...il.com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: About ip6_dst_destroy()



On 4/27/19 8:22 PM, David Ahern wrote:
> On 4/27/19 5:56 PM, Eric Dumazet wrote:
>> David
>>
>> I am staring at ip6_dst_destroy() and the last part makes really no sense to me.
>>
>> How rcu_read_lock()/rcu_read_unnlock() can help in a writer side ???
>>
>> Changlog of a68886a691804d3f6d479ebf6825480fbafb6a00 ("net/ipv6: Make from in rt6_info rcu protected")
>> does not make sense either.
>>
>> <quote>
>>     There is a race window when a FIB entry is deleted and the 'from' on the
>>     pcpu route is dropped and the pcpu route hits a cookie check. Handle
>>     this race using rcu on from.
>> </quote>
>>
> 
> A FIB entry (fib6_info) is deleted, but resources are not cleaned up as
> there are outstanding references to the entry. Specifically, the
> references are the 'from' on pcpu routes. Commit (93531c6743157) added
> code to release those references as otherwise there is nothing that
> forces it. Further testing hit the condition noted in a68886a69180.
> 
> I presume you are asking about ip6_dst_destroy vs all of the other
> 'from' references because of the fib6_info_release - which would result
> in an underflow when it is released twice. I guess something like a
> rmb() / wmb() pair is needed for this case.

I do not see how rmb/wmb pair will help.

Writers need to use a stronger synchronization between themselves. 

This can be some spinlock, a xchg() or cmpxchg()

The problem here is that nothing prevent ip6_dst_destroy() being called concurrently
with another writer like fib6_drop_pcpu_from()

fib6_drop_pcpu_from() uses &table->tb6_lock, which is not held in ip6_dst_destroy()

I will submit a patch switching all writers to xchg()