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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YWb1Z7EXruo6gaEp@T590>
Date:   Wed, 13 Oct 2021 23:04:07 +0800
From:   Ming Lei <ming.lei@...hat.com>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Miroslav Benes <mbenes@...e.cz>, tj@...nel.org,
        gregkh@...uxfoundation.org, akpm@...ux-foundation.org,
        minchan@...nel.org, jeyu@...nel.org, shuah@...nel.org,
        bvanassche@....org, dan.j.williams@...el.com, joe@...ches.com,
        tglx@...utronix.de, keescook@...omium.org, rostedt@...dmis.org,
        linux-spdx@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-block@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        ming.lei@...hat.com
Subject: Re: [PATCH v8 09/12] sysfs: fix deadlock race with module removal

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