Re: [net PATCH] xdp: obtain the mem_id mutex before trying to remove an entry.

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 3 Dec 2019 14:01:14 -0800
Jonathan Lemon <jonathan.lemon@gmail.com> wrote:

> A lockdep splat was observed when trying to remove an xdp memory
> model from the table since the mutex was obtained when trying to
> remove the entry, but not before the table walk started:
> 
> Fix the splat by obtaining the lock before starting the table walk.
> 
> Fixes: c3f812cea0d7 ("page_pool: do not release pool until inflight == 0.")
> Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>

Have you tested if this patch fix the problem reported by Grygorii?

Link: https://lore.kernel.org/netdev/c2de8927-7bca-612f-cdfd-e9112fee412a@ti.com

Grygorii can you test this?

> ---
>  net/core/xdp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index e334fad0a6b8..7c8390ad4dc6 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -80,12 +80,8 @@ static void mem_xa_remove(struct xdp_mem_allocator *xa)
>  {
>  	trace_mem_disconnect(xa);
>  
> -	mutex_lock(&mem_id_lock);
> -
>  	if (!rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params))
>  		call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> -
> -	mutex_unlock(&mem_id_lock);
>  }
>  
>  static void mem_allocator_disconnect(void *allocator)
> @@ -93,6 +89,8 @@ static void mem_allocator_disconnect(void *allocator)
>  	struct xdp_mem_allocator *xa;
>  	struct rhashtable_iter iter;
>  
> +	mutex_lock(&mem_id_lock);
> +
>  	rhashtable_walk_enter(mem_id_ht, &iter);
>  	do {
>  		rhashtable_walk_start(&iter);
> @@ -106,6 +104,8 @@ static void mem_allocator_disconnect(void *allocator)
>  
>  	} while (xa == ERR_PTR(-EAGAIN));
>  	rhashtable_walk_exit(&iter);
> +
> +	mutex_unlock(&mem_id_lock);
>  }
>  
>  static void mem_id_disconnect(int id)

Moving the mutex-lock might be a good idea, but I'm not sure that fixes
the issue.  I'm more suspect about the usage of rcu_read_lock() in
xdp_rxq_info_unreg_mem_model(), listed below.

void xdp_rxq_info_unreg_mem_model(struct xdp_rxq_info *xdp_rxq)
{
	struct xdp_mem_allocator *xa;
	int id = xdp_rxq->mem.id;

	if (xdp_rxq->reg_state != REG_STATE_REGISTERED) {
		WARN(1, "Missing register, driver bug");
		return;
	}

	if (id == 0)
		return;

	if (xdp_rxq->mem.type == MEM_TYPE_ZERO_COPY)
		return mem_id_disconnect(id);

	if (xdp_rxq->mem.type == MEM_TYPE_PAGE_POOL) {
		rcu_read_lock();
		xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
		page_pool_destroy(xa->page_pool);
		rcu_read_unlock();
	}
}
EXPORT_SYMBOL_GPL(xdp_rxq_info_unreg_mem_model);


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer