[PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output - Kernel

This is a discussion on [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output - Kernel ; When a read() requests an amount of data smaller than the amount of data that the seq_file's foo_show() outputs, the output starts looping and outputs the "stuck" element's data infinitely. There may be multiple sequential calls to foo_start(), foo_next()/foo_show(), and ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

  1. [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output


    When a read() requests an amount of data smaller than the amount of data
    that the seq_file's foo_show() outputs, the output starts looping and
    outputs the "stuck" element's data infinitely. There may be multiple
    sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
    for a single open with sequential read of the file. The _start() does not
    have to start with the 0th element and _show() might be called multiple
    times in a row for the same element for a given open/read of the seq_file.

    Signed-off-by: Tim Pepper
    Cc: Peter Zijlstra
    Cc: Ingo Molnar

    ---

    Assuming people are fine with this, it should probably find its way
    to stable.

    If you haven't seen the infinite output: it's easy to trigger with a
    simple 'cat /proc/lockdep' generally for me, a cat /proc/lock_stat piped
    to a file or for either of them a dd with the default bs=512 (or smaller)
    should do the job also.

    With this change to the lock_stat handler the data->iter member no longer
    attempts to hold state across calls, so it could be taken out of the
    lock_stat_seq struct and replace by a local variable in each function
    but that isn't a clear win to me so I just left it.

    --- linux-2.6.23-rc9.orig/kernel/lockdep_proc.c
    +++ linux-2.6.23-rc9/kernel/lockdep_proc.c
    @@ -34,19 +34,23 @@ static void *l_next(struct seq_file *m,
    lock_entry);
    else
    class = NULL;
    - m->private = class;

    return class;
    }

    static void *l_start(struct seq_file *m, loff_t *pos)
    {
    - struct lock_class *class = m->private;
    + struct lock_class *class;
    + loff_t i = 0;

    - if (&class->lock_entry == all_lock_classes.next)
    + if (*pos == 0)
    seq_printf(m, "all lock classes:\n");

    + list_for_each_entry(class, &all_lock_classes, lock_entry) {
    + if (i++ == *pos)
    + return class;
    + }
    + return NULL;
    - return class;
    }

    static void l_stop(struct seq_file *m, void *v)
    @@ -101,7 +105,7 @@ static void print_name(struct seq_file *
    static int l_show(struct seq_file *m, void *v)
    {
    unsigned long nr_forward_deps, nr_backward_deps;
    - struct lock_class *class = m->private;
    + struct lock_class *class = v;
    struct lock_list *entry;
    char c1, c2, c3, c4;

    @@ -523,12 +527,15 @@ static void *ls_start(struct seq_file *m
    {
    struct lock_stat_seq *data = m->private;

    - if (data->iter == data->stats)
    - seq_header(m);
    + data->iter = data->stats;
    + data->iter += *pos;

    - if (data->iter == data->iter_end)
    + if (data->iter >= data->iter_end)
    data->iter = NULL;

    + if (data->iter == data->stats)
    + seq_header(m);
    +
    return data->iter;
    }

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

    On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
    >
    > When a read() requests an amount of data smaller than the amount of data
    > that the seq_file's foo_show() outputs, the output starts looping and
    > outputs the "stuck" element's data infinitely. There may be multiple
    > sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
    > for a single open with sequential read of the file. The _start() does not
    > have to start with the 0th element and _show() might be called multiple
    > times in a row for the same element for a given open/read of the seq_file.
    >
    > static void *l_start(struct seq_file *m, loff_t *pos)
    > {
    > - struct lock_class *class = m->private;
    > + struct lock_class *class;
    > + loff_t i = 0;
    >
    > - if (&class->lock_entry == all_lock_classes.next)
    > + if (*pos == 0)
    > seq_printf(m, "all lock classes:\n");


    Do not generate output outside of ->show() and you won't have these
    problems. That's where your infinite output crap comes from.

    IOW, NAK - fix the underlying problem.
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

    On Tue 09 Oct at 02:30:11 +0100 viro@ftp.linux.org.uk said:
    > On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
    > >
    > > - if (&class->lock_entry == all_lock_classes.next)
    > > + if (*pos == 0)
    > > seq_printf(m, "all lock classes:\n");

    >
    > Do not generate output outside of ->show() and you won't have these
    > problems. That's where your infinite output crap comes from.
    >
    > IOW, NAK - fix the underlying problem.


    Aaah...OK. Can we add something like the following then:




    Document that output must only come from _show() and SEQ_START_TOKEN is how
    a _start() indicates a header is to be printed.

    Signed-off-by: Tim Pepper
    Cc: Al Viro

    ---

    --- linux-2.6.orig/include/linux/seq_file.h
    +++ linux-2.6.23-rc9/include/linux/seq_file.h
    @@ -36,9 +36,10 @@ ssize_t seq_read(struct file *, char __u
    loff_t seq_lseek(struct file *, loff_t, int);
    int seq_release(struct inode *, struct file *);
    int seq_escape(struct seq_file *, const char *, const char *);
    +
    +/* these may only be called from a (*show) function */
    int seq_putc(struct seq_file *m, char c);
    int seq_puts(struct seq_file *m, const char *s);
    -
    int seq_printf(struct seq_file *, const char *, ...)
    __attribute__ ((format (printf,2,3)));

    @@ -48,6 +49,11 @@ int single_open(struct file *, int (*)(s
    int single_release(struct inode *, struct file *);
    int seq_release_private(struct inode *, struct file *);

    +/*
    + * return SEQ_START_TOKEN in your (*start) function and test for
    + * (v == SEQ_START_TOKEN) in * your (*show) funtion in order to
    + * print a header before your seq data
    + */
    #define SEQ_START_TOKEN ((void *)1)

    /*
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

    On Tue, 2007-10-09 at 02:30 +0100, Al Viro wrote:
    > On Mon, Oct 08, 2007 at 06:15:51PM -0700, Tim Pepper wrote:
    > >
    > > When a read() requests an amount of data smaller than the amount of data
    > > that the seq_file's foo_show() outputs, the output starts looping and
    > > outputs the "stuck" element's data infinitely. There may be multiple
    > > sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
    > > for a single open with sequential read of the file. The _start() does not
    > > have to start with the 0th element and _show() might be called multiple
    > > times in a row for the same element for a given open/read of the seq_file.
    > >
    > > static void *l_start(struct seq_file *m, loff_t *pos)
    > > {
    > > - struct lock_class *class = m->private;
    > > + struct lock_class *class;
    > > + loff_t i = 0;
    > >
    > > - if (&class->lock_entry == all_lock_classes.next)
    > > + if (*pos == 0)
    > > seq_printf(m, "all lock classes:\n");

    >
    > Do not generate output outside of ->show() and you won't have these
    > problems. That's where your infinite output crap comes from.
    >
    > IOW, NAK - fix the underlying problem.



    FWIW I had to do Tim's bits too. Just moving all output from the start
    into the show method didn't fix it.

    Signed-off-by: Tim Pepper
    Signed-off-by: Peter Zijlstra
    ---
    kernel/lockdep_proc.c | 69 +++++++++++++++++++++++---------------------------
    1 file changed, 33 insertions(+), 36 deletions(-)

    Index: linux-2.6/kernel/lockdep_proc.c
    ================================================== =================
    --- linux-2.6.orig/kernel/lockdep_proc.c
    +++ linux-2.6/kernel/lockdep_proc.c
    @@ -23,32 +23,25 @@

    #include "lockdep_internals.h"

    -static void *l_next(struct seq_file *m, void *v, loff_t *pos)
    +static void *l_start(struct seq_file *m, loff_t *pos)
    {
    - struct lock_class *class = v;
    -
    - (*pos)++;
    -
    - if (class->lock_entry.next != &all_lock_classes)
    - class = list_entry(class->lock_entry.next, struct lock_class,
    - lock_entry);
    - else
    - class = NULL;
    - m->private = class;
    + struct lock_class *class;
    + int i = 0;

    - return class;
    + list_for_each_entry(class, &all_lock_classes, lock_entry) {
    + if (i++ == *pos)
    + return class;
    + }
    + return NULL;
    }

    -static void *l_start(struct seq_file *m, loff_t *pos)
    +static void *l_next(struct seq_file *m, void *v, loff_t *pos)
    {
    - struct lock_class *class = m->private;
    -
    - if (&class->lock_entry == all_lock_classes.next)
    - seq_printf(m, "all lock classes:\n");
    -
    - return class;
    + (*pos)++;
    + return l_start(m, pos);
    }

    +
    static void l_stop(struct seq_file *m, void *v)
    {
    }
    @@ -101,10 +94,16 @@ static void print_name(struct seq_file *
    static int l_show(struct seq_file *m, void *v)
    {
    unsigned long nr_forward_deps, nr_backward_deps;
    - struct lock_class *class = m->private;
    + struct lock_class *class = v;
    struct lock_list *entry;
    char c1, c2, c3, c4;

    + if (WARN_ON(class == NULL))
    + return 0;
    +
    + if (&class->lock_entry == all_lock_classes.next)
    + seq_printf(m, "all lock classes:\n");
    +
    seq_printf(m, "%p", class->key);
    #ifdef CONFIG_DEBUG_LOCKDEP
    seq_printf(m, " OPS:%8ld", class->ops);
    @@ -522,28 +521,19 @@ static void seq_header(struct seq_file *
    static void *ls_start(struct seq_file *m, loff_t *pos)
    {
    struct lock_stat_seq *data = m->private;
    + struct lock_stat_data *iter;

    - if (data->iter == data->stats)
    - seq_header(m);
    -
    - if (data->iter == data->iter_end)
    - data->iter = NULL;
    + iter = data->iter + *pos;
    + if (iter >= data->iter_end)
    + iter = NULL;

    - return data->iter;
    + return iter;
    }

    static void *ls_next(struct seq_file *m, void *v, loff_t *pos)
    {
    - struct lock_stat_seq *data = m->private;
    -
    (*pos)++;
    -
    - data->iter = v;
    - data->iter++;
    - if (data->iter == data->iter_end)
    - data->iter = NULL;
    -
    - return data->iter;
    + return ls_start(m, pos);
    }

    static void ls_stop(struct seq_file *m, void *v)
    @@ -553,8 +543,15 @@ static void ls_stop(struct seq_file *m,
    static int ls_show(struct seq_file *m, void *v)
    {
    struct lock_stat_seq *data = m->private;
    + struct lock_stat_data *iter = v;
    +
    + if (WARN_ON(iter == NULL))
    + return 0;
    +
    + if (iter == data->iter)
    + seq_header(m);

    - seq_stats(m, data->iter);
    + seq_stats(m, iter);
    return 0;
    }



    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.6 (GNU/Linux)

    iD8DBQBHC2KpXA2jU0ANEf4RAhswAKCA/FyWorav5yS7yCIH431QLXV3agCcD/sC
    S67HxHB56erI7asFl2PgLmY=
    =GiCk
    -----END PGP SIGNATURE-----


  5. Re: [PATCH] lockdep: Avoid /proc/lockdep & lock_stat infinite output

    On Tue 09 Oct at 13:14:49 +0200 a.p.zijlstra@chello.nl said:
    > FWIW I had to do Tim's bits too. Just moving all output from the start
    > into the show method didn't fix it.


    Yes. The way the original lockdep_proc.c code was doing its pointers
    around its seq data was definitely wrong, regardless of the output
    outside of _show().

    > -static void *l_start(struct seq_file *m, loff_t *pos)
    > +static void *l_next(struct seq_file *m, void *v, loff_t *pos)
    > {
    > - struct lock_class *class = m->private;
    > -
    > - if (&class->lock_entry == all_lock_classes.next)
    > - seq_printf(m, "all lock classes:\n");
    > -
    > - return class;
    > + (*pos)++;
    > + return l_start(m, pos);
    > }


    Isn't this is going to make l_start/l_next() approach O(n^2) when they can
    be more like O(n)? It appears to be the case that _next() will always
    get a valid *v, so you can just step immediately to the next element.
    The _start() seems to be the only place where you'd actually need to
    search based on *pos.

    The below moves the headers out of the _start() functions, but by using
    SEQ_START_TOKEN (as appears to be the trend in other seq_file users)
    to differentiate. This means *pos==0 then is the header and *pos==1..n
    are the lock class elements 0..(n-1), which again appears to be what
    others are doing.

    ================================================== ==============

    Both /proc/lockdep and /proc/lock_stat output may loop infinitely.

    When a read() requests an amount of data smaller than the amount of data
    that the seq_file's foo_show() outputs, the output starts looping and
    outputs the "stuck" element's data infinitely. There may be multiple
    sequential calls to foo_start(), foo_next()/foo_show(), and foo_stop()
    for a single open with sequential read of the file. The _start() does not
    have to start with the 0th element and _show() might be called multiple
    times in a row for the same element for a given open/read of the seq_file.

    Also header output should not be happening in _start(). All output should
    be in _show(), which SEQ_START_TOKEN is meant to help. Having output in
    _start() may also negatively impact seq_file's seq_read() and traverse()
    accounting.

    Signed-off-by: Tim Pepper
    Cc: Peter Zijlstra
    Cc: Ingo Molnar
    Cc: Al Viro

    ---

    Compared to my previous version this now also has output only happening
    in _show(). Compared to Peter's version with output only in _show(),
    this is more efficient in its _next().


    --- linux-2.6.23-rc9.orig/kernel/lockdep_proc.c
    +++ linux-2.6.23-rc9/kernel/lockdep_proc.c
    @@ -25,28 +25,38 @@

    static void *l_next(struct seq_file *m, void *v, loff_t *pos)
    {
    - struct lock_class *class = v;
    + struct lock_class *class;

    (*pos)++;

    + if (v == SEQ_START_TOKEN)
    + class = m->private;
    + else {
    + class = v;
    +
    + if (class->lock_entry.next != &all_lock_classes)
    + class = list_entry(class->lock_entry.next,
    + struct lock_class, lock_entry);
    + else
    + class = NULL;
    + }
    - if (class->lock_entry.next != &all_lock_classes)
    - class = list_entry(class->lock_entry.next, struct lock_class,
    - lock_entry);
    - else
    - class = NULL;
    - m->private = class;

    return class;
    }

    static void *l_start(struct seq_file *m, loff_t *pos)
    {
    - struct lock_class *class = m->private;
    + struct lock_class *class;
    + loff_t i = 0;

    - if (&class->lock_entry == all_lock_classes.next)
    - seq_printf(m, "all lock classes:\n");
    + if (*pos == 0)
    + return SEQ_START_TOKEN;

    + list_for_each_entry(class, &all_lock_classes, lock_entry) {
    + if (++i == *pos)
    + return class;
    + }
    + return NULL;
    - return class;
    }

    static void l_stop(struct seq_file *m, void *v)
    @@ -101,10 +111,15 @@ static void print_name(struct seq_file *
    static int l_show(struct seq_file *m, void *v)
    {
    unsigned long nr_forward_deps, nr_backward_deps;
    - struct lock_class *class = m->private;
    + struct lock_class *class = v;
    struct lock_list *entry;
    char c1, c2, c3, c4;

    + if (v == SEQ_START_TOKEN) {
    + seq_printf(m, "all lock classes:\n");
    + return 0;
    + }
    +
    seq_printf(m, "%p", class->key);
    #ifdef CONFIG_DEBUG_LOCKDEP
    seq_printf(m, " OPS:%8ld", class->ops);
    @@ -523,10 +538,11 @@ static void *ls_start(struct seq_file *m
    {
    struct lock_stat_seq *data = m->private;

    - if (data->iter == data->stats)
    - seq_header(m);
    + if (*pos == 0)
    + return SEQ_START_TOKEN;

    - if (data->iter == data->iter_end)
    + data->iter = data->stats + *pos;
    + if (data->iter >= data->iter_end)
    data->iter = NULL;

    return data->iter;
    @@ -538,8 +554,13 @@ static void *ls_next(struct seq_file *m,

    (*pos)++;

    + if (v == SEQ_START_TOKEN)
    + data->iter = data->stats;
    + else {
    + data->iter = v;
    + data->iter++;
    + }
    +
    - data->iter = v;
    - data->iter++;
    if (data->iter == data->iter_end)
    data->iter = NULL;

    @@ -552,9 +573,11 @@ static void ls_stop(struct seq_file *m,

    static int ls_show(struct seq_file *m, void *v)
    {
    - struct lock_stat_seq *data = m->private;
    + if (v == SEQ_START_TOKEN)
    + seq_header(m);
    + else
    + seq_stats(m, v);

    - seq_stats(m, data->iter);
    return 0;
    }

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread