Re: [PATCH net-next v2 1/2] net: use two lockdep classes for the netdev instance lock

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

On 09/25, 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
> 
> We can't put all devices in the same class, that'd be too permissive.
> netdev_nl_queue_create_doit() and netdev_nl_bind_tx_doit() lock
> a virtual device (netkit) before a physical device, without rtnl_lock.
> We can never allow locking in the opposite order even under rtnl_lock.
> cmp_fn cannot enforce this sort of rule: lockdep keys its chain cache on
> the sequence of lock classes, so with a single class both orders hash
> to the same chain and only whichever happens first is validated.
> 
> netdev_lock_cmp_fn() itself has another source of false-negatives.
> lockdep calls it from within __lock_acquire(), with the recursion counter
> already raised, so lock_is_held_type() always returns LOCK_STATE_UNKNOWN,
> which means lockdep_rtnl_is_held() always returns true / held.
> Use rtnl_is_locked(), which looks at the mutex directly and does
> work from that context. We may still miss a bad case if some other
> process is holding the lock, not us, but that's better than the
> 100% false negative rate of lockdep_rtnl_is_held().
> 
> One last thing, lockdep compares the address of the cmp function,
> so it can't be a static inline - that would work similarly to having
> separate classes, again. Move it to a source file.
> 
> Tested by nesting two instance locks from a module, one scenario per boot
> since the first splat turns debug_locks off:
> 
>   first       second      rtnl      reported
>   -----------------------------------------------------------
>   virt->virt              no        recursive locking
>   virt->virt              yes       -
>   virt->phys              no        -
>   virt->phys              yes       -
>   phys->virt              no        - (records the edge)
>   phys->virt              yes       - (records the edge)
>   phys->phys              no        recursive locking
>   phys->phys              yes       -
>   virt->phys  phys->virt  no, no    circular dependency
>   virt->phys  phys->virt  no, yes   circular dependency
>   virt->phys  phys->virt  yes, no   circular dependency
>   virt->phys  phys->virt  yes, yes  circular dependency
>   phys->virt  virt->phys  no, no    circular dependency
>   phys->virt  virt->phys  yes, yes  circular dependency
>   virt->virt  virt->virt  yes, no   recursive locking
>   phys->phys  phys->phys  yes, no   recursive locking
> 
> (netkit and dummy stood in for the virtual devices, virtio_net and
> netdevsim for the physical ones.)
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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