If .next function does not change position index,
following .show function will repeat output related
to current position index.
Cc: [email protected]
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 <[email protected]>
---
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
[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: [email protected]
> 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 <[email protected]>
> ---
> 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
On Tue, Feb 25, 2020 at 2:39 PM Kees Cook <[email protected]> wrote:
>
> [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: [email protected]
> > 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 <[email protected]>
> > ---
> > 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?
The merging of the per-cpu records is completed at boot time. The
above snip is related to reading the merged records and formatting
them. It makes sense.
One thing I was not sure about is, if we move "pos" forward but still
return NULL from next(), then does show() need to check if data is
NULL? As below. Otherwise the suggested patch looks sane to me.
diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
index 7fbe8f0582205..e3e7370b1a34d 100644
--- a/fs/pstore/inode.c
+++ b/fs/pstore/inode.c
@@ -101,6 +101,9 @@ static int pstore_ftrace_seq_show(struct seq_file
*s, void *v)
struct pstore_ftrace_seq_data *data = v;
struct pstore_ftrace_record *rec;
+ if (!data)
+ return 0;
+
rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %ps <- %pS\n",
On Wed, Feb 26, 2020 at 01:36:45PM -0500, Joel Fernandes wrote:
> One thing I was not sure about is, if we move "pos" forward but still
> return NULL from next(), then does show() need to check if data is
> NULL? As below. Otherwise the suggested patch looks sane to me.
>
> diff --git a/fs/pstore/inode.c b/fs/pstore/inode.c
> index 7fbe8f0582205..e3e7370b1a34d 100644
> --- a/fs/pstore/inode.c
> +++ b/fs/pstore/inode.c
> @@ -101,6 +101,9 @@ static int pstore_ftrace_seq_show(struct seq_file
> *s, void *v)
> struct pstore_ftrace_seq_data *data = v;
> struct pstore_ftrace_record *rec;
>
> + if (!data)
> + return 0;
> +
> rec = (struct pstore_ftrace_record *)(ps->record->buf + data->off);
>
> seq_printf(s, "CPU:%d ts:%llu %08lx %08lx %ps <- %pS\n",
Ah, good point. I'm not sure, but it's worth checking I think. :)
--
Kees Cook