Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: David Laight <David.Laight@ACULAB.COM>
To: 'Luis Chamberlain' <mcgrof@kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: "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 08:28:11 +0000	[thread overview]
Message-ID: <6054c136290346d581e276abbb2e3ff1@AcuMS.aculab.com> (raw)
In-Reply-To: <YQByfUaDaXCUqrlo@bombadil.infradead.org>

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

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, and in any case it doesn't help.
3b) The sysfs thread calls into the module code and waits on a mutex.
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 and not be
called with any locks (of any kind) held that the callback
functions acquire.

The module reference count is irrelevant.

	David

    

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


  reply	other threads:[~2021-07-28  8:28 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 [this message]
2021-07-28 13:49               ` Luis Chamberlain

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=6054c136290346d581e276abbb2e3ff1@AcuMS.aculab.com \
    --to=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=mcgrof@kernel.org \
    --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).