Merge pull request #328 from sysprog21/drop-this-module

Remove unsafe try_module_get(THIS_MODULE)

Author: jserv

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;
 }
 

examples/chardev2.c

@@ -40,15 +40,13 @@ static int device_open(struct inode *inode, struct file *file)
 {
     pr_info("device_open(%p)\n", file);
 
-    try_module_get(THIS_MODULE);
     return 0;
 }
 
 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;
 }
 

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;
 }
 

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 */
 }
 

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;
 }
 

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);

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.