Messages in this thread | | | Date | Tue, 25 Feb 2020 11:39:48 -0800 | From | Kees Cook <> | Subject | Re: [PATCH v2] pstore: pstore_ftrace_seq_next should increase position index |
| |
[merged threads]
On Tue, Feb 25, 2020 at 11:11:20AM +0300, Vasily Averin wrote: > In Aug 2018 NeilBrown noticed > commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface") > "Some ->next functions do not increment *pos when they return NULL... > Note that such ->next functions are buggy and should be fixed. > A simple demonstration is > > dd if=/proc/swaps bs=1000 skip=1 > > Choose any block size larger than the size of /proc/swaps. This will > always show the whole last line of /proc/swaps" > > /proc/swaps output was fixed recently, however there are lot of other > affected files, and one of them is related to pstore subsystem. > > If .next function does not change position index, following .show function > will repeat output related to current position index. > > There are at least 2 related problems: > - read after lseek beyond end of file, described above by NeilBrown > "dd if=<AFFECTED_FILE> bs=1000 skip=1" will generate whole last list > - read after lseek on in middle of last line will output expected rest of > last line but then repeat whole last line once again. > > If .show() function generates multy-line output > (like pstore_ftrace_seq_show() does ?) > following bash script cycles endlessly > > $ q=;while read -r r;do echo "$((++q)) $r";done < AFFECTED_FILE > > Unfortunately I'm not familiar enough to pstore subsystem and was unable to > find affected pstore-related file on my test node. > > If .next function does not change position index, > following .show function will repeat output related > to current position index. > > Cc: stable@vger.kernel.org > Fixes: 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code ...") > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206283 > Signed-off-by: Vasily Averin <vvs@virtuozzo.com> > --- > fs/pstore/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c > index 7fbe8f0..ea8799b 100644 > --- a/fs/pstore/inode.c > +++ b/fs/pstore/inode.c > @@ -87,11 +87,11 @@ static void *pstore_ftrace_seq_next(struct seq_file *s, void *v, loff_t *pos) > struct pstore_private *ps = s->private; > struct pstore_ftrace_seq_data *data = v; > > + (*pos)++; > data->off += REC_SIZE; > if (data->off + REC_SIZE > ps->total_size) > return NULL; > > - (*pos)++; > return data; > } > > -- > 1.8.3.1 >
I think this make sense, but I figured I'd check with Joel first. Does this look sane for how ftrace will merge records?
-- Kees Cook
| |