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
| ||
|
Date: Mon, 13 Aug 2018 01:04:40 +0200 From: Daniel Borkmann <daniel@...earbox.net> To: Tariq Toukan <tariqt@...lanox.com>, Alexei Starovoitov <alexei.starovoitov@...il.com> Cc: "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org, Eran Ben Elisha <eranbe@...lanox.com>, Jesper Dangaard Brouer <brouer@...hat.com> Subject: Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning On 08/12/2018 10:45 AM, Tariq Toukan wrote: > > > On 20/07/2018 12:36 AM, Alexei Starovoitov wrote: >> On Wed, Jul 18, 2018 at 05:13:54PM +0300, Tariq Toukan wrote: >>> >>> >>> On 17/07/2018 10:27 PM, Daniel Borkmann wrote: >>>> On 07/17/2018 06:47 PM, Alexei Starovoitov wrote: >>>>> On Tue, Jul 17, 2018 at 06:10:38PM +0300, Tariq Toukan wrote: >>>>>> Fix the warning below by calling rhashtable_lookup under >>>>>> RCU read lock. >>>>>> >>> >>> ... >>> >>>>>> mutex_lock(&mem_id_lock); >>>>>> + rcu_read_lock(); >>>>>> xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params); >>>>>> + rcu_read_unlock(); >>>>>> if (!xa) { >>>>> >>>>> if it's an actual bug rcu_read_unlock seems to be misplaced. >>>>> It silences the warn, but rcu section looks wrong. >>>> >>>> I think that whole piece in __xdp_rxq_info_unreg_mem_model() should be: >>>> >>>> mutex_lock(&mem_id_lock); >>>> xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params); >>>> if (xa && rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params) == 0) >>>> call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free); >>>> mutex_unlock(&mem_id_lock); >>>> >>>> Technically the RCU read side plus rhashtable_lookup() is the same, but lets >>>> use proper api. From the doc (https://lwn.net/Articles/751374/) object removal >>>> is wrapped around the RCU read side additionally, but in our case we're behind >>>> mem_id_lock for insertion/removal serialization. >>>> >>>> Cheers, >>>> Daniel >>>> >>> >>> Just as Daniel stated, I think there's no actual bug here, but we still want >>> to silence the RCU warning. >>> >>> Alexei, did you mean getting the if statement into the RCU lock critical >>> section? >> >> If what Daniel proposes silences the warn, I'd rather do that. >> Pattern like: >> rcu_lock; >> val = lookup(); >> rcu_unlock; >> if (val) >> will cause people to question the quality of the code and whether >> authors of the code understand rcu. >> There should be a way to silence the warn without adding >> "wrong on the first glance" code. > > I'm re-spinning this. > Can it still go to net, or better send it to bpf-next ? Please rebase against bpf-next and we route it to stable, thanks!
Powered by blists - more mailing lists