[PATCH] latencytop: fix kernel panic and memory leak on proc - Kernel

This is a discussion on [PATCH] latencytop: fix kernel panic and memory leak on proc - Kernel ; Hi, I posted 2 patches to fix kernel panic and memory leak. http://lkml.org/lkml/2008/2/14/282 http://lkml.org/lkml/2008/2/14/283 But, I think this patch is better than old ones. --- From: Hiroshi Shimamoto Reading /proc/ /latency or /proc/ /task/ /latency could cause NULL pointer dereference. ...

+ Reply to Thread
Results 1 to 5 of 5

Thread: [PATCH] latencytop: fix kernel panic and memory leak on proc

  1. [PATCH] latencytop: fix kernel panic and memory leak on proc

    Hi,

    I posted 2 patches to fix kernel panic and memory leak.
    http://lkml.org/lkml/2008/2/14/282
    http://lkml.org/lkml/2008/2/14/283

    But, I think this patch is better than old ones.

    ---
    From: Hiroshi Shimamoto

    Reading /proc//latency or /proc//task//latency could cause
    NULL pointer dereference.

    In lstats_open(), get_proc_task() can return NULL, in which case the kernel
    will oops at lstats_show_proc() because m->private is NULL.

    This can be reproduced by the follwoing script.
    while :
    do
    bash -c 'ls > ls.$$' &
    pid=$!
    cat /proc/$pid/latency &
    cat /proc/$pid/latency &
    cat /proc/$pid/latency &
    cat /proc/$pid/latency
    done

    And the task struct which gotten by get_proc_task() is never put.
    put_task_struct() should be called.

    This patch changes the private is used to store inode, and the task struct
    will be gotten and putted in read or write function.

    Signed-off-by: Hiroshi Shimamoto
    ---
    fs/proc/base.c | 27 +++++++++++----------------
    1 files changed, 11 insertions(+), 16 deletions(-)

    diff --git a/fs/proc/base.c b/fs/proc/base.c
    index 7c6b4ec..5de8dd5 100644
    --- a/fs/proc/base.c
    +++ b/fs/proc/base.c
    @@ -314,9 +314,12 @@ static int proc_pid_schedstat(struct task_struct *task, char *buffer)
    static int lstats_show_proc(struct seq_file *m, void *v)
    {
    int i;
    - struct task_struct *task = m->private;
    - seq_puts(m, "Latency Top version : v0.1\n");
    + struct inode *inode = m->private;
    + struct task_struct *task = get_proc_task(inode);

    + if (!task)
    + return -ESRCH;
    + seq_puts(m, "Latency Top version : v0.1\n");
    for (i = 0; i < 32; i++) {
    if (task->latency_record[i].backtrace[0]) {
    int q;
    @@ -341,32 +344,24 @@ static int lstats_show_proc(struct seq_file *m, void *v)
    }

    }
    + put_task_struct(task);
    return 0;
    }

    static int lstats_open(struct inode *inode, struct file *file)
    {
    - int ret;
    - struct seq_file *m;
    - struct task_struct *task = get_proc_task(inode);
    -
    - ret = single_open(file, lstats_show_proc, NULL);
    - if (!ret) {
    - m = file->private_data;
    - m->private = task;
    - }
    - return ret;
    + return single_open(file, lstats_show_proc, inode);
    }

    static ssize_t lstats_write(struct file *file, const char __user *buf,
    size_t count, loff_t *offs)
    {
    - struct seq_file *m;
    - struct task_struct *task;
    + struct task_struct *task = get_proc_task(file->f_dentry->d_inode);

    - m = file->private_data;
    - task = m->private;
    + if (!task)
    + return -ESRCH;
    clear_all_latency_tracing(task);
    + put_task_struct(task);

    return count;
    }
    --
    1.5.3.8

    --
    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] latencytop: fix kernel panic and memory leak on proc

    On Thu, 14 Feb 2008 14:51:19 -0800
    Hiroshi Shimamoto wrote:

    > Hi,
    >
    > I posted 2 patches to fix kernel panic and memory leak.
    > http://lkml.org/lkml/2008/2/14/282
    > http://lkml.org/lkml/2008/2/14/283
    >
    > But, I think this patch is better than old ones.
    >
    > ---
    > From: Hiroshi Shimamoto
    >
    > Reading /proc//latency or /proc//task//latency could
    > cause NULL pointer dereference.
    >
    > In lstats_open(), get_proc_task() can return NULL, in which case the
    > kernel will oops at lstats_show_proc() because m->private is NULL.
    >
    > This can be reproduced by the follwoing script.
    > while :
    > do
    > bash -c 'ls > ls.$$' &
    > pid=$!
    > cat /proc/$pid/latency &
    > cat /proc/$pid/latency &
    > cat /proc/$pid/latency &
    > cat /proc/$pid/latency
    > done
    >
    > And the task struct which gotten by get_proc_task() is never put.
    > put_task_struct() should be called.
    >
    > This patch changes the private is used to store inode, and the task
    > struct will be gotten and putted in read or write function.
    >
    > Signed-off-by: Hiroshi Shimamoto



    Fine with me; Ingo please merge
    Thanks for working on this!

    --
    If you want to reach me at my work email, use arjan@linux.intel.com
    For development, discussion and tips for power savings,
    visit http://www.lesswatts.org
    --
    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] latencytop: fix kernel panic and memory leak on proc


    * Arjan van de Ven wrote:

    > On Thu, 14 Feb 2008 14:51:19 -0800
    > Hiroshi Shimamoto wrote:
    >
    > > Hi,
    > >
    > > I posted 2 patches to fix kernel panic and memory leak.
    > > http://lkml.org/lkml/2008/2/14/282
    > > http://lkml.org/lkml/2008/2/14/283
    > >
    > > But, I think this patch is better than old ones.


    thanks Hiroshi, applied.

    Ingo
    --
    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] latencytop: fix kernel panic and memory leak on proc

    Ingo Molnar wrote:
    > * Arjan van de Ven wrote:
    >
    >> On Thu, 14 Feb 2008 14:51:19 -0800
    >> Hiroshi Shimamoto wrote:
    >>
    >>> Hi,
    >>>
    >>> I posted 2 patches to fix kernel panic and memory leak.
    >>> http://lkml.org/lkml/2008/2/14/282
    >>> http://lkml.org/lkml/2008/2/14/283
    >>>
    >>> But, I think this patch is better than old ones.

    >
    > thanks Hiroshi, applied.


    Hi Ingo,

    I'd like to be applied new patch for latencytop fix.
    I think it better than old ptaches.

    Can you apply this patch for latencytop issues?
    http://lkml.org/lkml/2008/2/14/451

    It's replacement of old patches you applied.
    It makes latency file behavior same as other proc files.
    get_proc_task() and put_task_struct() are called at read time,
    and returns -ESRCH if get_proc_task() failed.

    If is there any problem, please let me know.

    thanks,
    Hiroshi Shimamoto
    --
    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/

  5. [PATCH] latencytop: change /proc task_struct access method

    Hi Ingo,

    Here is a delta patch against sched-devel.git tree.

    These two patches in sched-devel.git tree fix the issues.
    latencytop: fix kernel panic while reading latency proc file
    latencytop: fix memory leak on latency proc file

    However, this is more appropriate way to fix, I think.

    ---
    From: Hiroshi Shimamoto

    Change getting task_struct by get_proc_task() at read or write time,
    and returns -ESRCH if get_proc_task() returns NULL.
    This is same behavior as other /proc files.

    Signed-off-by: Hiroshi Shimamoto
    ---
    fs/proc/base.c | 40 ++++++++++++----------------------------
    1 files changed, 12 insertions(+), 28 deletions(-)

    diff --git a/fs/proc/base.c b/fs/proc/base.c
    index 64661c3..bebf9a8 100644
    --- a/fs/proc/base.c
    +++ b/fs/proc/base.c
    @@ -314,9 +314,12 @@ static int proc_pid_schedstat(struct task_struct *task, char *buffer)
    static int lstats_show_proc(struct seq_file *m, void *v)
    {
    int i;
    - struct task_struct *task = m->private;
    - seq_puts(m, "Latency Top version : v0.1\n");
    + struct inode *inode = m->private;
    + struct task_struct *task = get_proc_task(inode);

    + if (!task)
    + return -ESRCH;
    + seq_puts(m, "Latency Top version : v0.1\n");
    for (i = 0; i < 32; i++) {
    if (task->latency_record[i].backtrace[0]) {
    int q;
    @@ -341,43 +344,24 @@ static int lstats_show_proc(struct seq_file *m, void *v)
    }

    }
    + put_task_struct(task);
    return 0;
    }

    static int lstats_open(struct inode *inode, struct file *file)
    {
    - int ret;
    - struct seq_file *m;
    - struct task_struct *task = get_proc_task(inode);
    -
    - if (!task)
    - return -ENOENT;
    - ret = single_open(file, lstats_show_proc, NULL);
    - if (!ret) {
    - m = file->private_data;
    - m->private = task;
    - }
    - return ret;
    -}
    -
    -static int lstats_release(struct inode *inode, struct file *file)
    -{
    - struct seq_file *m = file->private_data;
    - struct task_struct *task = m->private;
    -
    - put_task_struct(task);
    - return single_release(inode, file);
    + return single_open(file, lstats_show_proc, inode);
    }

    static ssize_t lstats_write(struct file *file, const char __user *buf,
    size_t count, loff_t *offs)
    {
    - struct seq_file *m;
    - struct task_struct *task;
    + struct task_struct *task = get_proc_task(file->f_dentry->d_inode);

    - m = file->private_data;
    - task = m->private;
    + if (!task)
    + return -ESRCH;
    clear_all_latency_tracing(task);
    + put_task_struct(task);

    return count;
    }
    @@ -387,7 +371,7 @@ static const struct file_operations proc_lstats_operations = {
    .read = seq_read,
    .write = lstats_write,
    .llseek = seq_lseek,
    - .release = lstats_release,
    + .release = single_release,
    };

    #endif
    --
    1.5.3.8

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