lkml.org 
[lkml]   [2010]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    SubjectRe: [PATCH 6/6] mqueue: fix mq_open() file descriptor leak on user-space processes
    From
    On Thu, Feb 25, 2010 at 12:25 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
    > On Thu, Feb 25, 2010 at 12:00 PM, Xiaotian Feng <xtfeng@gmail.com> wrote:
    >> 2010/2/25 Américo Wang <xiyou.wangcong@gmail.com>:
    >>> On Tue, Feb 23, 2010 at 3:04 PM, André Goddard Rosa
    >>> <andre.goddard@gmail.com> wrote:
    >>>> It can be triggered by the following test program:
    >>>>
    >>>
    >>> <snip>
    >>>
    >>>>
    >>>> When not running valgrind, user-space program segfaults trying to execute
    >>>> strerror(errno). With valgrind, it executes successfully and prints the
    >>>> 5 open files: stdin, stdout, stderr, pipe[0] and pipe[1].
    >>>>
    >>>> Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
    >>>> ---
    >>>
    >>> The code has more than just this problem, could you please try
    >>> my patch below?
    >>>
    >>> Thanks.
    >>>
    >>> ---------------------------->
    >>>
    >>> Clean up the failure path of sys_mq_open().
    >>>
    >>> Reorder the goto labels;
    >>> Rename 'upsem' to 'upunlock';
    >>> Remove some unused labels;
    >>> Fix some wrong goto path.
    >>>
    >>
    >> I think it's wrong to move dput after mntput
    >
    > Oh, this is to say mntget() should be called before lookup_one_len(),
    > the original code seems wrong again...
    >

    How about the one below?
    ---------

    Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
    diff --git a/ipc/mqueue.c b/ipc/mqueue.c
    index c79bd57..6824651 100644
    --- a/ipc/mqueue.c
    +++ b/ipc/mqueue.c
    @@ -686,7 +686,7 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
    struct file *filp;
    char *name;
    struct mq_attr attr;
    - int fd, error;
    + int fd, error = 0;
    struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;

    if (u_attr && copy_from_user(&attr, u_attr, sizeof(struct mq_attr)))
    @@ -701,13 +701,13 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
    if (fd < 0)
    goto out_putname;

    + mntget(ipc_ns->mq_mnt);
    mutex_lock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
    dentry = lookup_one_len(name, ipc_ns->mq_mnt->mnt_root, strlen(name));
    if (IS_ERR(dentry)) {
    error = PTR_ERR(dentry);
    - goto out_err;
    + goto out_unlock;
    }
    - mntget(ipc_ns->mq_mnt);

    if (oflag & O_CREAT) {
    if (dentry->d_inode) { /* entry already exists */
    @@ -731,24 +731,23 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,

    if (IS_ERR(filp)) {
    error = PTR_ERR(filp);
    - goto out_putfd;
    + goto out;
    }

    fd_install(fd, filp);
    - goto out_upsem;
    + goto out_unlock;

    out:
    dput(dentry);
    - mntput(ipc_ns->mq_mnt);
    -out_putfd:
    - put_unused_fd(fd);
    -out_err:
    - fd = error;
    -out_upsem:
    +out_unlock:
    mutex_unlock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
    + if (error) {
    + mntput(ipc_ns->mq_mnt);
    + put_unused_fd(fd);
    + }
    out_putname:
    putname(name);
    - return fd;
    + return error;
    }

    SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
    \
     
     \ /
      Last update: 2010-02-25 08:01    [W:0.029 / U:1.836 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site