Re: [PATCH net-next 1/2] net: use a single lockdep class for the netdev instance lock

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

On 09/23, Jakub Kicinski wrote:
> netdev_lockdep_set_classes() puts dev->lock in a separate lockdep class,
> by type (netkit vs dummy etc), with the intent of keeping the instance
> locks of individual devices as independent from each other as possible.
> In practice it does the opposite. lockdep only calls the cmp_fn for locks
> of the same class, so netdev_lock_cmp_fn() never gets a say when devices
> from different classes are nested. Instead lockdep records a dependency
> between the classes, and reports a circular locking problem as soon as
> the nesting happens the other way round, e.g. when devices are unregistered
> in a batch.
> 
>   ======================================================
>   WARNING: possible circular locking dependency detected
>   7.3.0-rc3+ #26 Not tainted
>   ------------------------------------------------------
>   kworker/u256:1/326 is trying to acquire lock:
>   ff110000104fce28 (&dev_instance_lock_key#6){+.+.}-{4:4}, at:
>   unregister_netdevice_many_notify+0x1141/0x1c30
> 
>   but task is already holding lock:
>   ff110000127f2e28 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
>   unregister_netdevice_many_notify+0x1141/0x1c30
> 
>   -> #1 (&dev_instance_lock_key#7){+.+.}-{4:4}:
>          __lock_acquire+0x767/0xd60
>          lock_acquire.part.0+0xd0/0x260
>          __mutex_lock+0x17d/0x1f20
>          unregister_netdevice_many_notify+0x1141/0x1c30
>          default_device_exit_batch+0x3ee/0x520
>          ops_undo_list+0x2cc/0x8a0
>          cleanup_net+0x442/0x9c0
>          process_one_work+0x951/0x1ab0
>          worker_thread+0x5a6/0xd10
>          kthread+0x339/0x430
>          ret_from_fork+0x4a4/0x6f0
>          ret_from_fork_asm+0x1a/0x30
> 
>   -> #0 (&dev_instance_lock_key#6){+.+.}-{4:4}:
>          check_prev_add+0xeb/0xe60
>          validate_chain+0x598/0x900
>          __lock_acquire+0x767/0xd60
>          lock_acquire.part.0+0xd0/0x260
>          __mutex_lock+0x17d/0x1f20
>          unregister_netdevice_many_notify+0x1141/0x1c30
>          default_device_exit_batch+0x3ee/0x520
>          ops_undo_list+0x2cc/0x8a0
>          cleanup_net+0x442/0x9c0
>          process_one_work+0x951/0x1ab0
>          worker_thread+0x5a6/0xd10
>          kthread+0x339/0x430
>          ret_from_fork+0x4a4/0x6f0
>          ret_from_fork_asm+0x1a/0x30
> 
>    Possible unsafe locking scenario:
> 
>          CPU0                    CPU1
>          ----                    ----
>     lock(&dev_instance_lock_key#7);
>                                  lock(&dev_instance_lock_key#6);
>                                  lock(&dev_instance_lock_key#7);
>     lock(&dev_instance_lock_key#6);
> 
>    *** DEADLOCK ***
> 
>   locks held by kworker/u256:1/326: 6, last CPU#6:
>    #0: ff11000001c2b540 ((wq_completion)netns){+.+.}-{0:0}, at:
>   process_one_work+0x117c/0x1ab0
>    #1: ffa0000001a3fd18 (net_cleanup_work){+.+.}-{0:0}, at:
>   process_one_work+0x8ce/0x1ab0
>    #2: ffffffff98f53288 (pernet_ops_rwsem){++++}-{4:4}, at:
>   cleanup_net+0xc1/0x9c0
>    #3: ffffffff98f6ede0 (rtnl_mutex){+.+.}-{4:4}, at:
>   default_device_exit_batch+0x92/0x520
>    #4: ff1100001321ae28 (&dev_instance_lock_key#7){+.+.}-{4:4}, at:
>   unregister_netdevice_many_notify+0x1141/0x1c30
> 
> Keep all instance locks in the class from mutex_init() and let
> the cmp_fn run. It allows nesting under rtnl_lock, which covers
> the most common case.
> 
> The cmp function itself also its address gets compared by lockdep
> so it can't be a static inline (that'd give each device type it's
> own cmp_fn).
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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