Re: [Patch] mqueue: fix the bad code in sys_mq_open()

["André Goddard Rosa" <andre.goddard@gmail.com>]

Hi, Américo!

On Thu, Feb 25, 2010 at 10:40 AM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>
> Fix bad code in ipc/mqueue.c.
>
> Inspired by André Goddard Rosa's  patch.
>
> a) do_create() and do_open() should not release the resources which
>   are not acquired within themselves, it's their caller's work;
>
> b) Fix an fd leak;
>
> c) The goto label 'out_upsem' doesn't make any sense, rename to
>   'out_unlock';
>
> d) mntget() should be called before looking up dentry within
>   ->mnt->mnt_root;
>
> e) When dealing with failure case, we should release the resources
>   in a reversed order of acquiring, so reorder the goto labels;
>
> f) Remove some unused labels due to reorder.
>
> Also, this shrinks the binary by 60 bytes:
>
>   text       data     bss       dec        hex filename
>   7674       1684        8      9366      2496 ipc/mqueue.o.BEFORE
>   7614       1684        8      9306      245a ipc/mqueue.o.AFTER
>
>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> Cc: André Goddard Rosa <andre.goddard@gmail.com>
>
> ---
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index c79bd57..fdd09da 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -650,8 +650,6 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct dentry *dir,
>  out_drop_write:
>        mnt_drop_write(ipc_ns->mq_mnt);
>  out:
> -       dput(dentry);
> -       mntput(ipc_ns->mq_mnt);
>        return ERR_PTR(ret);
>  }
>
> @@ -664,17 +662,11 @@ static struct file *do_open(struct ipc_namespace *ipc_ns,
>        static const int oflag2acc[O_ACCMODE] = { MAY_READ, MAY_WRITE,
>                                                  MAY_READ | MAY_WRITE };
>
> -       if ((oflag & O_ACCMODE) == (O_RDWR | O_WRONLY)) {
> -               dput(dentry);
> -               mntput(ipc_ns->mq_mnt);
> +       if ((oflag & O_ACCMODE) == (O_RDWR | O_WRONLY))
>                return ERR_PTR(-EINVAL);
> -       }
>
> -       if (inode_permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE])) {
> -               dput(dentry);
> -               mntput(ipc_ns->mq_mnt);
> +       if (inode_permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE]))
>                return ERR_PTR(-EACCES);
> -       }
>
>        return dentry_open(dentry, ipc_ns->mq_mnt, oflag, cred);
>  }
> @@ -686,7 +678,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 +693,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 +723,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)

I have some questions below:

Inside do_open() and do_create() on mqueue.c, we call
dentry_open()/__dentry_open().

If dentry_open() fails, it'll automatically call:
        dput(dentry);
        mntput(mnt);

Then we'll go here and set "error":

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

And finally here:

>  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);
> +       }

Is it ok to call:
        dput(dentry);
        mntput(ipc_ns->mq_mnt);

multiple times?

I also do not see where you set "error" to "fd":
> -       return fd;
> +       return error;

Am I missing something?

Could you please base your patch on top of mine?

Thank you,
André