On Mon, Sep 20, 2021 at 2:17 PM Luis Chamberlain [off-list ref] wrote:

On Mon, Sep 20, 2021 at 01:52:21PM -0700, Dan Williams wrote:

quoted

On Fri, Sep 17, 2021 at 10:05 PM Luis Chamberlain [off-list ref] wrote:

quoted

This deadlock was first reported with the zram driver, however the live
patching folks have acknowledged they have observed this as well with
live patching, when a live patch is removed. I was then able to
reproduce easily by creating a dedicated selftests.

A sketch of how this can happen follows:

CPU A                              CPU B
                                   whatever_store()
module_unload
  mutex_lock(foo)
                                   mutex_lock(foo)
   del_gendisk(zram->disk);
     device_del()
       device_remove_groups()

This flow seems possible to trigger with:

   echo $dev > /sys/bus/$bus/drivers/$driver/unbind

I am missing why module pinning

The aspect of try_module_get() which comes to value to prevent the
deadlock is it ensures kernfs ops do not run once exit is on the way.

quoted

is part of the solution when it's the
device_del() path that is racing?

But its not, the device_del() path will yield until the kernfs op
completes. It is fine to wait there.

The deadlock happens if a module exit routine uses a lock which is
also used on a sysfs op. If the lock was first held by module exit,
and module exit is waiting for the kernfs op to complete, and the
kernfs op is waiting to hold the same lock then the exit will wait
forever.

quoted

Module removal is just a more coarse
grained way to trigger unbind => device_del().

Right, but the device_del() path is not sharing a lock with the sysfs op.

The deadlock in the example comes from holding a lock over
device_del() that is also taken in a kernfs op.  For example, the code
above looks like something that runs from driver.remove(), not
exclusively module_exit(). Yes, module_exit() may trigger
driver.remove() via driver_unregister(), but there's other ways to
trigger driver.remove() that do not involve module_exit().

quoted

Isn't the above a bug
in the driver, not missing synchronization in kernfs?

We can certainly take the position as an alternative:

  "thou shalt not use a lock on exit which is also used on a syfs op"

However that seems counter intuitive, specially if we can resolve the
issue easily with a try_module_get().

Again, I don't see how try_module_get() can affect the ABBA failure
case of holding a lock over device_del() that is also held inside
sysfs op. I agree that the problem is subtle. Does lockdep not
complain about this case? If it's going to be avoided in the core it
seems try_module_get() does not completely cover the hole that
unsuspecting driver writers might fall into.