Fix potential concurrent access problems with VFS (#108)

Since Linux v3.14, the read, write and seek operations of "struct file" are
guaranteed for thread safety [1][2]. This patch added an explanation.

Here are the potential problems:
chardev.c:
- Move the "msg_ptr" pointer into the read function to remove unnecessary usage.
- List the clear states of "already_open" by using mnemonic enumeration.

chardev2.c:
- The "buffer" in the write function is user space data. It cannot use in the
  kernel space.
- Reduce the redundant type transformation.
- List the states of "already_open". Same as chardev.c.

[1] https://lore.kernel.org/lkml/20140303210359.26624.qmail@science.horizon.com/T/#u
[2] 9c225f2655

examples/chardev.c

@@ -27,10 +27,16 @@ static ssize_t device_write(struct file *, const char __user *, size_t,
 /* Global variables are declared as static, so are global within the file. */
 
 static int major; /* major number assigned to our device driver */
+
+enum {
+    CDEV_NOT_USED = 0,
+    CDEV_EXCLUSIVE_OPEN = 1,
+};
+
 /* Is device open? Used to prevent multiple access to device */
-static atomic_t already_open = ATOMIC_INIT(0);
+static atomic_t already_open = ATOMIC_INIT(CDEV_NOT_USED);
+
 static char msg[BUF_LEN]; /* The msg the device will give when asked */
-static char *msg_ptr;
 
 static struct class *cls;
 
@@ -78,11 +84,10 @@ static int device_open(struct inode *inode, struct file *file)
 {
     static int counter = 0;
 
-    if (atomic_cmpxchg(&already_open, 0, 1))
+    if (atomic_cmpxchg(&already_open, CDEV_NOT_USED, CDEV_EXCLUSIVE_OPEN))
         return -EBUSY;
 
     sprintf(msg, "I already told you %d times Hello world!\n", counter++);
-    msg_ptr = msg;
     try_module_get(THIS_MODULE);
 
     return SUCCESS;
@@ -91,7 +96,8 @@ static int device_open(struct inode *inode, struct file *file)
 /* Called when a process closes the device file. */
 static int device_release(struct inode *inode, struct file *file)
 {
-    atomic_set(&already_open, 0); /* We're now ready for our next caller */
+    /* 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 get rid of the module.
@@ -111,10 +117,14 @@ static ssize_t device_read(struct file *filp, /* see include/linux/fs.h   */
 {
     /* Number of bytes actually written to the buffer */
     int bytes_read = 0;
+    const char *msg_ptr = msg;
 
-    /* If we are at the end of message, return 0 signifying end of file. */
+    if (!*(msg_ptr + *offset)) { /* we are at the end of message */
-    if (*msg_ptr == 0)
+        *offset = 0; /* reset the offset */
-        return 0;
+        return 0; /* signify end of file */
+    }
+
+    msg_ptr += *offset;
 
     /* Actually put the data into the buffer */
     while (length && *msg_ptr) {
@@ -124,11 +134,12 @@ static ssize_t device_read(struct file *filp, /* see include/linux/fs.h   */
          * the user data segment.
          */
         put_user(*(msg_ptr++), buffer++);
-
         length--;
         bytes_read++;
     }
 
+    *offset += bytes_read;
+
     /* Most read functions return the number of bytes put into the buffer. */
     return bytes_read;
 }

examples/chardev2.c

@@ -17,19 +17,19 @@
 #define DEVICE_NAME "char_dev"
 #define BUF_LEN 80
 
+enum {
+    CDEV_NOT_USED = 0,
+    CDEV_EXCLUSIVE_OPEN = 1,
+};
+
 /* Is the device open right now? Used to prevent concurrent access into
  * the same device
  */
-static atomic_t already_open = ATOMIC_INIT(0);
+static atomic_t already_open = ATOMIC_INIT(CDEV_NOT_USED);
 
 /* The message the device will give when asked */
 static char message[BUF_LEN];
 
-/* How far did the process reading the message get? Useful if the message
- * is larger than the size of the buffer we get to fill in device_read.
- */
-static char *message_ptr;
-
 static struct class *cls;
 
 /* This is called whenever a process attempts to open the device file */
@@ -38,11 +38,9 @@ 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, 0, 1))
+    if (atomic_cmpxchg(&already_open, CDEV_NOT_USED, CDEV_EXCLUSIVE_OPEN))
         return -EBUSY;
 
-    /* Initialize the message */
-    message_ptr = message;
     try_module_get(THIS_MODULE);
     return SUCCESS;
 }
@@ -52,7 +50,7 @@ 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, 0);
+    atomic_set(&already_open, CDEV_NOT_USED);
 
     module_put(THIS_MODULE);
     return SUCCESS;
@@ -68,12 +66,17 @@ static ssize_t device_read(struct file *file, /* see include/linux/fs.h   */
 {
     /* Number of bytes actually written to the buffer */
     int bytes_read = 0;
+    /* How far did the process reading the message get? Useful if the message
+     * is larger than the size of the buffer we get to fill in device_read.
+     */
+    const char *message_ptr = message;
 
-    pr_info("device_read(%p,%p,%ld)\n", file, buffer, length);
+    if (!*(message_ptr + *offset)) { /* we are at the end of message */
+        *offset = 0; /* reset the offset */
+        return 0; /* signify end of file */
+    }
 
-    /* If at the end of message, return 0 (which signifies end of file). */
+    message_ptr += *offset;
-    if (*message_ptr == 0)
-        return 0;
 
     /* Actually put the data into the buffer */
     while (length && *message_ptr) {
@@ -89,6 +92,8 @@ static ssize_t device_read(struct file *file, /* see include/linux/fs.h   */
 
     pr_info("Read %d bytes, %ld left\n", bytes_read, length);
 
+    *offset += bytes_read;
+
     /* Read functions are supposed to return the number of bytes actually
      * inserted into the buffer.
      */
@@ -101,13 +106,11 @@ static ssize_t device_write(struct file *file, const char __user *buffer,
 {
     int i;
 
-    pr_info("device_write(%p,%s,%ld)", file, buffer, length);
+    pr_info("device_write(%p,%p,%ld)", file, buffer, length);
 
     for (i = 0; i < length && i < BUF_LEN; i++)
         get_user(message[i], buffer + i);
 
-    message_ptr = message;
-
     /* Again, return the number of input characters used. */
     return i;
 }
@@ -126,43 +129,44 @@ device_ioctl(struct file *file, /* ditto */
              unsigned long ioctl_param)
 {
     int i;
-    char *temp;
-    char ch;
 
     /* Switch according to the ioctl called */
     switch (ioctl_num) {
-    case IOCTL_SET_MSG:
+    case IOCTL_SET_MSG: {
         /* Receive a pointer to a message (in user space) and set that to
-         * be the device's message.  Get the parameter given to ioctl by
+         * be the device's message. Get the parameter given to ioctl by
          * the process.
          */
-        temp = (char *)ioctl_param;
+        char __user *tmp = (char __user *)ioctl_param;
+        char ch;
 
         /* Find the length of the message */
-        get_user(ch, (char __user *)temp);
+        get_user(ch, tmp);
-        for (i = 0; ch && i < BUF_LEN; i++, temp++)
+        for (i = 0; ch && i < BUF_LEN; i++, tmp++)
-            get_user(ch, (char __user *)temp);
+            get_user(ch, tmp);
 
         device_write(file, (char __user *)ioctl_param, i, NULL);
         break;
+    }
+    case IOCTL_GET_MSG: {
+        loff_t offset = 0;
 
-    case IOCTL_GET_MSG:
         /* Give the current message to the calling process - the parameter
          * we got is a pointer, fill it.
          */
-        i = device_read(file, (char __user *)ioctl_param, 99, NULL);
+        i = device_read(file, (char __user *)ioctl_param, 99, &offset);
 
         /* Put a zero at the end of the buffer, so it will be properly
          * terminated.
          */
         put_user('\0', (char __user *)ioctl_param + i);
         break;
-
+    }
     case IOCTL_GET_NTH_BYTE:
         /* This ioctl is both input (ioctl_param) and output (the return
          * value of this function).
          */
-        return message[ioctl_param];
+        return (long)message[ioctl_param];
         break;
     }
 

lkmpg.tex

@@ -835,6 +835,9 @@ The meaning is clear, and you should be aware that any member of the structure w
 
 An instance of \cpp|struct file_operations| containing pointers to functions that are used to implement \cpp|read|, \cpp|write|, \cpp|open|, \ldots{} system calls is commonly named \cpp|fops|.
 
+Since Linux v3.14, the read, write and seek operations are guaranteed for thread-safe by using the \cpp|f_pos| specific lock, which makes the file position update to become the mutual exclusion.
+So, we can safely implement those operations without unnecessary locking.
+
 Since Linux v5.6, the \cpp|proc_ops| structure was introduced to replace the use of the \cpp|file_operations| structure when registering proc handlers.
 
 \subsection{The file structure}
@@ -927,8 +930,8 @@ We simply read in the data and print a message acknowledging that we received it
 In the multiple-threaded environment, without any protection, concurrent access to the same memory may lead to the race condition, and will not preserve the performance.
 In the kernel module, this problem may happen due to multiple instances accessing the shared resources.
 Therefore, a solution is to enforce the exclusive access.
-We use atomic Compare-And-Swap (CAS), the single atomic operation, to determine whether the file is currently open by someone.
+We use atomic Compare-And-Swap (CAS) to maintain the states, \cpp|CDEV_NOT_USED| and \cpp|CDEV_EXCLUSIVE_OPEN|, to determine whether the file is currently opened by someone or not.
-CAS compares the contents of a memory loaction with the expected value and, only if they are the same, modifies the contents of that memory location to the desired value.
+CAS compares the contents of a memory location with the expected value and, only if they are the same, modifies the contents of that memory location to the desired value.
 See more concurrency details in the \ref{sec:synchronization} section.
 
 \samplec{examples/chardev.c}