On Wed, Oct 13, 2021 at 05:35:31AM -0700, Luis Chamberlain wrote:

On Wed, Oct 13, 2021 at 09:07:03AM +0800, Ming Lei wrote:

quoted

On Tue, Oct 12, 2021 at 02:18:28PM -0700, Luis Chamberlain wrote:

quoted

quoted

Looks test_sysfs isn't in linus tree, where can I find it?

https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210927-sysfs-generic-deadlock-fix

To reproduce the deadlock revert the patch in this thread and then run
either of these two tests as root:

./tools/testing/selftests/sysfs/sysfs.sh -w 0027
./tools/testing/selftests/sysfs/sysfs.sh -w 0028

You will need to enable the test_sysfs driver.

quoted

Can you share the code which waits for the sysfs / kernfs files to be
stop being used?

How about a call trace of the two tasks which deadlock, here is one of
running test 0027:

kdevops login: [  363.875459] INFO: task sysfs.sh:1271 blocked for more
than 120 seconds.

<-- snip -->

quoted

That doesn't show the deadlock is related with module_exit().

Not directly no.

Then the patch title of 'sysfs: fix deadlock race with module removal'
is wrong.

quoted

It is clearly one AA deadlock, what I meant was that it isn't related with
module exit cause lock & device_del() isn't always done in module exit, so
I doubt your fix with grabbing module refcnt is good or generic enough.

A device_del() *can* happen in other areas other than module exit sure,
but the issue is if a shared lock is used *before* device_del() and also
used on a sysfs op. Typically this can happen on module exit, and the
other common use case in my experience is on sysfs ops, such is the case
with the zram driver. Both cases are covered then by this fix.

Again, can you share the related zram code about the issue? In
zram_drv.c of linus or next tree, I don't see any lock is held before
calling del_gendisk().

If there are other areas, that is still driver specific, but of the
things we *can* generalize, definitely module exit is a common path.

quoted

Except for your cooked test_sys module, how many real drivers do suffer the
problem? What are they?

I only really seriously considered trying to generalize this after it

IMO your generalization isn't good or correct because this kind of issue
is _not_ related with module exit at all. What matters is just that one lock is
held before calling device_del(), meantime the same lock is required
in the device's attribute show/store function().

There are many cases in which we call device_del() not from module_exit(),
such as scsi scan, scsi sysfs store(), or even handling event from
device side, nvme error handling, usb hotplug, ...

was hinted to me live patching was also affected, and so clearly
something generic was desirable.

It might be just the only two drivers(zram and live patch) with this bug, and
it is one simply AA bug in driver. Not mention I don't see such usage in
zram_drv.c.

There may be other drivers for sure, but a hunt for that with semantics
would require a bit complex coccinelle patch with iteration support.

quoted

Why can't we fix the exact driver?

You can try, the way the lock is used in zram is correct, specially

What is the lock in zram? Again can you share the related functions?

after my other fix in this series which addresses another unrelated bug
with cpu hotplug multistate support. So we then can proceed to either
take the position to say: "Thou shalt not use a shared lock on module
exit and a sysfs op" and try to fix all places, or we generalize a fix
for this. A generic fix seems more desirable.

What matters is that the lock is held before calling device_del()
instead of being held in module_exit().



Thanks,
Ming