Re: [PATCH v8 09/12] sysfs: fix deadlock race with module removal

From: Ming Lei <ming.lei@redhat.com>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Miroslav Benes <mbenes@suse.cz>,
	tj@kernel.org, gregkh@linuxfoundation.org,
	akpm@linux-foundation.org, minchan@kernel.org, jeyu@kernel.org,
	shuah@kernel.org, bvanassche@acm.org, dan.j.williams@intel.com,
	joe@perches.com, tglx@linutronix.de, keescook@chromium.org,
	rostedt@goodmis.org, linux-spdx@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-block@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-kernel@vger.kernel.org, ming.lei@redhat.com
Subject: Re: [PATCH v8 09/12] sysfs: fix deadlock race with module removal
Date: Wed, 13 Oct 2021 23:04:07 +0800	[thread overview]
Message-ID: <YWb1Z7EXruo6gaEp@T590> (raw)
In-Reply-To: <YWbSk6p3bfXUPZ92@bombadil.infradead.org>

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=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.
> > > > 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