Re: [PATCH bpf-next v3] xsk: Fix circular locking dependency between &net->xdp.lock, &xs->mutex, and netdev_lock_ops

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Aug 26, 2026 at 11:00:19PM +0530, Khawar Ahemad wrote:
> syzbot reported a circular locking dependency involving &net->xdp.lock,
> &xs->mutex, and netdev_lock_ops():
> 
> -> #2 (&net->xdp.lock):
>        xsk_diag_dump
>        netlink_dump
> 
> -> #1 (&xs->mutex):
>        xsk_bind
> 
> -> #0 (netdev_lock_ops):
>        xsk_notifier
> 
> The lockdep dependency cycle arose from the following relationships:
> - xsk_diag_dump() established &net->xdp.lock -> &xs->mutex by calling
>   xsk_diag_fill() under &net->xdp.lock.
> - xsk_bind() established &xs->mutex -> netdev_lock_ops().
> - Device unregistration and xsk_notifier() established
>   netdev_lock_ops() -> &net->xdp.lock while also invoking xp_clear_dev()
>   under &net->xdp.lock and &xs->mutex.
> 
> Eliminate the cycle by decoupling the locks across both paths:
> 
> 1. In xsk_notifier(), split the unregistration into two phases:
>    - First, unbind all matching sockets under &net->xdp.lock and
>      &xs->mutex.
>    - Then, release &net->xdp.lock and perform device queue teardown by
>      sweeping the device queues via xsk_get_pool_from_qid() and calling
>      xp_clear_dev(pool) outside all AF_XDP locks.
> 2. In xsk_diag_dump(), avoid holding &net->xdp.lock while calling
>    xsk_diag_fill(). Instead, locate the target socket under &net->xdp.lock,
>    take a temporary socket reference via sock_hold(), release
>    &net->xdp.lock, and call xsk_diag_fill() (which acquires &xs->mutex)
>    with sock_put().
>    Distinguish -ENOENT (when an unbound socket is skipped) from -EMSGSIZE
>    (when the skb is full and the cursor must be retained for dump
>    continuation).
> 
> Fixes: 975b11ae9077 ("xsk: add socket allocate, create and bind")
> Reported-by: syzbot+aa48b5fe7bfda62d1682@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=aa48b5fe7bfda62d1682
> Signed-off-by: Khawar Ahemad <ahemadkhawar123@gmail.com>
> ---
> v2 -> v3:
> - Fix direct AB-BA lock inversion in xsk_notifier() by performing device
>   queue sweeps via xsk_get_pool_from_qid() outside &net->xdp.lock.
> - Eliminate &net->xdp.lock -> &xs->mutex in xsk_diag_dump() by taking a
>   temporary socket reference under &net->xdp.lock and releasing the lock
>   prior to xsk_diag_fill().
> - Distinguish -ENOENT (skipped unbound socket) from -EMSGSIZE (buffer
>   exhaustion) to preserve dump continuation without infinite loops.
> - Link to v2: https://lore.kernel.org/bpf/20260826162110.99879-1-ahemadkhawar123@gmail.com/
> 
> v1 -> v2:
> - Avoid reordering locks in xsk_bind() to preserve errno precedence.
> - Link to v1: https://lore.kernel.org/bpf/20260825152152.86092-1-ahemadkhawar123@gmail.com/
> 
>  net/xdp/xsk.c      | 17 +++++++-----
>  net/xdp/xsk_diag.c | 65 +++++++++++++++++++++++++++++++---------------
>  2 files changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index c2f47182dc..e72344fccb 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -2099,14 +2099,15 @@ static int xsk_notifier(struct notifier_block *this,
>  {
>  	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
>  	struct net *net = dev_net(dev);
> +	unsigned int max_queues;
>  	struct sock *sk;
> +	u16 qid;
>  
>  	switch (msg) {
>  	case NETDEV_UNREGISTER:
>  		mutex_lock(&net->xdp.lock);
>  		sk_for_each(sk, &net->xdp.list) {
>  			struct xdp_sock *xs = xdp_sk(sk);
> -			struct xsk_buff_pool *pool = NULL;

I'm sorry but I have to ask you to stop this spam. This is clearly a v3
based on your v2 which is not a correct way to post a fix.

Besides there is a mandatory 24 hour period between posting next
revisions.

>  
>  			mutex_lock(&xs->mutex);
>  			if (xs->dev == dev) {
> @@ -2114,18 +2115,22 @@ static int xsk_notifier(struct notifier_block *this,
>  				if (!sock_flag(sk, SOCK_DEAD))
>  					sk_error_report(sk);
>  
> -				pool = xs->pool;
>  				xsk_unbind_dev(xs);
>  			}
>  			mutex_unlock(&xs->mutex);
> +		}
> +		mutex_unlock(&net->xdp.lock);
> +
> +		/* Clear device references outside AF_XDP locks to avoid
> +		 * lock inversion with netdev_lock_ops().
> +		 */
> +		max_queues = max(dev->real_num_rx_queues, dev->real_num_tx_queues);
> +		for (qid = 0; qid < max_queues; qid++) {
> +			struct xsk_buff_pool *pool = xsk_get_pool_from_qid(dev, qid);
>  
> -			/* Clear device references outside xs->mutex to avoid
> -			 * lock inversion with netdev_lock_ops().
> -			 */
>  			if (pool)
>  				xp_clear_dev(pool);
>  		}
> -		mutex_unlock(&net->xdp.lock);
>  		break;
>  	}
>  	return NOTIFY_DONE;
> diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> index 0170363eb5..bad0b13064 100644
> --- a/net/xdp/xsk_diag.c
> +++ b/net/xdp/xsk_diag.c
> @@ -97,6 +97,7 @@ static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
>  	struct xdp_sock *xs = xdp_sk(sk);
>  	struct xdp_diag_msg *msg;
>  	struct nlmsghdr *nlh;
> +	int err = -EMSGSIZE;
>  
>  	nlh = nlmsg_put(nlskb, portid, seq, SOCK_DIAG_BY_FAMILY, sizeof(*msg),
>  			flags);
> @@ -111,8 +112,10 @@ static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
>  	sock_diag_save_cookie(sk, msg->xdiag_cookie);
>  
>  	mutex_lock(&xs->mutex);
> -	if (READ_ONCE(xs->state) == XSK_UNBOUND)
> +	if (READ_ONCE(xs->state) == XSK_UNBOUND) {
> +		err = -ENOENT;
>  		goto out_nlmsg_trim;
> +	}
>  
>  	if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
>  		goto out_nlmsg_trim;
> @@ -145,7 +148,7 @@ static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
>  out_nlmsg_trim:
>  	mutex_unlock(&xs->mutex);
>  	nlmsg_cancel(nlskb, nlh);
> -	return -EMSGSIZE;
> +	return err;
>  }
>  
>  static int xsk_diag_dump(struct sk_buff *nlskb, struct netlink_callback *cb)
> @@ -153,28 +156,48 @@ static int xsk_diag_dump(struct sk_buff *nlskb, struct netlink_callback *cb)
>  	struct xdp_diag_req *req = nlmsg_data(cb->nlh);
>  	struct net *net = sock_net(nlskb->sk);
>  	int num = 0, s_num = cb->args[0];
> -	struct sock *sk;
> -
> -	mutex_lock(&net->xdp.lock);
> -
> -	sk_for_each(sk, &net->xdp.list) {
> -		if (!net_eq(sock_net(sk), net))
> -			continue;
> -		if (num++ < s_num)
> -			continue;
> -
> -		if (xsk_diag_fill(sk, nlskb, req,
> -				  sk_user_ns(NETLINK_CB(cb->skb).sk),
> -				  NETLINK_CB(cb->skb).portid,
> -				  cb->nlh->nlmsg_seq, NLM_F_MULTI,
> -				  sock_i_ino(sk)) < 0) {
> -			num--;
> -			break;
> +	struct sock *sk, *target_sk;
> +	int err;
> +
> +	for (;;) {
> +		target_sk = NULL;
> +		num = 0;
> +
> +		mutex_lock(&net->xdp.lock);
> +		sk_for_each(sk, &net->xdp.list) {
> +			if (!net_eq(sock_net(sk), net))
> +				continue;
> +			if (num++ == s_num) {
> +				sock_hold(sk);
> +				target_sk = sk;
> +				break;
> +			}
>  		}
> +		mutex_unlock(&net->xdp.lock);
> +
> +		if (!target_sk)
> +			break;
> +
> +		err = xsk_diag_fill(target_sk, nlskb, req,
> +				    sk_user_ns(NETLINK_CB(cb->skb).sk),
> +				    NETLINK_CB(cb->skb).portid,
> +				    cb->nlh->nlmsg_seq, NLM_F_MULTI,
> +				    sock_i_ino(target_sk));
> +		sock_put(target_sk);
> +
> +		/*
> +		 * xsk_diag_fill() returns:
> +		 *   0:         entry added successfully.
> +		 *   -ENOENT:   socket is unbound, skip it.
> +		 *   -EMSGSIZE: skb is full, retry this socket on the next dump callback.
> +		 */
> +		if (err == -EMSGSIZE)
> +			break;
> +
> +		s_num++;
>  	}
>  
> -	mutex_unlock(&net->xdp.lock);
> -	cb->args[0] = num;
> +	cb->args[0] = s_num;
>  	return nlskb->len;
>  }
>  
> -- 
> 2.54.0 (Apple Git-157)
>