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:
On Tue, Oct 12, 2021 at 02:18:28PM -0700, Luis Chamberlain wrote:
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...
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.
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 -->
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.
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.
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.
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