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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Queue-based iteration in xsk_notifier() using real_num_rx/tx_queues misses stranded pools, leading to net_device refcount leaks and Use-After-Free.
- [High] O(N^2) list traversal in xsk_diag_dump() causes CPU soft lockups, exacerbated by skipping unbound sockets.
- [Medium] Netlink DUMP rule violation: missing NLM_F_DUMP_INTR flag when dropping locks.

Pre-existing issues:
- [High] The circular locking dependency is not fully resolved because `xsk_notifier()` still acquires `&xs->mutex` while holding `&net->xdp.lock`.
--

commit 5d27bb72970d01c1b3fb96dd5f15dd2e0ec89bb0
Author: Khawar Ahemad <ahemadkhawar123@gmail.com>

xsk: Fix circular locking dependency between &net->xdp.lock, &xs->mutex, and netdev_lock_ops

This commit attempts to fix a circular locking dependency involving
net->xdp.lock, port->pnodes_lock, netdev_lock_ops, and xs->mutex. It modifies
xsk_diag_dump() to release net->xdp.lock temporarily and defers xp_clear_dev()
outside of the locks in xsk_notifier().

Closes: https://syzkaller.appspot.com/bug?extid=aa48b5fe7bfda62d1682

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 7855ee09c4b64..e72344fccbf26 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -2114,13 +2116,21 @@ static int xsk_notifier(struct notifier_block *this,
>  					sk_error_report(sk);
>  
>  				xsk_unbind_dev(xs);

[Severity: High]
This is a pre-existing issue, but does this code still preserve the circular
locking dependency?

The patch intended to fix a 4-lock circular dependency involving
port->pnodes_lock -> net->xdp.lock -> xs->mutex -> netdev_lock_ops.
However, xsk_notifier() still acquires &xs->mutex while holding
&net->xdp.lock (just prior to this xsk_unbind_dev() call).

Since xsk_bind() still fundamentally acquires netdev_lock_ops while holding
&xs->mutex, the exact lock dependency chain is preserved and lockdep will
continue to report the cycle.

>  			}
>  			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);
> +
> +			if (pool)
> +				xp_clear_dev(pool);
> +		}

[Severity: High]
Can this loop miss pools that were bound to queues which have since been
disabled?

If an unprivileged user binds an AF_XDP socket to a high queue index
(e.g., qid=3), and the interface's active queues are then reduced via
ethtool (e.g., rx 2), max_queues will be 2. When the device is unregistered,
this loop will only iterate up to max_queues and miss the pool on qid 3
because xsk_get_pool_from_qid() strictly enforces current queue counts.

Missing stranded pools during device unregistration fails to clear
pool->netdev, leaking the net_device reference. This causes an infinite
loop in netdev_wait_allrefs() upon device removal, freezing the network
stack. A subsequent socket close will trigger a Use-After-Free when
xp_clear_dev() dereferences the freed net_device.

>  		break;
>  	}
>  	return NOTIFY_DONE;

[ ... ]

> diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> index 0170363eb542c..bad0b13064f8b 100644
> --- a/net/xdp/xsk_diag.c
> +++ b/net/xdp/xsk_diag.c
> @@ -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;
> +			}
>  		}

[Severity: High]
Does this introduce an O(N^2) list traversal that could cause CPU soft
lockups?

By dropping locks and restarting the sk_for_each traversal from the list
head up to s_num on every iteration, this loop introduces an O(N^2) time
complexity.

Crucially, the loop never yields (e.g., via cond_resched()), and skipped
unbound sockets (-ENOENT) cause the loop to increment s_num and continue
without returning to userspace. If a user creates tens of thousands of
unbound AF_XDP sockets and triggers an xsk_diag_dump, the kernel will
execute billions of loop iterations within a single recvmsg syscall,
leading to severe lock contention on net->xdp.lock and CPU starvation.

> +		mutex_unlock(&net->xdp.lock);
> +
> +		if (!target_sk)
> +			break;

[Severity: Medium]
When dropping locks during a netlink dump iteration, should this set the
NLM_F_DUMP_INTR flag on the SKB?

Iterating over a list using an index while dropping locks allows concurrent
modifications to the list, which can cause the dump iteration to silently
skip elements or dump the same element multiple times.

The Netlink API contract mandates setting the NLM_F_DUMP_INTR flag when this
occurs so userspace is warned of the inconsistency.

> +
> +		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++;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826174744.3394-1-ahemadkhawar123@gmail.com?part=1