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

[bot+bpf-ci]

> syzbot reported a circular locking dependency involving &net->xdp.lock,
> &port->pnodes_lock, netdev_lock_ops(), and &xs->mutex:
>
> -> #3 (&net->xdp.lock):
>        xsk_notifier+0x3d/0x2c0 net/xdp/xsk.c:2106
>        ipvlan_device_event+0x310/0x4e0 drivers/net/ipvlan/ipvlan_main.c:834
>        unregister_netdevice_many_notify+0x808/0x18b0 net/core/dev.c:12518
>
> -> #2 (&port->pnodes_lock):
>        ipvlan_device_event+0x85/0x4e0 drivers/net/ipvlan/ipvlan_main.c:795
>        notifier_call_chain+0xb5/0x410 kernel/notifier.c:85

Does the commit message accurately describe the #2 lock acquisition?

The syzbot report shows the #2 acquisition of pnodes_lock comes from the
bonding/features path (NETDEV_FEAT_CHANGE event triggering
netdev_compute_master_upper_features), not from device unregistration.
The #3 stack shows unregister calling into ipvlan_device_event, but
that's a different event path.

> The cycle exists through the following dependency chain:
> 1. xsk_bind() acquired netdev_lock_ops() while holding &xs->mutex (#1).
> 2. Device unregistration in ipvlan_device_event() acquired
>    &port->pnodes_lock (#2) and called xsk_notifier(), which acquired
>    &net->xdp.lock (#3).
> 3. Both xsk_diag_dump() and xsk_notifier() acquire &xs->mutex while
>    holding &net->xdp.lock (#0).

How does this enumeration form a cycle?

The enumerated items describe edges:
  #0 -> #1: xs->mutex -> netdev_lock
  #2 -> #3: pnodes_lock -> xdp.lock
  #3 -> #0: xdp.lock -> xs->mutex

To close the cycle, we need the netdev_lock -> pnodes_lock edge (#1 -> #2),
but this edge is never mentioned in the enumeration. Without that fourth
edge, the description doesn't actually describe a complete cycle. The syzbot
report does contain this edge (via the bonding/features path in frame #2),
but the commit message omits it from the enumeration.

Could the commit message be clarified to explicitly state all four edges
that form the cycle?

[ ... ]

> Reported-by: syzbot+aa48b5fe7bfda62d1682@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=aa48b5fe7bfda62d1682

Is it appropriate to use the Closes tag here?

The syzbot dashboard shows this bug was already fixed by commit a8455260b2e9
("ipvlan: unregister upper devices outside pnodes_lock"), which removed the
pnodes_lock -> net->xdp.lock edge on the ipvlan side. That commit is present
in this tree at drivers/net/ipvlan/ipvlan_main.c:899-902.

Using the Closes tag on a redundant patch may cause syzbot and patchwork to
misattribute the fix to this commit instead of the actual fix.

Additionally, Maciej Fijalkowski informed you in v5 that they had already
fixed the issue on the ipvlan side:

  https://lore.kernel.org/bpf/DM4PR11MB6117332DE5BD8408848E453D82B72@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

  "We went with fixing ipvlan side instead:
   https://lore.kernel.org/netdev/20260828164918.451364-1-maciej.fijalkowski@xxxxxxxxx/

   Forgot to CC you, sorry about that."

Does this XSK patch still serve a purpose given the ipvlan fix has already
resolved the reported issue?

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 33475b180ea6..b7072092cc51 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);

[ ... ]

> @@ -1786,18 +1788,20 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
>  	}
>
>  out_unlock:
> -	if (err) {
> -		dev_put(dev);
> -	} else {
> +	if (!err) {
>  		/* Matches smp_rmb() in bind() for shared umem
>  		 * sockets, and xsk_is_bound().
>  		 */
>  		smp_wmb();
>  		WRITE_ONCE(xs->state, XSK_BOUND);
>  	}
> -	netdev_unlock_ops(dev);
>  out_release:
>  	mutex_unlock(&xs->mutex);
> +	if (dev) {
> +		netdev_unlock_ops(dev);
> +		if (err)
> +			dev_put(dev);
> +	}
>  	rtnl_unlock();
>  	return err;
>  }


---
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/34946645726