seq_file: fix problem when seeking mid-record.
diff mbox series

Message ID 87mugojl0f.fsf@notabene.neil.brown.name
State Accepted
Commit 6a2aeab59e97101b4001bac84388fc49a992f87e
Headers show
Series
  • seq_file: fix problem when seeking mid-record.
Related show

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.

Patch
diff mbox series

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;
 	}