Re: [PATCH net-next] net: only give queue leasing devices a separate instance lock class

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 09/29, Jakub Kicinski wrote:
> Commit b6f74dff6d26 ("net: use two lockdep classes for the netdev
> instance lock") put every device without a parent in the virtual class.
> It also restricted the locking order for the virtual class, because
> queue leasing has hard requirements on the exact order.
> 
> This bites us back on bond, which is "virtual" and needs to be taken
> before taking the locks of the lowers. NIPA hit the following on the
> new test I recently posted for XDP+bond:
> 
>   WARNING: possible circular locking dependency detected
>   ------------------------------------------------------
>   python3/18635 is trying to acquire lock:
>   ff11000120b8ce30 (&dev->lock){+.+.}-{4:4}, at:
>   netdev_put_lock+0x2d/0x1a0
> 
>   but task is already holding lock:
>   ff110001ef77ae30 (&netdev_virt_instance_lock_key){+.+.}-{4:4}, at:
>   netdev_put_lock+0x2d/0x1a0
> 
>   which lock already depends on the new lock.
> 
>   the existing dependency chain (in reverse order) is:
> 
>   -> #1 (&netdev_virt_instance_lock_key){+.+.}-{4:4}:
>          __mutex_lock+0x1ae/0x1f10
>          xdp_set_features_flag+0x2b/0x50
>          bond_xdp_set_features+0x1eb/0x360
>          bond_netdev_event+0x13f/0x300
>          notifier_call_chain+0xae/0x300
>          call_netdevice_notifiers+0x70/0xa0
>          bnxt_xdp_set+0x2f6/0x620
>          netif_xdp_propagate+0x503/0xc60
>          dev_xdp_propagate+0xa1/0x230
>          bond_xdp_set+0x234/0x700
>          dev_xdp_install+0x592/0xd70
>          dev_xdp_attach+0x355/0xf50
>          dev_change_xdp_fd+0x176/0x210
>          do_setlink.isra.0+0x220d/0x2b20
>          rtnl_newlink+0x9f1/0x11b0
> 
>   -> #0 (&dev->lock){+.+.}-{4:4}:
>          __mutex_lock+0x1ae/0x1f10
>          netdev_put_lock+0x2d/0x1a0
>          netdev_nl_queue_create_doit+0x801/0x1a70
>          genl_family_rcv_msg_doit+0x206/0x300
> 
>    Possible unsafe locking scenario:
> 
>          CPU0                    CPU1
>          ----                    ----
>     lock(&netdev_virt_instance_lock_key);
>                                  lock(&dev->lock);
>                                  lock(&netdev_virt_instance_lock_key);
>     lock(&dev->lock);
> 
> Let's narrow down the "virtual" class to only the devices which
> can actually create a queue. More LoC and complexity, but that
> is what we actually care about here. The rest needs to nest
> under rtnl_lock, which bond does (famous last words?)
> 
> Take the instance locks in two passes, first the netkits then
> the rest (matching the queue leasing order).
> An alternative would be to make sure the close list is sorted
> correctly from the start (queue head/tail appropriately in
> unregister_netdevice_queue()). I think it works but feels
> a little more fragile. Happy to change, tho.
> 
> netdev_can_create_queue() will now be used on paths where we
> genuinely handle non-netkit, so we can't always set the extack.
> Unfortunately, the (recently) added tracepoint in extack fires
> even when extack is NULL.
> 
> Fixes: b6f74dff6d26 ("net: use two lockdep classes for the netdev instance lock")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: daniel@iogearbox.net
> CC: hawk@kernel.org
> CC: john.fastabend@gmail.com
> CC: sdf@fomichev.me
> CC: razor@blackwall.org
> CC: dw@davidwei.uk
> ---
>  net/core/dev.c           | 48 ++++++++++++++++++++++------------------
>  net/core/netdev_queues.c | 31 ++++++++++++++------------
>  2 files changed, 44 insertions(+), 35 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a8eb382f40ca..f225906f7b6f 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -575,7 +575,7 @@ static int netdev_lock_cmp_fn(const struct lockdep_map *a,
>  	if (a == b)
>  		return 0;
>  
> -	/* @a and @b must be of same class - both virtual or physical.
> +	/* @a and @b are of same lock class.
>  	 * cmp_fn won't be called for devices of different classes.
>  	 *
>  	 * For the same class only allow nesting under the protection
> @@ -589,12 +589,13 @@ static int netdev_lock_cmp_fn(const struct lockdep_map *a,
>   * queues from, see netdev_nl_queue_create_doit(). Keep the two kinds
>   * in separate classes so the dependency graph enforces the order;
>   * netdev_lock_cmp_fn() then only has to rule on same-class nesting.
> + * Other virtual devices stay in the default class.
>   */
>  void netdev_set_instance_lock_class(struct net_device *dev)
>  {
>  	static struct lock_class_key netdev_virt_instance_lock_key;
>  
> -	if (dev->dev.parent)
> +	if (!netdev_can_create_queue(dev, NULL))
>  		return;
>  
>  	lockdep_set_class(&dev->lock, &netdev_virt_instance_lock_key);
> @@ -12465,19 +12466,31 @@ static void netif_close_many_and_unlock(struct list_head *close_head)
>  	}
>  }
>  
> -static void netif_close_many_and_unlock_cond(struct list_head *close_head)
> +/* Handle one class of ops-locked devices. Since close requires the lock
> + * we need to be careful about which classes we allow to nest.
> + */
> +static void netdev_lock_ops_close_many(struct list_head *head,
> +				       struct list_head *close_head,
> +				       bool leasing)
>  {
> -#ifdef CONFIG_LOCKDEP
> -	/* We can only track up to MAX_LOCK_DEPTH locks per task.
> -	 *
> -	 * Reserve half the available slots for additional locks possibly
> -	 * taken by notifiers and (soft)irqs.
> -	 */
> -	unsigned int limit = MAX_LOCK_DEPTH / 2;
> +	struct net_device *dev;
>  
> -	if (lockdep_depth(current) > limit)
> -		netif_close_many_and_unlock(close_head);
> +	list_for_each_entry(dev, head, unreg_list) {
> +		if (!(dev->flags & IFF_UP) || !netdev_need_ops_lock(dev) ||
> +		    netdev_can_create_queue(dev, NULL) != leasing)
> +			continue;
> +		list_add_tail(&dev->close_list, close_head);
> +		netdev_lock(dev);
> +
> +#ifdef CONFIG_LOCKDEP
> +		/* We can only track up to MAX_LOCK_DEPTH locks per task.
> +		 * Reserve half the available slots for additional locks
> +		 * possibly taken by notifiers and (soft)irqs.
> +		 */
> +		if (lockdep_depth(current) > MAX_LOCK_DEPTH / 2)
> +			netif_close_many_and_unlock(close_head);
>  #endif
> +	}
>  }
>  
>  bool unregister_netdevice_queued(const struct net_device *dev)
> @@ -12517,15 +12530,8 @@ void unregister_netdevice_many_notify(struct list_head *head,
>  	}
>  
>  	/* If device is running, close it first. Start with ops locked... */
> -	list_for_each_entry(dev, head, unreg_list) {
> -		if (!(dev->flags & IFF_UP))
> -			continue;
> -		if (netdev_need_ops_lock(dev)) {
> -			list_add_tail(&dev->close_list, &close_head);
> -			netdev_lock(dev);
> -		}
> -		netif_close_many_and_unlock_cond(&close_head);
> -	}

Acked-by: Stanislav Fomichev <sdf@fomichev.me>

Don't know if it's gonna help anyone, but if you happen to respin,
maybe add a comment here along the lines of
/* see netdev_set_instance_lock_class kdoc on why queue leasing first */

Although the hole is so deep now, not sure it's worth it :-D

> +	netdev_lock_ops_close_many(head, &close_head, true); /* queue leasing */
> +	netdev_lock_ops_close_many(head, &close_head, false); /* the rest */