Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Luis Chamberlain <mcgrof@kernel.org>
To: David Laight <David.Laight@aculab.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"tj@kernel.org" <tj@kernel.org>,
	"shuah@kernel.org" <shuah@kernel.org>,
	"akpm@linux-foundation.org" <akpm@linux-foundation.org>,
	"rafael@kernel.org" <rafael@kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"ast@kernel.org" <ast@kernel.org>,
	"andriin@fb.com" <andriin@fb.com>,
	"daniel@iogearbox.net" <daniel@iogearbox.net>,
	"atenart@kernel.org" <atenart@kernel.org>,
	"alobakin@pm.me" <alobakin@pm.me>,
	"weiwan@google.com" <weiwan@google.com>,
	"ap420073@gmail.com" <ap420073@gmail.com>,
	"jeyu@kernel.org" <jeyu@kernel.org>,
	"ngupta@vflare.org" <ngupta@vflare.org>,
	"sergey.senozhatsky.work@gmail.com" 
	<sergey.senozhatsky.work@gmail.com>,
	"minchan@kernel.org" <minchan@kernel.org>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"mbenes@suse.com" <mbenes@suse.com>,
	"jpoimboe@redhat.com" <jpoimboe@redhat.com>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"jikos@kernel.org" <jikos@kernel.org>,
	"rostedt@goodmis.org" <rostedt@goodmis.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Douglas Gilbert <dgilbert@interlog.com>,
	Hannes Reinecke <hare@suse.de>,
	"linux-kselftest@vger.kernel.org"
	<linux-kselftest@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] kernel/module: add documentation for try_module_get()
Date: Wed, 28 Jul 2021 06:49:03 -0700	[thread overview]
Message-ID: <YQFgTxz62G+3Lc8G@bombadil.infradead.org> (raw)
In-Reply-To: <6054c136290346d581e276abbb2e3ff1@AcuMS.aculab.com>

On Wed, Jul 28, 2021 at 08:28:11AM +0000, David Laight wrote:
> ...
> > sysfs files are safe to use try_module_get() because once they are
> > active a removal of the file cannot happen, and so removal will wait.
> 
> I doubt it.

But that is what happens.

> If the module_remove() function removes sysfs nodes then (something
> like) this has to happen.
> 
> 1) rmmod (or similar) tries to remove the module.
> 2) The reference count is zero so the remove is allowed.
> 3) Something tries to access a sysfs node in the module.
> 3a) If sysfs knew the nodes were in a module it could use
>     try_module_get() to ensure the module wasn't being unloaded.
>     Failure would cause the sysfs access to fail.
>     But I'm not sure it does,


It does, if a sysfs file had a try_module_get() it would fail as the
module is going.

>     and in any case it doesn't help.

Not clear how from your example.

> 3b) The sysfs thread calls into the module code and waits on a mutex.

If try_module_get() is used on the syfs files, the deadlock is escaped if
used on remove.

> 3c) The rmmod thread gets around to calling into sysfs to remove the nodes.
> 
> At this point we hit the standard 'deregistering a callback' issue.
> Exactly the same issue affects removal of per-device sysfs node
> from a driver's .remove function.
> 
> Typically this is solved by making the deregister routing sleep
> until all the callbacks have completed.
> 
> So this would require functions like sysfs_remove_group() and
> hwmon_device_unregister() to be allowed to sleep

Both can.

Both kernfs_find_and_get_ns() and kernfs_remove_by_name_ns() call
mutex_lock(), they certainly can sleep.

hwmon_device_unregister() calls device_del() which also holds a mutex.

> and not be
> called with any locks (of any kind) held that the callback
> functions acquire.

Not sure why you think this is a requirement.

> The module reference count is irrelevant.

To be clear, there were concerns that there were races here which would
make things murky on sysfs operations and module removal (null
deferences when accessing back the gendisk->private_data) however a
a new selftest driver for sysfs [0], and error injections to allow us to
test and verify all these things I just said are true. If you'd like
to extend the tests to include something you might be concerned about
and want to try, please send me a patch against my tree [1].

[0] https://lkml.kernel.org/r/20210703004632.621662-1-mcgrof@kernel.org
[1] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20210701-sysfs-fix-races-v2

  Luis

      reply	other threads:[~2021-07-28 13:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-22 22:19 Luis Chamberlain
2021-07-22 22:39 ` Stephen Hemminger
2021-07-23  2:33 ` Bart Van Assche
2021-07-24 12:15 ` David Laight
2021-07-27 17:30   ` Luis Chamberlain
2021-07-27 17:46     ` gregkh
2021-07-27 18:18       ` Luis Chamberlain
2021-07-27 18:38         ` gregkh
2021-07-27 20:54           ` Luis Chamberlain
2021-07-28  8:28             ` David Laight
2021-07-28 13:49               ` Luis Chamberlain [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YQFgTxz62G+3Lc8G@bombadil.infradead.org \
    --to=mcgrof@kernel.org \
    --cc=David.Laight@aculab.com \
    --cc=akpm@linux-foundation.org \
    --cc=alobakin@pm.me \
    --cc=andriin@fb.com \
    --cc=ap420073@gmail.com \
    --cc=ast@kernel.org \
    --cc=atenart@kernel.org \
    --cc=axboe@kernel.dk \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dgilbert@interlog.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hare@suse.de \
    --cc=jeyu@kernel.org \
    --cc=jikos@kernel.org \
    --cc=jpoimboe@redhat.com \
    --cc=keescook@chromium.org \
    --cc=kuba@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mbenes@suse.com \
    --cc=minchan@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ngupta@vflare.org \
    --cc=peterz@infradead.org \
    --cc=rafael@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=shuah@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=weiwan@google.com \
    --subject='Re: [PATCH] kernel/module: add documentation for try_module_get()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).