netdev - Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning

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]

Date:   Mon, 13 Aug 2018 14:22:41 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Tariq Toukan <tariqt@...lanox.com>
Cc:     Alexei Starovoitov <ast@...nel.org>, netdev@...r.kernel.org,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Neil Brown <neilb@...e.com>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Subject: Re: [PATCH bpf-next V3] net/xdp: Fix suspicious RCU usage warning

On 08/13/2018 02:02 PM, Jesper Dangaard Brouer wrote:
> On Mon, 13 Aug 2018 13:57:04 +0300
> Tariq Toukan <tariqt@...lanox.com> wrote:
>> On 13/08/2018 1:31 PM, Jesper Dangaard Brouer wrote:
>>> On Mon, 13 Aug 2018 12:21:58 +0300
>>> Tariq Toukan <tariqt@...lanox.com> wrote:
[...]
>>> In the example[1], the sequence is wrapped in rcu_read_lock/unlock,
>>> while you have not done so. The rhashtable_lookup_fast and
>>> rhashtable_remove_fast calls have their own rcu_read_lock/unlock,
>>> but the outer rcu_read_lock/unlock, makes sure that a RCU period
>>> cannot slip in between the two calls.
>>>
>>> I still think your fix is correct, due to the mutex_lock.  Given the
>>> mutex sync removal and insert in this rhashtable.
>>
>> Right, we rely here on the mutex to avoid the scenario you described.
>> So the outer rcu lock calls are not necessary.

Agree.

>>> I do wonder if it would be better to add the outer
>>> rcu_read_lock/unlock, calls if someone else reads and copy-paste
>>> this code (and don't have an mutex sync scheme) ?
>>
>> Yeah it'll be safer for future unaware developers, but I think
>> reviewers should always comment and make it clear that the best
>> generic reference is [1], not any specific/optimized  use case.
>>
>> If you guys still want to me to fix this then please let me know and 
>> I'll re-spin.
> 
> I'll let Daniel make the choice.

Patch is fine as is. If we would be adding the RCU read lock/unlock pair
even though it's not necessary but for other developers to copy paste
from, I think this might be double-confusing: in the one case for people
reading the current code as they will wonder why the additional RCU read
side is needed here (so it will leave them puzzling), and in the other
case for people trying to copy-paste from it wondering whether they would
need similar scheme with mutex in addition. So I strongly prefer to 'do
the right thing' based on the situation. Given BPF PR is still pending,
I'll get the patch in once it has been pulled.

Thanks,
Daniel