Re: [PATCH bpf-next v6] 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 1 potential issue(s) to consider:
- [High] xsk: direct AB-BA lock inversion between netdev_lock_ops and xs->mutex
--

commit fb0725dd07b52433e13678029edcd12b678195df
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 break a 4-lock circular dependency at its root in
xsk_bind(). It reorders the acquisition of netdev_lock_ops() and xs->mutex
with the goal of fully removing the xs->mutex -> netdev_lock_ops edge.

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

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 33475b180ea6f..b7072092cc51f 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1636,20 +1636,22 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
>  		return -EINVAL;
>  
>  	rtnl_lock();
> +
> +	dev = dev_get_by_index(sock_net(sk), sxdp->sxdp_ifindex);
> +	if (dev)
> +		netdev_lock_ops(dev);
> +
>  	mutex_lock(&xs->mutex);

[Severity: High]
Does this introduce a direct AB-BA lock inversion between netdev_lock_ops
and xs->mutex?

By moving the netdev_lock_ops() acquisition before mutex_lock(&xs->mutex)
in xsk_bind(), we establish a netdev_lock_ops -> xs->mutex dependency.

However, there is an existing, unmodified path in xsk_notifier() that takes
xs->mutex and then calls xp_clear_dev():

net/xdp/xsk.c:xsk_notifier() {
    ...
    mutex_lock(&xs->mutex);
    if (xs->dev == dev) {
        ...
        xp_clear_dev(xs->pool);
    }
    mutex_unlock(&xs->mutex);
    ...
}

And xp_clear_dev() unconditionally acquires netdev_lock_ops():

net/xdp/xsk_buff_pool.c:xp_clear_dev() {
    ...
    netdev_lock_ops(netdev);
    xp_disable_drv_zc(pool);
    xsk_clear_pool_at_qid(pool->netdev, pool->queue_id);
    pool->netdev = NULL;
    netdev_unlock_ops(netdev);
}

This retains an xs->mutex -> netdev_lock_ops dependency during device
unregistration.

While both paths hold rtnl_lock() (preventing an actual runtime deadlock),
Lockdep tracks lock dependencies globally by class and will flag this as a
circular locking dependency. On systems with panic_on_warn enabled, this
can be triggered by an unprivileged user with CAP_NET_RAW by binding an
XDP socket and unregistering a network device.

Is there another way to resolve the original 4-lock cycle without inverting
the lock order here?

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