Re: [PATCH 6/6] mqueue: fix mq_open() file descriptor leak on user-space processes

[AmÃrico Wang]

On Thu, Feb 25, 2010 at 12:25 PM, AmÃrico Wang <xiyou.wangcong@xxxxxxxxx> wrote:
> On Thu, Feb 25, 2010 at 12:00 PM, Xiaotian Feng <xtfeng@xxxxxxxxx> wrote:
>> 2010/2/25 AmÃrico Wang <xiyou.wangcong@xxxxxxxxx>:
>>> On Tue, Feb 23, 2010 at 3:04 PM, Andrà Goddard Rosa
>>> <andre.goddard@xxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>
>>> 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@xxxxxxxxx>
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)