Re: [PATCH RESEND] kernel/relay.c: fix read_pos error when multiple readers

[Pengcheng Yang]

On Fri, 24 Jan 2020 9:23:26 +0800 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> 
> On Wed, 22 Jan 2020 19:06:15 +0800 Pengcheng Yang <yangpc@xxxxxxxxxx> wrote:
> 
> > When reading, read_pos should start with bytes_consumed,
> > not file->f_pos. Because when there is more than one reader,
> > the read_pos corresponding to file->f_pos may have been consumed,
> > which will cause the data that has been consumed to be read
> > and the bytes_consumed update error.
> 
> That sounds fairly serious.  Are you able to describe a userspace setup
> which will trigger this?  Do you have any test code which is able to
> demonstrate the bug?
> 
> We really should have a relay testcase in tools/testing, but relay came
> along before we became diligent about this.


Thanks for following this case!

The problem is that Relay uses the file->f_pos associated with 
the process to read the buffer, and adds the number of bytes read 
to rchan_buf->bytes_consumed.

Here is a simple test case, the kernel module relay_test writes 
the string "12345678" in module_init through relay_write; 
the relay_read program uses two processes to alternately 
read 2 bytes from Relay.
It shows that process A reads the string "34" that has been 
consumed by process B, and the string "56" may be skipped next.


========
Module relay_test:

#include <linux/module.h>
#include <linux/relay.h>
#include <linux/debugfs.h>

static struct rchan *chan = NULL;
static struct dentry *dir = NULL;

static size_t subbuf_size = 512;
static int n_subbufs = 32;

static struct dentry *create_buf_file_handler(const char *filename,
					      struct dentry *parent,
					      umode_t mode,
					      struct rchan_buf *buf,
					      int *is_global)
{
	return debugfs_create_file(filename, mode, parent, buf,
				   &relay_file_operations);
}

static int remove_buf_file_handler(struct dentry *dentry)
{
	debugfs_remove(dentry);
	return 0;
}

static struct rchan_callbacks relay_callbacks = {
	.create_buf_file = create_buf_file_handler,
	.remove_buf_file = remove_buf_file_handler,
};

static int relay_init(void)
{
	dir = debugfs_create_dir("relay_test", NULL);
	if (!dir)
		goto err;

	chan = relay_open("cpu", dir, subbuf_size, n_subbufs, &relay_callbacks, NULL);
	if (!chan) {
		debugfs_remove(dir);
		goto err;
	}
	
	relay_write(chan, "12345678", 9);

	return 0;
err:
	return -EFAULT;
}
	
static void relay_exit(void)
{
	relay_close(chan);
	debugfs_remove(dir);
}

module_init(relay_init);
module_exit(relay_exit);


========
Program relay_read:

#include <stdio.h>
#include <fcntl.h>
#include <sys/stat.h>
#include <unistd.h>  
#include <sys/types.h>  

int main(void)
{
	int pa[2], pb[2];
	char buf[16], buf_r[16] = {0};
	pid_t pid;

	if (pipe(pa) || pipe(pb))
		return -1;

	if ((pid = fork()) < 0) {
		return -2;
	} else if (pid > 0) { /* process A */
		int relay_fd = open("/sys/kernel/debug/relay_test/cpu0", O_RDONLY);
		if (relay_fd < 0)
			return -1;
		close(pa[0]);
		close(pb[1]);

		/* expect to read "12" */
		read(relay_fd, buf_r, 2);
		printf("process A: read from relay %s\n", buf_r);

		write(pa[1], "w", 2); /* wake up process B */
		read(pb[0], buf, 256); /* wait */

		/* BUG: expect to read "56"
		 * actually read "34" which is already consumed
		 */
		read(relay_fd, buf_r, 2);
		printf("process A: read from relay %s\n", buf_r);

	} else { /* process B */
		int relay_fd = open("/sys/kernel/debug/relay_test/cpu0", O_RDONLY);
		if (relay_fd < 0)
			return -1;
		close(pa[1]);
		close(pb[0]);

		read(pa[0], buf, 256); /* wait */

		/* expect to read "34" */
		read(relay_fd, buf_r, 2);
		printf("process B: read from relay %s\n", buf_r);

		write(pb[1], "w", 2); /* wake up process A */
	}

	return 0;
}


========
Here is runing the test:

# taskset -c 0 insmod relay_test.ko
# ./relay_read 
process A: read from relay 12
process B: read from relay 34
process A: read from relay 34
# cat /sys/kernel/debug/relay_test/cpu0
78