Fix seq_file mishandling of consecutive pread() invocations.
PATCH 1/1 Fix seq_file mishandling of consecutive pread() invocations.
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.
--- fs/seq_file.c | 28 +++++++++++++++------------- 1 files changed, 15 insertions(+), 13 deletions(-)
fs/seq_file.c +15 -13
@@ -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 );
Signature · 1 line
1.7.0.4
[ 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.
--- fs/seq_file.c | 28 +++++++++++++++------------- 1 files changed, 15 insertions(+), 13 deletions(-)
fs/seq_file.c +15 -13
@@ -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 );
Signature · 1 line
1.7.0.4
Quoted 20 lines · On Sun, 22 Jan 2012 11:01:21 -0800 Earl Chew <[email protected]> wrote:
[ Added Maintainers; Added reference to bugzilla.kernel.org in commit log ]
Also reported in:
https://bugzilla.kernel.org/show_bug.cgi?id=11856The 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);
Quoted 2 lines
The second
printf()prints zero, but uncommenting thelseek()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??