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

From: Luis Chamberlain <mcgrof@kernel.org>
To: Ming Lei <ming.lei@redhat.com>, Miroslav Benes <mbenes@suse.cz>
Cc: 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
Subject: Re: [PATCH v8 09/12] sysfs: fix deadlock race with module removal
Date: Wed, 13 Oct 2021 05:35:31 -0700	[thread overview]
Message-ID: <YWbSk6p3bfXUPZ92@bombadil.infradead.org> (raw)
In-Reply-To: <YWYxN875B6rlmAjC@T590>

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.

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

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
was hinted to me live patching was also affected, and so clearly
something generic was desirable.

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

  Luis