diff mbox series

seq_file: fix problem when seeking mid-record.

Message ID 87mugojl0f.fsf@notabene.neil.brown.name
State New, archived
Headers show
Series seq_file: fix problem when seeking mid-record. | expand

Commit Message

NeilBrown Aug. 5, 2019, 4:26 a.m. UTC
If you use lseek or similar (e.g. pread) to access
a location in a seq_file file that is within a record,
rather than at a record boundary, then the first read
will return the remainder of the record, and the second
read will return the whole of that same record (instead
of the next record).
Whnn seeking to a record boundary, the next record is
correctly returned.

This bug was introduced by a recent patch (identified below)
Before that patch, seq_read() would increment m->index when
the last of the buffer was returned (m->count == 0).
After that patch, we rely on ->next to increment m->index
after filling the buffer - but there was one place where that
didn't happen.

Link: https://lkml.kernel.org/lkml/877e7xl029.fsf@notabene.neil.brown.name/
Reported-by-tested-by: Sergei Turchanov <turchanov@farpost.com>
Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
	and interface")
Cc: stable@vger.kernel.org (v4.19+)
Signed-off-by: NeilBrown <neilb@suse.com>
---

Hi Andrew: as you applied the offending patch for me, maybe you could
queue up this fix too.
Thanks,
NeilBrown

 fs/seq_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Markus Elfring Aug. 5, 2019, 9:15 a.m. UTC | #1
> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
> 	and interface")

Please do not split this tag across multiple lines in the final commit description.

Regards,
Markus
NeilBrown Aug. 5, 2019, 10:08 a.m. UTC | #2
On Mon, Aug 05 2019, Markus Elfring wrote:

>> Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code
>> 	and interface")
>
> Please do not split this tag across multiple lines in the final commit description.

I tend to agree...
I had previously seen
     "Possible unwrapped commit description (prefer a maximum 75 chars per line)\n"
warnings from checkpatch, but one closer look that doesn't apply to
Fixes: lines (among other special cases).

Maybe Andrew will fix it up for me when it applies .... (please!)

Thanks,
NeilBrown
Andrew Morton Aug. 6, 2019, 10:42 p.m. UTC | #3
On Mon, 05 Aug 2019 14:26:08 +1000 NeilBrown <neilb@suse.com> wrote:

> If you use lseek or similar (e.g. pread) to access
> a location in a seq_file file that is within a record,
> rather than at a record boundary, then the first read
> will return the remainder of the record, and the second
> read will return the whole of that same record (instead
> of the next record).
> Whnn seeking to a record boundary, the next record is
> correctly returned.

ouch.  I'm surprised it took this long to be noticed.

Maybe we need a seqfile-basher in tools/testing/selftests/proc or
somewhere.
diff mbox series

Patch

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 04f09689cd6d..1600034a929b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -119,6 +119,7 @@  static int traverse(struct seq_file *m, loff_t offset)
 		}
 		if (seq_has_overflowed(m))
 			goto Eoverflow;
+		p = m->op->next(m, p, &m->index);
 		if (pos + m->count > offset) {
 			m->from = offset - pos;
 			m->count -= m->from;
@@ -126,7 +127,6 @@  static int traverse(struct seq_file *m, loff_t offset)
 		}
 		pos += m->count;
 		m->count = 0;
-		p = m->op->next(m, p, &m->index);
 		if (pos == offset)
 			break;
 	}