* [PATCH RESEND] kernel/relay.c: fix read_pos error when multiple readers
@ 2020-01-22 11:06 Pengcheng Yang
2020-01-24 1:23 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Pengcheng Yang @ 2020-01-22 11:06 UTC (permalink / raw)
To: akpm, gregkh, jannh, viro; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] kernel/relay.c: fix read_pos error when multiple readers
2020-01-22 11:06 [PATCH RESEND] kernel/relay.c: fix read_pos error when multiple readers Pengcheng Yang
@ 2020-01-24 1:23 ` Andrew Morton
2020-03-20 11:04 ` Pengcheng Yang
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Morton @ 2020-01-24 1:23 UTC (permalink / raw)
To: Pengcheng Yang; +Cc: gregkh, jannh, viro, linux-kernel
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] kernel/relay.c: fix read_pos error when multiple readers
2020-01-24 1:23 ` Andrew Morton
@ 2020-03-20 11:04 ` Pengcheng Yang
0 siblings, 0 replies; 3+ messages in thread
From: Pengcheng Yang @ 2020-03-20 11:04 UTC (permalink / raw)
To: 'Andrew Morton'; +Cc: gregkh, jannh, viro, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-20 11:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-22 11:06 [PATCH RESEND] kernel/relay.c: fix read_pos error when multiple readers Pengcheng Yang
2020-01-24 1:23 ` Andrew Morton
2020-03-20 11:04 ` Pengcheng Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).