From beb1ff1595656cf8e1942fbaa1980c0de1491320 Mon Sep 17 00:00:00 2001 From: linD026 Date: Sun, 17 Apr 2022 00:23:49 +0800 Subject: [PATCH] Fix potential concurrent problems in chardev2.c After forking, Each file descriptor in the child refers to the same open file description as the parent. So when calling open() before fork(), the child can access the device file without checking by exclusive access in device_open(). It may cause race conditions in device_ioctl(). Because of that, it is unnecessary to check the multiple access in device_open(). It just needs check in device_ioctl(), since read(), write(), seek() system call are atomic [1][2]. Related discussion: - https://github.com/sysprog21/lkmpg/issues/148 [1] https://lore.kernel.org/lkml/53022DB1.4070805@gmail.com/ [2] https://www.kernel.org/doc/html/latest/filesystems/files.html Close #148 --- examples/chardev2.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/examples/chardev2.c b/examples/chardev2.c index 7690070..db5e24d 100644 --- a/examples/chardev2.c +++ b/examples/chardev2.c @@ -37,10 +37,6 @@ static int device_open(struct inode *inode, struct file *file) { pr_info("device_open(%p)\n", file); - /* We don't want to talk to two processes at the same time. */ - if (atomic_cmpxchg(&already_open, CDEV_NOT_USED, CDEV_EXCLUSIVE_OPEN)) - return -EBUSY; - try_module_get(THIS_MODULE); return SUCCESS; } @@ -49,9 +45,6 @@ static int device_release(struct inode *inode, struct file *file) { pr_info("device_release(%p,%p)\n", inode, file); - /* We're now ready for our next caller */ - atomic_set(&already_open, CDEV_NOT_USED); - module_put(THIS_MODULE); return SUCCESS; } @@ -129,6 +122,11 @@ device_ioctl(struct file *file, /* ditto */ unsigned long ioctl_param) { int i; + long ret = SUCCESS; + + /* We don't want to talk to two processes at the same time. */ + if (atomic_cmpxchg(&already_open, CDEV_NOT_USED, CDEV_EXCLUSIVE_OPEN)) + return -EBUSY; /* Switch according to the ioctl called */ switch (ioctl_num) { @@ -166,11 +164,14 @@ device_ioctl(struct file *file, /* ditto */ /* This ioctl is both input (ioctl_param) and output (the return * value of this function). */ - return (long)message[ioctl_param]; + ret = (long)message[ioctl_param]; break; } - return SUCCESS; + /* We're now ready for our next caller */ + atomic_set(&already_open, CDEV_NOT_USED); + + return ret; } /* Module Declarations */