Message-Id: <20230310190457.3779415-1-mcgrof@kernel.org>
Date:   Fri, 10 Mar 2023 11:04:57 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org,
        tj@...nel.org, bpf@...r.kernel.org
Cc:     christophe.leroy@...roup.eu, jolsa@...nel.org, vmalik@...hat.com,
        john.fastabend@...il.com, andrii@...nel.org, martin.lau@...ux.dev,
        song@...nel.org, yhs@...com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, nick.alcock@...cle.com, keescook@...omium.org,
        gregkh@...uxfoundation.org, tglx@...utronix.de, mcgrof@...nel.org
Subject: [PATCH] kernel/module: add documentation for try_module_get()

There is quite a bit of tribal knowledge around proper use of try_module_get()
and requiring *somehow* the module to still exist to use this call in a way
that is safe. Document this bit of tribal knowledge. To be clear, you should
only use try_module_get() *iff* you are 100% sure the module already does
exist and is not on its way out.

You can be sure the module still exists and is alive through:

1) Direct protection with its refcount: you know some earlier caller called
   __module_get() safely
2) Implied protection: there is an implied protection against module removal

Having an idea of when you are sure __module_get() might be called earlier is
easy to understand however the implied protection requires an example. We use
sysfs an an example for implied protection without a direct module reference
count bump. kernfs / sysfs uses its own internal reference counting for files
being actively used, when such file are active they completely prevent
the module from being removed. kernfs protects this with its kernfs_active().
Effort has been put into verifying the kernfs implied protection works by
using a currently out-of-tree test_sysfs selftest test #32 [0]:

./tools/testing/selftests/sysfs/sysfs.sh -t 0032

Without kernfs / sysfs preventing module removal through its active reference
count (kernfs_active()) the write would fail or worse, a crash would happen in
this test and it does not.

Similar safeguards are required for other users of try_module_get() *iff*
they are not ensuring the above rule 1) is followed.

[0] https://lore.kernel.org/lkml/20211029184500.2821444-4-mcgrof@kernel.org/

Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>
---

I had actually written an initial version of these docs [0] *two years* ago
through my test_sysfs patches [1]. Back then the only pushback I got to get this
test upstream was Greg's opposition to me using copyleft-next for my test driver.
Upon feedback from the comunity, we've now have passed that hurtle, and we
have proper SPDX tags for copyleft-next driver upstream. Before I follow up
again with that test_sysfs driver though *this* patch in particular seems
important enough to get upstream and so sending this separately because
I see some folks might still be in high need of these docs.

I'll reply to the thread / patch in question next.

[0] https://lore.kernel.org/all/20211029184500.2821444-7-mcgrof@kernel.org/
[1] https://lore.kernel.org/all/20211029184500.2821444-1-mcgrof@kernel.org/

 include/linux/module.h | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 91726444d55f..c3b357196470 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -671,10 +671,46 @@ void symbol_put_addr(void *addr);
    to handle the error case (which only happens with rmmod --wait). */
 extern void __module_get(struct module *module);
 
-/* This is the Right Way to get a module: if it fails, it's being removed,
- * so pretend it's not there. */
+/**
+ * try_module_get() - take module refcount unless module is being removed
+ * @module: the module we should check for
+ *
+ * Only try to get a module reference count if the module is not being removed.
+ * This call will fail if the module is already being removed.
+ *
+ * Care must also be taken to ensure the module exists and is alive prior to
+ * usage of this call. This can be gauranteed through two means:
+ *
+ * 1) Direct protection: you know an earlier caller must have increased the
+ *    module reference through __module_get(). This can typically be achieved
+ *    by having another entity other than the module itself increment the
+ *    module reference count.
+ *
+ * 2) Implied protection: there is an implied protection against module
+ *    removal. An example of this is the implied protection used by kernfs /
+ *    sysfs. The sysfs store / read file operations are guaranteed to exist
+ *    through the use of kernfs's active reference (see kernfs_active()) and a
+ *    sysfs / kernfs file removal cannot happen unless the same file is not
+ *    active. Therefore, if a sysfs file is being read or written to the module
+ *    which created it must still exist. It is therefore safe to use
+ *    try_module_get() on module sysfs store / read ops.
+ *
+ * One of the real values to try_module_get() is the module_is_live() check
+ * which ensures that the caller of try_module_get() can yield to userspace
+ * module removal requests and gracefully fail if the module is on its way out.
+ *
+ * Returns true if the reference count was successfully incremented.
+ */
 extern bool try_module_get(struct module *module);
 
+/**
+ * module_put() - release a reference count to a module
+ * @module: the module we should release a reference count for
+ *
+ * If you successfully bump a reference count to a module with try_module_get(),
+ * when you are finished you must call module_put() to release that reference
+ * count.
+ */
 extern void module_put(struct module *module);
 
 #else /*!CONFIG_MODULE_UNLOAD*/
-- 
2.39.1

Powered by blists - more mailing lists