Add the description about some file ops and fix several problems

[linD026]

Even though the concurrent access to the module has been fixed at commit 148fb01,
the function of device operations can be called concurrently.

In chardev2.c, the message buffer can be simultaneously accessed by the read and
write operations. It can solve by allowing only one of the operations to work at
the time.
In chardev.c, the msg_ptr is the global variable. Therefore, when someone is
concurrently using the read operation will lead to a data race.
The pointer can change into the local variable of the read operation. So, each
read operation will not use the same pointer at the same time.

---

Edited:

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

Following are the trivial 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] torvalds/linux@9c225f2

• sysprog21/lkmpg: Several race conditions in chardev.c? #93

[linD026]

According to this comment:

Sometime, the read function from user space will fail (can't get the message) because the "Offset" in the open file table is not correct.
They all use the same open file table entry so there will also have race condition if we update the offset before device_read() return the bytes_read.

With multi-read, the "offset" variable will have a race condition.
Do we need to deal with it?
If so, the read function may become like:

char *msg_ptr = msg;

while (length && *msg_ptr) {
    put_user(*(msg_ptr++), buffer++);
    length--;
}

return 0;

[jserv]

I would like to ask @afiskon for reviewing.

[linD026]

From this mail list, when the reference count of struct file (files->count)[1] is larger than 1, it will atomically do the {read, write, lseek} syscall[2][3].

• file.c

unsigned long __fdget_pos(unsigned int fd)
{
	unsigned long v = __fdget(fd);
	struct file *file = (struct file *)(v & ~3);

	if (file && (file->f_mode & FMODE_ATOMIC_POS)) {
		if (file_count(file) > 1) {
			v |= FDPUT_POS_UNLOCK;
			mutex_lock(&file->f_pos_lock);
		}
	}
	return v;
}

So, the f_pos variable will not have the race condition, which is the stored offset value in the struct file.

• fs.h

struct file {
	loff_t			f_pos;
}

• read_write.c

ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
{
	struct fd f = fdget_pos(fd);
	ssize_t ret = -EBADF;

	if (f.file) {
		loff_t pos, *ppos = file_ppos(f.file);
		if (ppos) {
			pos = *ppos;
			ppos = &pos;
		}
		ret = vfs_read(f.file, buf, count, ppos);
		if (ret >= 0 && ppos)
			f.file->f_pos = pos;
		fdput_pos(f);
	}
	return ret;
}

So, do we have to change the example code?

[linD026]

I tested it.
With the following changes, It seems like the read and write functions will work flawlessly.

static ssize_t device_read(struct file *filp, /* see include/linux/fs.h   */
                           char __user *buffer, /* buffer to fill with data */
                           size_t length, /* length of the buffer     */
                           loff_t *offset)
{
    int bytes_read = 0;
    const char *msg_ptr = msg;

    if (!*(msg_ptr + *offset)) { /* we are at the end of message */
        *offset = 0; /* reset the offset */
        return 0; /* signify end of file */
    }

    msg_ptr += *offset;

    while (length && *msg_ptr) {
        put_user(*(msg_ptr++), buffer++);

        length--;
        bytes_read++;
    }

    *offset += bytes_read;

    return bytes_read;
}

test code:

#define _GNU_SOURCE
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <pthread.h>

int fd;

#define len 10

void worker(void *args)
{
    char buf[len];

    read(fd, buf, len);
    printf("unlock %d: \"%s\"\n", gettid(), buf);

    pthread_exit(NULL);
}

int main(int argc, char *argv[])
{
    pthread_t t[10];
    
    fd = open("/dev/chardev", O_RDWR);

    for (int i = 0;i < 10;i++)
        pthread_create(&t[i], NULL, worker, NULL);
    
    for (int i = 0;i < 10;i++)
        pthread_join(t[i], NULL);
    
    return 0;
}

output (it is serialized output):

unlock 15733: "I already "
unlock 15732: "told you 2"
unlock 15734: " times Hel"
unlock 15735: "lo world!
"
unlock 15731: ""
unlock 15736: "I already "
unlock 15730: "told you 2"
unlock 15729: " times Hel"
unlock 15728: "lo world!
"
unlock 15727: ""