[PATCH net] net/page_pool: Fix inconsistent lock state warning

[PATCH net] net/page_pool: Fix inconsistent lock state warning

[Tariq Toukan]

Fix the warning below by calling the ptr_ring_consume_bh,
which uses spin_[un]lock_bh.

[  179.064300] ================================
[  179.069073] WARNING: inconsistent lock state
[  179.073846] 4.18.0-rc2+ #18 Not tainted
[  179.078133] --------------------------------
[  179.082907] inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
[  179.089637] swapper/21/0 [HC0[0]:SC1[1]:HE1:SE0] takes:
[  179.095478] 00000000963d1995 (&(&r->consumer_lock)->rlock){+.?.}, at:
__page_pool_empty_ring+0x61/0x100
[  179.105988] {SOFTIRQ-ON-W} state was registered at:
[  179.111443]   _raw_spin_lock+0x35/0x50
[  179.115634]   __page_pool_empty_ring+0x61/0x100
[  179.120699]   page_pool_destroy+0x32/0x50
[  179.125204]   mlx5e_free_rq+0x38/0xc0 [mlx5_core]
[  179.130471]   mlx5e_close_channel+0x20/0x120 [mlx5_core]
[  179.136418]   mlx5e_close_channels+0x26/0x40 [mlx5_core]
[  179.142364]   mlx5e_close_locked+0x44/0x50 [mlx5_core]
[  179.148509]   mlx5e_close+0x42/0x60 [mlx5_core]
[  179.153936]   __dev_close_many+0xb1/0x120
[  179.158749]   dev_close_many+0xa2/0x170
[  179.163364]   rollback_registered_many+0x148/0x460
[  179.169047]   rollback_registered+0x56/0x90
[  179.174043]   unregister_netdevice_queue+0x7e/0x100
[  179.179816]   unregister_netdev+0x18/0x20
[  179.184623]   mlx5e_remove+0x2a/0x50 [mlx5_core]
[  179.190107]   mlx5_remove_device+0xe5/0x110 [mlx5_core]
[  179.196274]   mlx5_unregister_interface+0x39/0x90 [mlx5_core]
[  179.203028]   cleanup+0x5/0xbfc [mlx5_core]
[  179.208031]   __x64_sys_delete_module+0x16b/0x240
[  179.213640]   do_syscall_64+0x5a/0x210
[  179.218151]   entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  179.224218] irq event stamp: 334398
[  179.228438] hardirqs last  enabled at (334398): [<ffffffffa511d8b7>]
rcu_process_callbacks+0x1c7/0x790
[  179.239178] hardirqs last disabled at (334397): [<ffffffffa511d872>]
rcu_process_callbacks+0x182/0x790
[  179.249931] softirqs last  enabled at (334386): [<ffffffffa509732e>] irq_enter+0x5e/0x70
[  179.259306] softirqs last disabled at (334387): [<ffffffffa509741c>] irq_exit+0xdc/0xf0
[  179.268584]
[  179.268584] other info that might help us debug this:
[  179.276572]  Possible unsafe locking scenario:
[  179.276572]
[  179.283877]        CPU0
[  179.286954]        ----
[  179.290033]   lock(&(&r->consumer_lock)->rlock);
[  179.295546]   <Interrupt>
[  179.298830]     lock(&(&r->consumer_lock)->rlock);
[  179.304550]
[  179.304550]  *** DEADLOCK ***

Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/page_pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 68bf07206744..43a932cb609b 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -269,7 +269,7 @@ static void __page_pool_empty_ring(struct page_pool *pool)
 	struct page *page;
 
 	/* Empty recycle ring */
