[PATCH 1/1] Fix seq_file mishandling of consecutive pread() invocations.

[PATCH 1/1] Fix seq_file mishandling of consecutive pread() invocations.

[Earl Chew]

The following program illustrates the problem:

    char buf[8192];

    int fd = open("/proc/self/maps", O_RDONLY);

    n = pread(fd, buf, sizeof(buf), 0);
    printf("%d\n", n);

    /* lseek(fd, 0, SEEK_CUR); */ /* Uncomment to work around */

    n = pread(fd, buf, sizeof(buf), 0);
    printf("%d\n", n);

The second printf() prints zero, but uncommenting the lseek()
corrects the behaviour of seq_file.

To fix, make seq_read() mirror seq_lseek() when processing changes in *ppos.
Restore m->version first, then if required traverse and update read_pos
on success.

Signed-off-by: Earl Chew <echew@ixiacom.com>
---
 fs/seq_file.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index dba43c3..7a45306 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -140,9 +140,21 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 
 	mutex_lock(&m->lock);
 
+	/*
+	 * seq_file->op->..m_start/m_stop/m_next may do special actions
+	 * or optimisations based on the file->f_version, so we want to
+	 * pass the file->f_version to those methods.
+	 *
+	 * seq_file->version is just copy of f_version, and seq_file
+	 * methods can treat it simply as file version.
+	 * It is copied in first and copied out after all operations.
+	 * It is convenient to have it as  part of structure to avoid the
+	 * need of passing another argument to all the seq_file methods.
+	 */
+	m->version = file->f_version;
+
 	/* Don't assume *ppos is where we left it */
 	if (unlikely(*ppos != m->read_pos)) {
-		m->read_pos = *ppos;
 		while ((err = traverse(m, *ppos)) == -EAGAIN)
 			;
 		if (err) {
@@ -152,21 +164,11 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 			m->index = 0;
 			m->count = 0;
 			goto Done;
+		} else {
+			m->read_pos = *ppos;
 		}
 	}
 
-	/*
-	 * seq_file->op->..m_start/m_stop/m_next may do special actions
-	 * or optimisations based on the file->f_version, so we want to
-	 * pass the file->f_version to those methods.
-	 *
-	 * seq_file->version is just copy of f_version, and seq_file
-	 * methods can treat it simply as file version.
-	 * It is copied in first and copied out after all operations.
-	 * It is convenient to have it as  part of structure to avoid the
-	 * need of passing another argument to all the seq_file methods.
-	 */
-	m->version = file->f_version;
 	/* grab buffer if we didn't have one */
 	if (!m->buf) {
 		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
-- 
1.7.0.4

Re: [PATCH 1/1] Fix seq_file mishandling of consecutive pread() invocations.

[Earl Chew]

[ Added Maintainers; Added reference to bugzilla.kernel.org in commit log ]

Also reported in:

    https://bugzilla.kernel.org/show_bug.cgi?id=11856

The following program illustrates the problem:

    char buf[8192];

    int fd = open("/proc/self/maps", O_RDONLY);

    n = pread(fd, buf, sizeof(buf), 0);
    printf("%d\n", n);

    /* lseek(fd, 0, SEEK_CUR); */ /* Uncomment to work around */

    n = pread(fd, buf, sizeof(buf), 0);
    printf("%d\n", n);

The second printf() prints zero, but uncommenting the lseek()
corrects its behaviour.

To fix, make seq_read() mirror seq_lseek() when processing changes in *ppos.
Restore m->version first, then if required traverse and update read_pos
on success.

Signed-off-by: Earl Chew <echew@ixiacom.com>
---
 fs/seq_file.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index dba43c3..7a45306 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -140,9 +140,21 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 
 	mutex_lock(&m->lock);
 
+	/*
+	 * seq_file->op->..m_start/m_stop/m_next may do special actions
+	 * or optimisations based on the file->f_version, so we want to
+	 * pass the file->f_version to those methods.
+	 *
+	 * seq_file->version is just copy of f_version, and seq_file
+	 * methods can treat it simply as file version.
+	 * It is copied in first and copied out after all operations.
+	 * It is convenient to have it as  part of structure to avoid the
+	 * need of passing another argument to all the seq_file methods.
+	 */
+	m->version = file->f_version;
+
 	/* Don't assume *ppos is where we left it */
 	if (unlikely(*ppos != m->read_pos)) {
-		m->read_pos = *ppos;
 		while ((err = traverse(m, *ppos)) == -EAGAIN)
 			;
 		if (err) {
@@ -152,21 +164,11 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 			m->index = 0;
 			m->count = 0;
 			goto Done;
+		} else {
+			m->read_pos = *ppos;
 		}
 	}
 
-	/*
-	 * seq_file->op->..m_start/m_stop/m_next may do special actions
-	 * or optimisations based on the file->f_version, so we want to
-	 * pass the file->f_version to those methods.
-	 *
-	 * seq_file->version is just copy of f_version, and seq_file
-	 * methods can treat it simply as file version.
-	 * It is copied in first and copied out after all operations.
-	 * It is convenient to have it as  part of structure to avoid the
-	 * need of passing another argument to all the seq_file methods.
-	 */
-	m->version = file->f_version;
 	/* grab buffer if we didn't have one */
 	if (!m->buf) {
 		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
-- 
1.7.0.4

Re: [PATCH 1/1] Fix seq_file mishandling of consecutive pread() invocations.

[Andrew Morton]

On Sun, 22 Jan 2012 11:01:21 -0800
Earl Chew <echew@ixiacom.com> wrote:

> [ Added Maintainers; Added reference to bugzilla.kernel.org in commit log ]
> 
> Also reported in:
> 
>     https://bugzilla.kernel.org/show_bug.cgi?id=11856
> 
> The following program illustrates the problem:
> 
>     char buf[8192];
> 
>     int fd = open("/proc/self/maps", O_RDONLY);
> 
>     n = pread(fd, buf, sizeof(buf), 0);
>     printf("%d\n", n);
> 
>     /* lseek(fd, 0, SEEK_CUR); */ /* Uncomment to work around */
> 
>     n = pread(fd, buf, sizeof(buf), 0);
>     printf("%d\n", n);
> 
> The second printf() prints zero, but uncommenting the lseek()
> corrects its behaviour.

I'm stunned and confused.  That sequence of operations is the only sane
way in which to poll the contents of a procfs file.

Surely there are many applications which open a procfs file then
repeatedly read it with pread(fd, ..., 0).  How can this problem not
have been noticed in the first five minutes??