Re: [PATCH bpf-next] xsk: Fix circular locking dependency in xsk_bind

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

> diff --git net/xdp/xsk.c net/xdp/xsk.c
> index 7855ee09c4b6..d2fbbeb7b6d7 100644
> --- net/xdp/xsk.c
> +++ net/xdp/xsk.c
> @@ -1612,19 +1612,18 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
>  		return -EINVAL;
>
>  	rtnl_lock();
> -	mutex_lock(&xs->mutex);
> -	if (xs->state != XSK_READY) {
> -		err = -EBUSY;
> -		goto out_release;
> -	}
> -
>  	dev = dev_get_by_index(sock_net(sk), sxdp->sxdp_ifindex);
>  	if (!dev) {
>  		err = -ENODEV;
> -		goto out_release;
> +		goto out_rtnl_unlock;
>  	}
>
>  	netdev_lock_ops(dev);
> +	mutex_lock(&xs->mutex);
> +	if (xs->state != XSK_READY) {
> +		err = -EBUSY;
> +		goto out_unlock;
> +	}

Does this reordering change the errno precedence userspace sees?

Before the patch, bind() on an already-bound socket (state != XSK_READY)
returned -EBUSY regardless of whether the ifindex was valid. After the
patch, the same call returns -ENODEV if the ifindex lookup fails, because
dev_get_by_index() is now checked before the state test.

If preserving the -EBUSY precedence matters for userspace compatibility,
would the state check need to stay ahead of dev_get_by_index()?

[ ... ]

> @@ -1771,9 +1770,9 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
>  		smp_wmb();
>  		WRITE_ONCE(xs->state, XSK_BOUND);
>  	}
> -	netdev_unlock_ops(dev);
> -out_release:
>  	mutex_unlock(&xs->mutex);
> +	netdev_unlock_ops(dev);
> +out_rtnl_unlock:
>  	rtnl_unlock();
>  	return err;
>  }

Looking at the commit message:

>     Fixes: 978939c08db1 ("xsk: use netdev_lock_ops in xsk_bind")

The Fixes tag references commit 978939c08db1, but this commit does not exist
in this repository. Running git log and searching lore shows no commit with
this SHA or subject line. The tag appears to be fabricated.

The buggy locking pattern was actually introduced in commit 975b11ae9077
when net/xdp/xsk.c was first added to this repository. Should the Fixes
tag reference that commit instead?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32867683423