Possible deadlock related to CPU hotplug and kernfs

Hi Rafael and Tejun,
When running CPU hotplug tests, it triggers an lockdep warning
as follow. The two possible deadlock paths are:
1) echo x > /sys/devices/system/cpu/cpux/online ->kernfs_fop_write()

     ->kernfs_get_active()
1.a)   ->rwsem_acquire_read(&kn->dep_map, 0, 1, _RET_IP_);
         ->cpu_up()
1.b)       ->cpu_hotplug_begin()[lock_map_acquire(&cpu_hotplug.dep_map)]
2) hardware triggers hotplug evetns
   ->acpi_device_hotplug()
     ->acpi_processor_remove()
2.a)   ->cpu_hotplug_begin()[lock_map_acquire(&cpu_hotplug.dep_map)]
         ->unregister_cpu()
           ->device_del()
             ->kernfs_remove_by_name_ns()
               ->__kernfs_remove()
                 ->kernfs_drain()

2.b) ->rwsem_acquire(&kn->dep_map, 0, 0, _RET_IP_)

So there is a possible deadlock scenario among 1.a, 1.b, 2.a and 2.b. I'm not familiar with kernfs, so could you please help to comment:
1) whether is a real deadlock issue?

2) any recommended way to get it fixed?

Quoted 20 lines · Jiang Liu

Hi Rafael and Tejun,
When running CPU hotplug tests, it triggers an lockdep warning
as follow. The two possible deadlock paths are:
1) echo x > /sys/devices/system/cpu/cpux/online ->kernfs_fop_write()

     ->kernfs_get_active()
1.a)   ->rwsem_acquire_read(&kn->dep_map, 0, 1, _RET_IP_);
         ->cpu_up()
1.b)       ->cpu_hotplug_begin()[lock_map_acquire(&cpu_hotplug.dep_map)]
2) hardware triggers hotplug evetns
   ->acpi_device_hotplug()
     ->acpi_processor_remove()
2.a)   ->cpu_hotplug_begin()[lock_map_acquire(&cpu_hotplug.dep_map)]
         ->unregister_cpu()
           ->device_del()
             ->kernfs_remove_by_name_ns()
               ->__kernfs_remove()
                 ->kernfs_drain()

2.b) ->rwsem_acquire(&kn->dep_map, 0, 0, _RET_IP_)

Quoted 3 lines

So there is a possible deadlock scenario among 1.a, 1.b, 2.a and 2.b. I'm not familiar with kernfs, so could you please help to comment:
1) whether is a real deadlock issue?

Yes, it seems to be. It's highly unlikely but still possible.

Quoted 1 line

2) any recommended way to get it fixed?

This usually happens with "delete" files and it's worked around by performing special self-removal on the file before actually removing the device. I suppose on/offline files would need to turn off active_protection with kernfs_[un]break_active_protection() which should probably grow sysfs and device layer wrappers.

So acpi_device_hotplug() calls lock_device_hotplug() which simply acquires device_hotplug_lock. It is held throughout the entire hot-add/hot-remove code path.

Witing anything to /sys/devices/system/cpu/cpux/online goes through online_store() in drivers/base/core.c and that does lock_device_hotplug_sysfs() which then attempts to acquire device_hotplug_lock using mutex_trylock(). And it only calls either device_online() or device_offline() if it ends up with the lock held.

Quite frankly, I don't see how these particular two code paths can deadlock in any way.

So either a third code path is involved which is not executed under device_hotplug_lock, or lockdep needs to be told to actually take device_hotplug_lock into account in this case IMO.

Hello, Rafael.

Quoted 11 lines · Rafael J. Wysocki

So acpi_device_hotplug() calls lock_device_hotplug() which simply acquires device_hotplug_lock. It is held throughout the entire hot-add/hot-remove code path.

Witing anything to /sys/devices/system/cpu/cpux/online goes through online_store() in drivers/base/core.c and that does lock_device_hotplug_sysfs() which then attempts to acquire device_hotplug_lock using mutex_trylock(). And it only calls either device_online() or device_offline() if it ends up with the lock held.

Quoted 6 lines

Quite frankly, I don't see how these particular two code paths can deadlock in any way.

So either a third code path is involved which is not executed under device_hotplug_lock, or lockdep needs to be told to actually take device_hotplug_lock into account in this case IMO.

