RE: [PATCH] kernel/module: add documentation for try_module_get()

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)