From b859b8b86f7b7393f91bb830a8c9914ec7cce8b0 Mon Sep 17 00:00:00 2001 From: Jim Huang Date: Tue, 5 Aug 2025 17:22:55 +0800 Subject: [PATCH] Remove unsafe try_module_get(THIS_MODULE) This commit removes all instances of try_module_get(THIS_MODULE) and corresponding module_put(THIS_MODULE) calls from example modules. This pattern is considered unsafe because: - The kernel automatically manages module reference counting during file operations - Manual reference counting within a module's own code can lead to race conditions - It is redundant and unnecessary for proper module operation It also updates documentation to: - Remove references to try_module_get/module_put functions - Add note explaining why these functions should be avoided - Keep only module_refcount() as a safe display function Close #52 --- examples/chardev.c | 6 ------ examples/chardev2.c | 2 -- examples/procfs3.c | 2 -- examples/sleep.c | 19 +------------------ examples/static_key.c | 8 -------- examples/vinput.c | 5 ----- lkmpg.tex | 13 ++++++++----- 7 files changed, 9 insertions(+), 46 deletions(-) diff --git a/examples/chardev.c b/examples/chardev.c index b64305a4..129b02d1 100644 --- a/examples/chardev.c +++ b/examples/chardev.c @@ -96,7 +96,6 @@ static int device_open(struct inode *inode, struct file *file) return -EBUSY; sprintf(msg, "I already told you %d times Hello world!\n", counter++); - try_module_get(THIS_MODULE); return 0; } @@ -107,11 +106,6 @@ static int device_release(struct inode *inode, struct file *file) /* We're now ready for our next caller */ atomic_set(&already_open, CDEV_NOT_USED); - /* Decrement the usage count, or else once you opened the file, you will - * never get rid of the module. - */ - module_put(THIS_MODULE); - return 0; } diff --git a/examples/chardev2.c b/examples/chardev2.c index 0fcb63f7..e8cb6d75 100644 --- a/examples/chardev2.c +++ b/examples/chardev2.c @@ -40,7 +40,6 @@ static int device_open(struct inode *inode, struct file *file) { pr_info("device_open(%p)\n", file); - try_module_get(THIS_MODULE); return 0; } @@ -48,7 +47,6 @@ static int device_release(struct inode *inode, struct file *file) { pr_info("device_release(%p,%p)\n", inode, file); - module_put(THIS_MODULE); return 0; } diff --git a/examples/procfs3.c b/examples/procfs3.c index 38e7bd18..5428245e 100644 --- a/examples/procfs3.c +++ b/examples/procfs3.c @@ -52,12 +52,10 @@ static ssize_t procfs_write(struct file *file, const char __user *buffer, } static int procfs_open(struct inode *inode, struct file *file) { - try_module_get(THIS_MODULE); return 0; } static int procfs_close(struct inode *inode, struct file *file) { - module_put(THIS_MODULE); return 0; } diff --git a/examples/sleep.c b/examples/sleep.c index e38b1daf..ad46c9fb 100644 --- a/examples/sleep.c +++ b/examples/sleep.c @@ -95,7 +95,6 @@ static int module_open(struct inode *inode, struct file *file) /* Try to get without blocking */ if (!atomic_cmpxchg(&already_open, 0, 1)) { /* Success without blocking, allow the access */ - try_module_get(THIS_MODULE); return 0; } /* If the file's flags include O_NONBLOCK, it means the process does not @@ -106,12 +105,6 @@ static int module_open(struct inode *inode, struct file *file) if (file->f_flags & O_NONBLOCK) return -EAGAIN; - /* This is the correct place for try_module_get(THIS_MODULE) because if - * a process is in the loop, which is within the kernel module, - * the kernel module must not be removed. - */ - try_module_get(THIS_MODULE); - while (atomic_cmpxchg(&already_open, 0, 1)) { int i, is_sig = 0; @@ -132,15 +125,7 @@ static int module_open(struct inode *inode, struct file *file) is_sig = current->pending.signal.sig[i] & ~current->blocked.sig[i]; if (is_sig) { - /* It is important to put module_put(THIS_MODULE) here, because - * for processes where the open is interrupted there will never - * be a corresponding close. If we do not decrement the usage - * count here, we will be left with a positive usage count - * which we will have no way to bring down to zero, giving us - * an immortal module, which can only be killed by rebooting - * the machine. - */ - module_put(THIS_MODULE); + /* Return -EINTR if we got a signal */ return -EINTR; } } @@ -163,8 +148,6 @@ static int module_close(struct inode *inode, struct file *file) */ wake_up(&waitq); - module_put(THIS_MODULE); - return 0; /* success */ } diff --git a/examples/static_key.c b/examples/static_key.c index 1b5b7399..b7be8e19 100644 --- a/examples/static_key.c +++ b/examples/static_key.c @@ -100,8 +100,6 @@ static int device_open(struct inode *inode, struct file *file) pr_alert("do unlikely thing\n"); pr_info("fastpath 2\n"); - try_module_get(THIS_MODULE); - return 0; } @@ -113,12 +111,6 @@ static int device_release(struct inode *inode, struct file *file) /* We are now ready for our next caller. */ atomic_set(&already_open, CDEV_NOT_USED); - /** - * Decrement the usage count, or else once you opened the file, you will - * never get rid of the module. - */ - module_put(THIS_MODULE); - return 0; } diff --git a/examples/vinput.c b/examples/vinput.c index 0b33b185..1b0a6bad 100644 --- a/examples/vinput.c +++ b/examples/vinput.c @@ -157,8 +157,6 @@ static void vinput_destroy_vdevice(struct vinput *vinput) clear_bit(vinput->id, vinput_ids); spin_unlock(&vinput_lock); - module_put(THIS_MODULE); - kfree(vinput); } @@ -182,8 +180,6 @@ static struct vinput *vinput_alloc_vdevice(void) return ERR_PTR(-ENOMEM); } - try_module_get(THIS_MODULE); - spin_lock_init(&vinput->lock); spin_lock(&vinput_lock); @@ -217,7 +213,6 @@ static struct vinput *vinput_alloc_vdevice(void) list_del(&vinput->list); fail_id: spin_unlock(&vinput_lock); - module_put(THIS_MODULE); kfree(vinput); return ERR_PTR(err); diff --git a/lkmpg.tex b/lkmpg.tex index 34f33905..ceecca91 100644 --- a/lkmpg.tex +++ b/lkmpg.tex @@ -1095,16 +1095,17 @@ \subsection{Unregistering A Device} You can see what its value is by looking at the 3rd field with the command \sh|cat /proc/modules| or \sh|lsmod|. If this number isn't zero, \sh|rmmod| will fail. Note that you do not have to check the counter within \cpp|cleanup_module| because the check will be performed for you by the system call \cpp|sys_delete_module|, defined in \src{include/linux/syscalls.h}. -You should not use this counter directly, but there are functions defined in \src{include/linux/module.h} which let you increase, decrease and display this counter: +You should not use this counter directly, but there are functions defined in \src{include/linux/module.h} which let you display this counter: \begin{itemize} - \item \cpp|try_module_get(THIS_MODULE)|: Increment the reference count of current module. - \item \cpp|module_put(THIS_MODULE)|: Decrement the reference count of current module. \item \cpp|module_refcount(THIS_MODULE)|: Return the value of reference count of current module. \end{itemize} -It is important to keep the counter accurate; if you ever lose track of the correct usage count, you will never be able to unload the module; it is now reboot time. -This is bound to happen to you sooner or later during a module's development. +Note: The use of \cpp|try_module_get(THIS_MODULE)| and \cpp|module_put(THIS_MODULE)| within a module's own code is considered unsafe and should be avoided. +The kernel automatically manages the reference count when file operations are in progress, +so manual reference counting is unnecessary and can lead to race conditions. +For a deeper understanding of when and how to properly use module reference counting, +see \url{https://stackoverflow.com/questions/1741415/linux-kernel-modules-when-to-use-try-module-get-module-put}. \subsection{chardev.c} \label{sec:chardev_c} @@ -1152,6 +1153,8 @@ \section{The /proc Filesystem} The inode contains information about the file, for example the file's permissions, together with a pointer to the disk location or locations where the file's data can be found. Because we do not get called when the file is opened or closed, there is nowhere for us to put \cpp|try_module_get| and \cpp|module_put| in this module, and if the file is opened and then the module is removed, there is no way to avoid the consequences. +The kernel's automatic reference counting for file operations helps prevent module removal while files are in use, +but \verb|/proc| files require careful handling due to their different lifecycle. Here is a simple example showing how to use a \verb|/proc| file. This is the HelloWorld for the \verb|/proc| filesystem.