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

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

[Pengcheng Yang]

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.

Signed-off-by: Pengcheng Yang <yangpc@wangsu.com>
---
 kernel/relay.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/kernel/relay.c b/kernel/relay.c
index ade14fb..07ee1a7 100644
--- a/kernel/relay.c
+++ b/kernel/relay.c
@@ -991,14 +991,14 @@ static void relay_file_read_consume(struct rchan_buf *buf,
 /*
  *	relay_file_read_avail - boolean, are there unconsumed bytes available?
  */
-static int relay_file_read_avail(struct rchan_buf *buf, size_t read_pos)
+static int relay_file_read_avail(struct rchan_buf *buf)
 {
 	size_t subbuf_size = buf->chan->subbuf_size;
 	size_t n_subbufs = buf->chan->n_subbufs;
 	size_t produced = buf->subbufs_produced;
 	size_t consumed = buf->subbufs_consumed;
 
-	relay_file_read_consume(buf, read_pos, 0);
+	relay_file_read_consume(buf, 0, 0);
 
 	consumed = buf->subbufs_consumed;
 
@@ -1059,23 +1059,20 @@ static size_t relay_file_read_subbuf_avail(size_t read_pos,
 
 /**
  *	relay_file_read_start_pos - find the first available byte to read
- *	@read_pos: file read position
  *	@buf: relay channel buffer
  *
- *	If the @read_pos is in the middle of padding, return the
+ *	If the read_pos is in the middle of padding, return the
  *	position of the first actually available byte, otherwise
  *	return the original value.
  */
-static size_t relay_file_read_start_pos(size_t read_pos,
-					struct rchan_buf *buf)
+static size_t relay_file_read_start_pos(struct rchan_buf *buf)
 {
 	size_t read_subbuf, padding, padding_start, padding_end;
 	size_t subbuf_size = buf->chan->subbuf_size;
 	size_t n_subbufs = buf->chan->n_subbufs;
 	size_t consumed = buf->subbufs_consumed % n_subbufs;
+	size_t read_pos = consumed * subbuf_size + buf->bytes_consumed;
 
-	if (!read_pos)
-		read_pos = consumed * subbuf_size + buf->bytes_consumed;
 	read_subbuf = read_pos / subbuf_size;
 	padding = buf->padding[read_subbuf];
 	padding_start = (read_subbuf + 1) * subbuf_size - padding;
@@ -1131,10 +1128,10 @@ static ssize_t relay_file_read(struct file *filp,
 	do {
 		void *from;
 
-		if (!relay_file_read_avail(buf, *ppos))
+		if (!relay_file_read_avail(buf))
 			break;
 
-		read_start = relay_file_read_start_pos(*ppos, buf);
+		read_start = relay_file_read_start_pos(buf);
 		avail = relay_file_read_subbuf_avail(read_start, buf);
 		if (!avail)
 			break;
-- 
1.8.3.1

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

[Andrew Morton]

On Wed, 22 Jan 2020 19:06:15 +0800 Pengcheng Yang <yangpc@wangsu.com> 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.

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@linux-foundation.org> wrote:
> 
> On Wed, 22 Jan 2020 19:06:15 +0800 Pengcheng Yang <yangpc@wangsu.com> 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