Hmm... all sysfs rw functions are protected from removal. ie. by default, removal of a sysfs file drains in-flight rw operations, so the hot plug path grabs a lock and then tries to remove a file and writing to the online file makes the file's write method to try to grab the same lock. It deadlocks if the hotunplug path already has the lock and trying to drain the online file for removal.

Hi Tejun,

Quoted 21 lines · Tejun Heo

Hello, Rafael.

Quoted 11 lines · Rafael J. Wysocki

So acpi_device_hotplug() calls lock_device_hotplug() which simply acquires device_hotplug_lock. It is held throughout the entire hot-add/hot-remove code path.

Witing anything to /sys/devices/system/cpu/cpux/online goes through online_store() in drivers/base/core.c and that does lock_device_hotplug_sysfs() which then attempts to acquire device_hotplug_lock using mutex_trylock(). And it only calls either device_online() or device_offline() if it ends up with the lock held.

Quoted 6 lines

Quite frankly, I don't see how these particular two code paths can deadlock in any way.

So either a third code path is involved which is not executed under device_hotplug_lock, or lockdep needs to be told to actually take device_hotplug_lock into account in this case IMO.

Quoted 6 lines

Hmm... all sysfs rw functions are protected from removal. ie. by default, removal of a sysfs file drains in-flight rw operations, so the hot plug path grabs a lock and then tries to remove a file and writing to the online file makes the file's write method to try to grab the same lock. It deadlocks if the hotunplug path already has the lock and trying to drain the online file for removal.

My point is that you cannot get into that situation. If hotplug already holds device_hotplug_lock, the write to "online" will end up doing restart_syscall().

If the "online" code path is holding the lock, hotplug cannot acquire it and cannot proceed.

Am I missing anything?

Thanks,
Rafael

Quoted 31 lines · Rafael J. Wysocki

Hi Tejun,

Quoted 21 lines · Tejun Heo

Hello, Rafael.

Quoted 11 lines · Rafael J. Wysocki

So acpi_device_hotplug() calls lock_device_hotplug() which simply acquires device_hotplug_lock. It is held throughout the entire hot-add/hot-remove code path.

Witing anything to /sys/devices/system/cpu/cpux/online goes through online_store() in drivers/base/core.c and that does lock_device_hotplug_sysfs() which then attempts to acquire device_hotplug_lock using mutex_trylock(). And it only calls either device_online() or device_offline() if it ends up with the lock held.

Quoted 6 lines

Quite frankly, I don't see how these particular two code paths can deadlock in any way.

So either a third code path is involved which is not executed under device_hotplug_lock, or lockdep needs to be told to actually take device_hotplug_lock into account in this case IMO.

Quoted 6 lines

Hmm... all sysfs rw functions are protected from removal. ie. by default, removal of a sysfs file drains in-flight rw operations, so the hot plug path grabs a lock and then tries to remove a file and writing to the online file makes the file's write method to try to grab the same lock. It deadlocks if the hotunplug path already has the lock and trying to drain the online file for removal.

Quoted 8 lines

My point is that you cannot get into that situation. If hotplug already holds device_hotplug_lock, the write to "online" will end up doing restart_syscall().

If the "online" code path is holding the lock, hotplug cannot acquire it and cannot proceed.

Am I missing anything?

Hi Rafael,
I think your are right. The lock_device_hotplug_sysfs() has
already provided a solution for such a deadlock scenario. And there's another related code path at boot as:
smp_init()

        ->cpu_up()
                ->cpu_hotplug_begin()
        So it seems to be a false alarm. Any way to teach lockdep

about this to get rid of the false alarm?

Hi,

Quoted 45 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Hi Tejun,

Quoted 21 lines · Tejun Heo

Hello, Rafael.

Quoted 11 lines · Rafael J. Wysocki

So acpi_device_hotplug() calls lock_device_hotplug() which simply acquires device_hotplug_lock. It is held throughout the entire hot-add/hot-remove code path.

Witing anything to /sys/devices/system/cpu/cpux/online goes through online_store() in drivers/base/core.c and that does lock_device_hotplug_sysfs() which then attempts to acquire device_hotplug_lock using mutex_trylock(). And it only calls either device_online() or device_offline() if it ends up with the lock held.

Quoted 6 lines

Quite frankly, I don't see how these particular two code paths can deadlock in any way.

So either a third code path is involved which is not executed under device_hotplug_lock, or lockdep needs to be told to actually take device_hotplug_lock into account in this case IMO.

Quoted 6 lines

