Skip to content

Commit 7a3f63d

Browse files
committed
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
1 parent b9ed1a7 commit 7a3f63d

File tree

7 files changed

+4
-46
lines changed

7 files changed

+4
-46
lines changed

examples/chardev.c

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,6 @@ static int device_open(struct inode *inode, struct file *file)
9696
return -EBUSY;
9797

9898
sprintf(msg, "I already told you %d times Hello world!\n", counter++);
99-
try_module_get(THIS_MODULE);
10099

101100
return 0;
102101
}
@@ -107,11 +106,6 @@ static int device_release(struct inode *inode, struct file *file)
107106
/* We're now ready for our next caller */
108107
atomic_set(&already_open, CDEV_NOT_USED);
109108

110-
/* Decrement the usage count, or else once you opened the file, you will
111-
* never get rid of the module.
112-
*/
113-
module_put(THIS_MODULE);
114-
115109
return 0;
116110
}
117111

examples/chardev2.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,13 @@ static int device_open(struct inode *inode, struct file *file)
4040
{
4141
pr_info("device_open(%p)\n", file);
4242

43-
try_module_get(THIS_MODULE);
4443
return 0;
4544
}
4645

4746
static int device_release(struct inode *inode, struct file *file)
4847
{
4948
pr_info("device_release(%p,%p)\n", inode, file);
5049

51-
module_put(THIS_MODULE);
5250
return 0;
5351
}
5452

examples/procfs3.c

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,10 @@ static ssize_t procfs_write(struct file *file, const char __user *buffer,
5252
}
5353
static int procfs_open(struct inode *inode, struct file *file)
5454
{
55-
try_module_get(THIS_MODULE);
5655
return 0;
5756
}
5857
static int procfs_close(struct inode *inode, struct file *file)
5958
{
60-
module_put(THIS_MODULE);
6159
return 0;
6260
}
6361

examples/sleep.c

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ static int module_open(struct inode *inode, struct file *file)
9595
/* Try to get without blocking */
9696
if (!atomic_cmpxchg(&already_open, 0, 1)) {
9797
/* Success without blocking, allow the access */
98-
try_module_get(THIS_MODULE);
9998
return 0;
10099
}
101100
/* 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)
106105
if (file->f_flags & O_NONBLOCK)
107106
return -EAGAIN;
108107

109-
/* This is the correct place for try_module_get(THIS_MODULE) because if
110-
* a process is in the loop, which is within the kernel module,
111-
* the kernel module must not be removed.
112-
*/
113-
try_module_get(THIS_MODULE);
114-
115108
while (atomic_cmpxchg(&already_open, 0, 1)) {
116109
int i, is_sig = 0;
117110

@@ -132,15 +125,7 @@ static int module_open(struct inode *inode, struct file *file)
132125
is_sig = current->pending.signal.sig[i] & ~current->blocked.sig[i];
133126

134127
if (is_sig) {
135-
/* It is important to put module_put(THIS_MODULE) here, because
136-
* for processes where the open is interrupted there will never
137-
* be a corresponding close. If we do not decrement the usage
138-
* count here, we will be left with a positive usage count
139-
* which we will have no way to bring down to zero, giving us
140-
* an immortal module, which can only be killed by rebooting
141-
* the machine.
142-
*/
143-
module_put(THIS_MODULE);
128+
/* Return -EINTR if we got a signal */
144129
return -EINTR;
145130
}
146131
}
@@ -163,8 +148,6 @@ static int module_close(struct inode *inode, struct file *file)
163148
*/
164149
wake_up(&waitq);
165150

166-
module_put(THIS_MODULE);
167-
168151
return 0; /* success */
169152
}
170153

examples/static_key.c

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,6 @@ static int device_open(struct inode *inode, struct file *file)
100100
pr_alert("do unlikely thing\n");
101101
pr_info("fastpath 2\n");
102102

103-
try_module_get(THIS_MODULE);
104-
105103
return 0;
106104
}
107105

@@ -113,12 +111,6 @@ static int device_release(struct inode *inode, struct file *file)
113111
/* We are now ready for our next caller. */
114112
atomic_set(&already_open, CDEV_NOT_USED);
115113

116-
/**
117-
* Decrement the usage count, or else once you opened the file, you will
118-
* never get rid of the module.
119-
*/
120-
module_put(THIS_MODULE);
121-
122114
return 0;
123115
}
124116

examples/vinput.c

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,6 @@ static void vinput_destroy_vdevice(struct vinput *vinput)
157157
clear_bit(vinput->id, vinput_ids);
158158
spin_unlock(&vinput_lock);
159159

160-
module_put(THIS_MODULE);
161-
162160
kfree(vinput);
163161
}
164162

@@ -182,8 +180,6 @@ static struct vinput *vinput_alloc_vdevice(void)
182180
return ERR_PTR(-ENOMEM);
183181
}
184182

185-
try_module_get(THIS_MODULE);
186-
187183
spin_lock_init(&vinput->lock);
188184

189185
spin_lock(&vinput_lock);
@@ -217,7 +213,6 @@ static struct vinput *vinput_alloc_vdevice(void)
217213
list_del(&vinput->list);
218214
fail_id:
219215
spin_unlock(&vinput_lock);
220-
module_put(THIS_MODULE);
221216
kfree(vinput);
222217

223218
return ERR_PTR(err);

lkmpg.tex

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1092,16 +1092,14 @@ \subsection{Unregistering A Device}
10921092
You can see what its value is by looking at the 3rd field with the command \sh|cat /proc/modules| or \sh|lsmod|.
10931093
If this number isn't zero, \sh|rmmod| will fail.
10941094
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}.
1095-
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:
1095+
You should not use this counter directly, but there are functions defined in \src{include/linux/module.h} which let you display this counter:
10961096

10971097
\begin{itemize}
1098-
\item \cpp|try_module_get(THIS_MODULE)|: Increment the reference count of current module.
1099-
\item \cpp|module_put(THIS_MODULE)|: Decrement the reference count of current module.
11001098
\item \cpp|module_refcount(THIS_MODULE)|: Return the value of reference count of current module.
11011099
\end{itemize}
11021100

1103-
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.
1104-
This is bound to happen to you sooner or later during a module's development.
1101+
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.
1102+
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.
11051103

11061104
\subsection{chardev.c}
11071105
\label{sec:chardev_c}

0 commit comments

Comments
 (0)