-	while ((page = ptr_ring_consume(&pool->ring))) {
+	while ((page = ptr_ring_consume_bh(&pool->ring))) {
 		/* Verify the refcnt invariant of cached pages */
 		if (!(page_ref_count(page) == 1))
 			pr_crit("%s() page_pool refcnt %d violation\n",
-- 
1.8.3.1

[PATCH net] net/xdp: Fix suspicious RCU usage warning

[Tariq Toukan]

Fix the warning below by calling rhashtable_lookup under
RCU read lock.

[  342.450870] WARNING: suspicious RCU usage
[  342.455856] 4.18.0-rc2+ #17 Tainted: G           O
[  342.462210] -----------------------------
[  342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
[  342.476568]
[  342.476568] other info that might help us debug this:
[  342.476568]
[  342.486978]
[  342.486978] rcu_scheduler_active = 2, debug_locks = 1
[  342.495211] 4 locks held by modprobe/3934:
[  342.500265]  #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
mlx5_unregister_interface+0x18/0x90 [mlx5_core]
[  342.511953]  #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
[  342.521109]  #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
[mlx5_core]
[  342.531642]  #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
[  342.541206]
[  342.541206] stack backtrace:
[  342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G           O      4.18.0-rc2+ #17
[  342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
[  342.565606] Call Trace:
[  342.568861]  dump_stack+0x78/0xb3
[  342.573086]  xdp_rxq_info_unreg+0x3f5/0x6b0
[  342.578285]  ? __call_rcu+0x220/0x300
[  342.582911]  mlx5e_free_rq+0x38/0xc0 [mlx5_core]
[  342.588602]  mlx5e_close_channel+0x20/0x120 [mlx5_core]
[  342.594976]  mlx5e_close_channels+0x26/0x40 [mlx5_core]
[  342.601345]  mlx5e_close_locked+0x44/0x50 [mlx5_core]
[  342.607519]  mlx5e_close+0x42/0x60 [mlx5_core]
[  342.613005]  __dev_close_many+0xb1/0x120
[  342.617911]  dev_close_many+0xa2/0x170
[  342.622622]  rollback_registered_many+0x148/0x460
[  342.628401]  ? __lock_acquire+0x48d/0x11b0
[  342.633498]  ? unregister_netdev+0xe/0x20
[  342.638495]  rollback_registered+0x56/0x90
[  342.643588]  unregister_netdevice_queue+0x7e/0x100
[  342.649461]  unregister_netdev+0x18/0x20
[  342.654362]  mlx5e_remove+0x2a/0x50 [mlx5_core]
[  342.659944]  mlx5_remove_device+0xe5/0x110 [mlx5_core]
[  342.666208]  mlx5_unregister_interface+0x39/0x90 [mlx5_core]
[  342.673038]  cleanup+0x5/0xbfc [mlx5_core]
[  342.678094]  __x64_sys_delete_module+0x16b/0x240
[  342.683725]  ? do_syscall_64+0x1c/0x210
[  342.688476]  do_syscall_64+0x5a/0x210
[  342.693025]  entry_SYSCALL_64_after_hwframe+0x49/0xbe

Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
Cc: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/xdp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/xdp.c b/net/core/xdp.c
index 9d1f22072d5d..c20fefbfb76c 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -102,7 +102,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
 
 	mutex_lock(&mem_id_lock);
 
+	rcu_read_lock();
 	xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
+	rcu_read_unlock();
 	if (!xa) {
 		mutex_unlock(&mem_id_lock);
 		return;
-- 
1.8.3.1

Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning

[Alexei Starovoitov]

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.
> 
> [  342.450870] WARNING: suspicious RCU usage
> [  342.455856] 4.18.0-rc2+ #17 Tainted: G           O
> [  342.462210] -----------------------------
> [  342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
> [  342.476568]
> [  342.476568] other info that might help us debug this:
> [  342.476568]
> [  342.486978]
> [  342.486978] rcu_scheduler_active = 2, debug_locks = 1
> [  342.495211] 4 locks held by modprobe/3934:
> [  342.500265]  #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
> [  342.511953]  #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
> [  342.521109]  #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
> [mlx5_core]
> [  342.531642]  #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
> [  342.541206]
> [  342.541206] stack backtrace:
> [  342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G           O      4.18.0-rc2+ #17
> [  342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
> [  342.565606] Call Trace:
> [  342.568861]  dump_stack+0x78/0xb3
> [  342.573086]  xdp_rxq_info_unreg+0x3f5/0x6b0
> [  342.578285]  ? __call_rcu+0x220/0x300
> [  342.582911]  mlx5e_free_rq+0x38/0xc0 [mlx5_core]
> [  342.588602]  mlx5e_close_channel+0x20/0x120 [mlx5_core]
> [  342.594976]  mlx5e_close_channels+0x26/0x40 [mlx5_core]
> [  342.601345]  mlx5e_close_locked+0x44/0x50 [mlx5_core]
> [  342.607519]  mlx5e_close+0x42/0x60 [mlx5_core]
> [  342.613005]  __dev_close_many+0xb1/0x120
> [  342.617911]  dev_close_many+0xa2/0x170
> [  342.622622]  rollback_registered_many+0x148/0x460
> [  342.628401]  ? __lock_acquire+0x48d/0x11b0
> [  342.633498]  ? unregister_netdev+0xe/0x20
> [  342.638495]  rollback_registered+0x56/0x90
> [  342.643588]  unregister_netdevice_queue+0x7e/0x100
> [  342.649461]  unregister_netdev+0x18/0x20
> [  342.654362]  mlx5e_remove+0x2a/0x50 [mlx5_core]
> [  342.659944]  mlx5_remove_device+0xe5/0x110 [mlx5_core]
> [  342.666208]  mlx5_unregister_interface+0x39/0x90 [mlx5_core]
> [  342.673038]  cleanup+0x5/0xbfc [mlx5_core]
> [  342.678094]  __x64_sys_delete_module+0x16b/0x240
> [  342.683725]  ? do_syscall_64+0x1c/0x210
> [  342.688476]  do_syscall_64+0x5a/0x210
> [  342.693025]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>  net/core/xdp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 9d1f22072d5d..c20fefbfb76c 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -102,7 +102,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
>  
>  	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.

Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning

[Daniel Borkmann]

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.
>>
>> [  342.450870] WARNING: suspicious RCU usage
>> [  342.455856] 4.18.0-rc2+ #17 Tainted: G           O
>> [  342.462210] -----------------------------
>> [  342.467202] ./include/linux/rhashtable.h:481 suspicious rcu_dereference_check() usage!
>> [  342.476568]
>> [  342.476568] other info that might help us debug this:
>> [  342.476568]
>> [  342.486978]
>> [  342.486978] rcu_scheduler_active = 2, debug_locks = 1
>> [  342.495211] 4 locks held by modprobe/3934:
>> [  342.500265]  #0: 00000000e23116b2 (mlx5_intf_mutex){+.+.}, at:
>> mlx5_unregister_interface+0x18/0x90 [mlx5_core]
>> [  342.511953]  #1: 00000000ca16db96 (rtnl_mutex){+.+.}, at: unregister_netdev+0xe/0x20
>> [  342.521109]  #2: 00000000a46e2c4b (&priv->state_lock){+.+.}, at: mlx5e_close+0x29/0x60
>> [mlx5_core]
>> [  342.531642]  #3: 0000000060c5bde3 (mem_id_lock){+.+.}, at: xdp_rxq_info_unreg+0x93/0x6b0
>> [  342.541206]
>> [  342.541206] stack backtrace:
>> [  342.547075] CPU: 12 PID: 3934 Comm: modprobe Tainted: G           O      4.18.0-rc2+ #17
>> [  342.556621] Hardware name: Dell Inc. PowerEdge R730/0H21J3, BIOS 1.5.4 10/002/2015
>> [  342.565606] Call Trace:
>> [  342.568861]  dump_stack+0x78/0xb3
>> [  342.573086]  xdp_rxq_info_unreg+0x3f5/0x6b0
>> [  342.578285]  ? __call_rcu+0x220/0x300
>> [  342.582911]  mlx5e_free_rq+0x38/0xc0 [mlx5_core]
>> [  342.588602]  mlx5e_close_channel+0x20/0x120 [mlx5_core]
>> [  342.594976]  mlx5e_close_channels+0x26/0x40 [mlx5_core]
>> [  342.601345]  mlx5e_close_locked+0x44/0x50 [mlx5_core]
>> [  342.607519]  mlx5e_close+0x42/0x60 [mlx5_core]
>> [  342.613005]  __dev_close_many+0xb1/0x120
>> [  342.617911]  dev_close_many+0xa2/0x170
>> [  342.622622]  rollback_registered_many+0x148/0x460
>> [  342.628401]  ? __lock_acquire+0x48d/0x11b0
>> [  342.633498]  ? unregister_netdev+0xe/0x20
>> [  342.638495]  rollback_registered+0x56/0x90
>> [  342.643588]  unregister_netdevice_queue+0x7e/0x100
>> [  342.649461]  unregister_netdev+0x18/0x20
>> [  342.654362]  mlx5e_remove+0x2a/0x50 [mlx5_core]
>> [  342.659944]  mlx5_remove_device+0xe5/0x110 [mlx5_core]
>> [  342.666208]  mlx5_unregister_interface+0x39/0x90 [mlx5_core]
>> [  342.673038]  cleanup+0x5/0xbfc [mlx5_core]
>> [  342.678094]  __x64_sys_delete_module+0x16b/0x240
>> [  342.683725]  ? do_syscall_64+0x1c/0x210
>> [  342.688476]  do_syscall_64+0x5a/0x210
>> [  342.693025]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>>
>> Fixes: 8d5d88527587 ("xdp: rhashtable with allocator ID to pointer mapping")
>> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
>> Cc: Jesper Dangaard Brouer <brouer@redhat.com>
>> ---
>>  net/core/xdp.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 9d1f22072d5d..c20fefbfb76c 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -102,7 +102,9 @@ static void __xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
>>  
>>  	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

Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning

[Tariq Toukan]

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?

Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning

[Alexei Starovoitov]

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.

Re: [PATCH net] net/page_pool: Fix inconsistent lock state warning

[David Miller]

From: Tariq Toukan <tariqt@mellanox.com>
Date: Tue, 17 Jul 2018 18:10:37 +0300

> Fix the warning below by calling the ptr_ring_consume_bh,
> which uses spin_[un]lock_bh.
 ...
> Fixes: ff7d6b27f894 ("page_pool: refurbish version of page_pool code")
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> Cc: Jesper Dangaard Brouer <brouer@redhat.com>

Applied, thanks.

Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning

[Tariq Toukan]

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 ?

Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning

[Daniel Borkmann]

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!