Hmm... all sysfs rw functions are protected from removal. ie. by default, removal of a sysfs file drains in-flight rw operations, so the hot plug path grabs a lock and then tries to remove a file and writing to the online file makes the file's write method to try to grab the same lock. It deadlocks if the hotunplug path already has the lock and trying to drain the online file for removal.

Quoted 8 lines

My point is that you cannot get into that situation. If hotplug already holds device_hotplug_lock, the write to "online" will end up doing restart_syscall().

If the "online" code path is holding the lock, hotplug cannot acquire it and cannot proceed.

Am I missing anything?

Hi Rafael,
I think your are right. The lock_device_hotplug_sysfs() has
already provided a solution for such a deadlock scenario. And there's another related code path at boot as:
smp_init()

Quoted 4 lines

        ->cpu_up()
                ->cpu_hotplug_begin()
        So it seems to be a false alarm. Any way to teach lockdep

about this to get rid of the false alarm?

Well, maybe we could call lock_device_hotplug() from that code path too?

Quoted 53 lines · Rafael J. Wysocki

Hi,

Quoted 49 lines · Jiang Liu

Quoted 31 lines · Rafael J. Wysocki

Hi Tejun,

Quoted 21 lines · Tejun Heo

Hello, Rafael.

Quoted 11 lines · Rafael J. Wysocki

So acpi_device_hotplug() calls lock_device_hotplug() which simply acquires device_hotplug_lock. It is held throughout the entire hot-add/hot-remove code path.

Witing anything to /sys/devices/system/cpu/cpux/online goes through online_store() in drivers/base/core.c and that does lock_device_hotplug_sysfs() which then attempts to acquire device_hotplug_lock using mutex_trylock(). And it only calls either device_online() or device_offline() if it ends up with the lock held.

Quoted 6 lines

Quite frankly, I don't see how these particular two code paths can deadlock in any way.

So either a third code path is involved which is not executed under device_hotplug_lock, or lockdep needs to be told to actually take device_hotplug_lock into account in this case IMO.

Quoted 6 lines

Hmm... all sysfs rw functions are protected from removal. ie. by default, removal of a sysfs file drains in-flight rw operations, so the hot plug path grabs a lock and then tries to remove a file and writing to the online file makes the file's write method to try to grab the same lock. It deadlocks if the hotunplug path already has the lock and trying to drain the online file for removal.

Quoted 8 lines

My point is that you cannot get into that situation. If hotplug already holds device_hotplug_lock, the write to "online" will end up doing restart_syscall().

If the "online" code path is holding the lock, hotplug cannot acquire it and cannot proceed.

Am I missing anything?

Hi Rafael,
I think your are right. The lock_device_hotplug_sysfs() has
already provided a solution for such a deadlock scenario. And there's another related code path at boot as:
smp_init()

        ->cpu_up()
                ->cpu_hotplug_begin()
        So it seems to be a false alarm. Any way to teach lockdep

about this to get rid of the false alarm?

Quoted 1 line

Well, maybe we could call lock_device_hotplug() from that code path too?

Hi Rafael,
Adding lock_device_hotplug() to smp_init() doesn't solve the
issue. So it seems to be an false alarm of lockdep, and I don't know how to get rid of such an lockdep false alarm:(

On Monday, September 07, 2015 11:11:19 AM Jiang Liu wrote:
Peter, Ingo, some help from lockdep expert is needed.

We have a splat that almost certainly is a false positive (the original report is here http://marc.info/?l=linux-kernel&m=144109156901959&w=4) and no ideas how to make it go away. Can you please have a look and advise?

From that email I get kernfs_fop_write() which calls

Quoted 6 lines · Rafael J. Wysocki

On Monday, September 07, 2015 11:11:19 AM Jiang Liu wrote:
Peter, Ingo, some help from lockdep expert is needed.

We have a splat that almost certainly is a false positive (the original report is here http://marc.info/?l=linux-kernel&m=144109156901959&w=4) and no ideas how to make it go away. Can you please have a look and advise?

I can't even find the relevant code :/

From that email I get kernfs_fop_write() which calls kernfs_get_active(), but that does _NOT_ call cpu_up(), so that callchain is shite.

The actual lockdep splat is also not really helpful, and is spraying names over: acpi, device, sysfs and kernfs (do we really need that many layeres of obfuscation for a simple file?)

So, please, start by explaining the thing proper such that simple people like me know what to look for.