On Tue, 2020-02-25 at 19:56 -0800, Andrew Morton wrote:
> ------------------------------------------------------
> From: Vasily Averin <[email protected]>
> Subject: fs/seq_file.c: seq_read(): add info message about buggy .next functions
>
> Patch series "seq_file .next functions should increase position index".
>
> 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"
>
> Described problem is still actual. If you make lseek into middle of last
> output line following read will output end of last line and whole last
> line once again.
>
> $ dd if=/proc/swaps bs=1 # usual output
> Filename Type Size Used Priority
> /dev/dm-0 partition 4194812 97536 -2
> 104+0 records in
> 104+0 records out
> 104 bytes copied
>
> $ dd if=/proc/swaps bs=40 skip=1 # last line was generated twice
> dd: /proc/swaps: cannot skip to specified offset
> v/dm-0 partition 4194812 97536 -2
> /dev/dm-0 partition 4194812 97536 -2
> 3+1 records in
> 3+1 records out
> 131 bytes copied
>
> There are lot of other affected files, I've found 30+ including
> /proc/net/ip_tables_matches and /proc/sysvipc/*
>
> I've sent patches into maillists of affected subsystems already, this
> patch-set fixes the problem in files related to pstore, tracing, gcov,
> sysvipc and other subsystems processed via linux-kernel@ mailing list
> directly
>
> https://bugzilla.kernel.org/show_bug.cgi?id=206283
>
>
> This patch (of 4):
>
> Add debug code to seq_read() to detect missed or out-of-tree incorrect
> .next seq_file functions.
This patch spams the console like crazy while reading sysfs,
# dmesg | grep 'buggy seq_file' | wc -l
4204
[ 9505.321981] LTP: starting read_all_proc (read_all -d /proc -q -r 10)
[ 9508.222934] buggy seq_file .next function xt_match_seq_next [x_tables] did
not updated position index
[ 9508.223319] buggy seq_file .next function xt_match_seq_next [x_tables] did
not updated position index
[ 9508.223654] buggy seq_file .next function xt_match_seq_next [x_tables] did
not updated position index
[ 9508.223994] buggy seq_file .next function xt_match_seq_next [x_tables] did
not updated position index
[ 9508.224337] buggy seq_file .next function xt_match_seq_next [x_tables] did
not updated position index
...
>
> https://bugzilla.kernel.org/show_bug.cgi?id=206283
> Link: http://lkml.kernel.org/r/[email protected]
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: Vasily Averin <[email protected]>
> Cc: NeilBrown <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Davidlohr Bueso <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Manfred Spraul <[email protected]>
> Cc: Peter Oberparleiter <[email protected]>
> Cc: Waiman Long <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> fs/seq_file.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> --- a/fs/seq_file.c~seq_read-info-message-about-buggy-next-functions
> +++ a/fs/seq_file.c
> @@ -256,9 +256,12 @@ Fill:
> loff_t pos = m->index;
>
> p = m->op->next(m, p, &m->index);
> - if (pos == m->index)
> - /* Buggy ->next function */
> + if (pos == m->index) {
> + pr_info("buggy seq_file .next function %ps "
> + "did not updated position index\n",
> + m->op->next);
> m->index++;
> + }
> if (!p || IS_ERR(p)) {
> err = PTR_ERR(p);
> break;
> _
>
> Patches currently in -mm which might be from [email protected] are
>
> seq_read-info-message-about-buggy-next-functions.patch
> pstore_ftrace_seq_next-should-increase-position-index.patch
> gcov_seq_next-should-increase-position-index.patch
> sysvipc_find_ipc-should-increase-position-index.patch
>
On Mon, 2020-03-02 at 13:20 -0500, Qian Cai wrote:
> This patch spams the console like crazy while reading sysfs,
>
> # dmesg | grep 'buggy seq_file' | wc -l
> 4204
>
> [ 9505.321981] LTP: starting read_all_proc (read_all -d /proc -q -r 10)
> [ 9508.222934] buggy seq_file .next function xt_match_seq_next [x_tables] did
> not updated position index
> [ 9508.223319] buggy seq_file .next function xt_match_seq_next [x_tables] did
> not updated position index
> [ 9508.223654] buggy seq_file .next function xt_match_seq_next [x_tables] did
> not updated position index
> [ 9508.223994] buggy seq_file .next function xt_match_seq_next [x_tables] did
> not updated position index
> [ 9508.224337] buggy seq_file .next function xt_match_seq_next [x_tables] did
> not updated position index
> ...
>
>
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=206283
> > Link: http://lkml.kernel.org/r/[email protected]
> > Link: http://lkml.kernel.org/r/[email protected]
> > Signed-off-by: Vasily Averin <[email protected]>
> > Cc: NeilBrown <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Davidlohr Bueso <[email protected]>
> > Cc: Ingo Molnar <[email protected]>
> > Cc: Manfred Spraul <[email protected]>
> > Cc: Peter Oberparleiter <[email protected]>
> > Cc: Waiman Long <[email protected]>
> > Signed-off-by: Andrew Morton <[email protected]>
> > ---
> >
> > fs/seq_file.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > --- a/fs/seq_file.c~seq_read-info-message-about-buggy-next-functions
> > +++ a/fs/seq_file.c
> > @@ -256,9 +256,12 @@ Fill:
> > loff_t pos = m->index;
> >
> > p = m->op->next(m, p, &m->index);
> > - if (pos == m->index)
> > - /* Buggy ->next function */
> > + if (pos == m->index) {
> > + pr_info("buggy seq_file .next function %ps "
> > + "did not updated position index\n",
> > + m->op->next);
This?
s/pr_info/pr_info_ratelimited/
> > m->index++;
> > + }
> > if (!p || IS_ERR(p)) {
> > err = PTR_ERR(p);
> > break;
> > _
> >
> > Patches currently in -mm which might be from [email protected] are
> >
> > seq_read-info-message-about-buggy-next-functions.patch
> > pstore_ftrace_seq_next-should-increase-position-index.patch
> > gcov_seq_next-should-increase-position-index.patch
> > sysvipc_find_ipc-should-increase-position-index.patch
> >
On Mon, 02 Mar 2020 14:31:48 -0500 Qian Cai <[email protected]> wrote:
> On Mon, 2020-03-02 at 13:20 -0500, Qian Cai wrote:
> > This patch spams the console like crazy while reading sysfs,
> >
> > # dmesg | grep 'buggy seq_file' | wc -l
> > 4204
> >
> > [ 9505.321981] LTP: starting read_all_proc (read_all -d /proc -q -r 10)
> > [ 9508.222934] buggy seq_file .next function xt_match_seq_next [x_tables] did
> > not updated position index
> > [ 9508.223319] buggy seq_file .next function xt_match_seq_next [x_tables] did
> > not updated position index
> > [ 9508.223654] buggy seq_file .next function xt_match_seq_next [x_tables] did
> > not updated position index
> > [ 9508.223994] buggy seq_file .next function xt_match_seq_next [x_tables] did
> > not updated position index
> > [ 9508.224337] buggy seq_file .next function xt_match_seq_next [x_tables] did
> > not updated position index
> > ...
> >
> >
> > > --- a/fs/seq_file.c~seq_read-info-message-about-buggy-next-functions
> > > +++ a/fs/seq_file.c
> > > @@ -256,9 +256,12 @@ Fill:
> > > loff_t pos = m->index;
> > >
> > > p = m->op->next(m, p, &m->index);
> > > - if (pos == m->index)
> > > - /* Buggy ->next function */
> > > + if (pos == m->index) {
> > > + pr_info("buggy seq_file .next function %ps "
> > > + "did not updated position index\n",
> > > + m->op->next);
>
> This?
>
> s/pr_info/pr_info_ratelimited/
>
Fair enough - I made that change.
On 3/2/20 11:42 PM, Andrew Morton wrote:
> On Mon, 02 Mar 2020 14:31:48 -0500 Qian Cai <[email protected]> wrote:
>
>> On Mon, 2020-03-02 at 13:20 -0500, Qian Cai wrote:
>>> This patch spams the console like crazy while reading sysfs,
>>>
>>> # dmesg | grep 'buggy seq_file' | wc -l
>>> 4204
>>>
>>> [ 9505.321981] LTP: starting read_all_proc (read_all -d /proc -q -r 10)
>>> [ 9508.222934] buggy seq_file .next function xt_match_seq_next [x_tables] did
>>> not updated position index
>>> [ 9508.223319] buggy seq_file .next function xt_match_seq_next [x_tables] did
>>> not updated position index
>>> [ 9508.223654] buggy seq_file .next function xt_match_seq_next [x_tables] did
>>> not updated position index
>>> [ 9508.223994] buggy seq_file .next function xt_match_seq_next [x_tables] did
>>> not updated position index
>>> [ 9508.224337] buggy seq_file .next function xt_match_seq_next [x_tables] did
>>> not updated position index
It should be fixed by following patch-set submitted to Netfilter-Devel mailing list
[PATCH v2 0/4] netfilter: seq_file .next functions should increase position index
https://lore.kernel.org/netfilter-devel/[email protected]/T/
>>>> --- a/fs/seq_file.c~seq_read-info-message-about-buggy-next-functions
>>>> +++ a/fs/seq_file.c
>>>> @@ -256,9 +256,12 @@ Fill:
>>>> loff_t pos = m->index;
>>>>
>>>> p = m->op->next(m, p, &m->index);
>>>> - if (pos == m->index)
>>>> - /* Buggy ->next function */
>>>> + if (pos == m->index) {
>>>> + pr_info("buggy seq_file .next function %ps "
>>>> + "did not updated position index\n",
>>>> + m->op->next);
>>
>> This?
>>
>> s/pr_info/pr_info_ratelimited/
>>
>
> Fair enough - I made that change.
>