[PATCH 0/6] Fix file descriptor leak on user-space processes and cleanup

[PATCH 0/6] Fix file descriptor leak on user-space processes and cleanup

[André Goddard Rosa]

Fix a file descriptor leak on user-space processes and perform a cleanup,
reducing the code size:
text    data     bss     dec     hex filename
9949      72      16   10037    2735 ipc/mqueue-BEFORE.o
9885      72      16    9973    26f5 ipc/mqueue-AFTER.o

André Goddard Rosa (6):
  mqueue: remove unneeded info->messages initialization
  mqueue: apply mathematics distributivity on mq_bytes calculation
  mqueue: simplify do_open() error handling
  mqueue: only set error codes if they are really necessary
  mqueue: fix typo "failues" -> "failures"
  mqueue: fix mq_open() file descriptor leak on user-space processes

[PATCH 1/6] mqueue: remove unneeded info->messages initialization

[André Goddard Rosa]

... and abort earlier if we couldn't allocate the message pointers array,
avoiding the u->mq_bytes accounting logic.

It reduces code size:
   text    data     bss     dec     hex filename
   9949      72      16   10037    2735 ipc/mqueue-BEFORE.o
   9941      72      16   10029    272d ipc/mqueue-AFTER.o

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 ipc/mqueue.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index c79bd57..2d76647 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -134,7 +134,6 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
 			init_waitqueue_head(&info->wait_q);
 			INIT_LIST_HEAD(&info->e_wait_q[0].list);
 			INIT_LIST_HEAD(&info->e_wait_q[1].list);
-			info->messages = NULL;
 			info->notify_owner = NULL;
 			info->qsize = 0;
 			info->user = NULL;	/* set when all is ok */
@@ -146,6 +145,10 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
 				info->attr.mq_msgsize = attr->mq_msgsize;
 			}
 			mq_msg_tblsz = info->attr.mq_maxmsg * sizeof(struct msg_msg *);
+			info->messages = kmalloc(mq_msg_tblsz, GFP_KERNEL);
+			if (!info->messages)
+				goto out_inode;
+
 			mq_bytes = (mq_msg_tblsz +
 				(info->attr.mq_maxmsg * info->attr.mq_msgsize));
 
@@ -154,18 +157,12 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
 		 	    u->mq_bytes + mq_bytes >
 			    p->signal->rlim[RLIMIT_MSGQUEUE].rlim_cur) {
 				spin_unlock(&mq_lock);
+				kfree(info->messages);
 				goto out_inode;
 			}
 			u->mq_bytes += mq_bytes;
 			spin_unlock(&mq_lock);
 
-			info->messages = kmalloc(mq_msg_tblsz, GFP_KERNEL);
-			if (!info->messages) {
-				spin_lock(&mq_lock);
-				u->mq_bytes -= mq_bytes;
-				spin_unlock(&mq_lock);
-				goto out_inode;
-			}
 			/* all is ok */
 			info->user = get_uid(u);
 		} else if (S_ISDIR(mode)) {
-- 
1.7.0.87.g0901d

[PATCH 2/6] mqueue: apply mathematics distributivity on mq_bytes calculation

[André Goddard Rosa]

Code size reduction:
   text    data     bss     dec     hex filename
   9941      72      16   10029    272d ipc/mqueue-BEFORE.o
   9925      72      16   10013    271d ipc/mqueue-AFTER.o

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 ipc/mqueue.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 2d76647..04403fd 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -261,8 +261,9 @@ static void mqueue_delete_inode(struct inode *inode)
 
 	clear_inode(inode);
 
-	mq_bytes = (info->attr.mq_maxmsg * sizeof(struct msg_msg *) +
-		   (info->attr.mq_maxmsg * info->attr.mq_msgsize));
+	/* Total amount of bytes accounted for the mqueue */
+	mq_bytes = info->attr.mq_maxmsg * (sizeof(struct msg_msg *)
+	    + info->attr.mq_msgsize);
 	user = info->user;
 	if (user) {
 		spin_lock(&mq_lock);
@@ -601,8 +602,8 @@ static int mq_attr_ok(struct ipc_namespace *ipc_ns, struct mq_attr *attr)
 	/* check for overflow */
 	if (attr->mq_msgsize > ULONG_MAX/attr->mq_maxmsg)
 		return 0;
-	if ((unsigned long)(attr->mq_maxmsg * attr->mq_msgsize) +
-	    (attr->mq_maxmsg * sizeof (struct msg_msg *)) <
+	if ((unsigned long)(attr->mq_maxmsg * (attr->mq_msgsize
+	    + sizeof (struct msg_msg *))) <
 	    (unsigned long)(attr->mq_maxmsg * attr->mq_msgsize))
 		return 0;
 	return 1;
-- 
1.7.0.87.g0901d

[PATCH 3/6] mqueue: simplify do_open() error handling

[André Goddard Rosa]

It reduces code size:
text    data     bss     dec     hex filename
9925      72      16   10013    271d ipc/mqueue-BEFORE.o
9885      72      16    9973    26f5 ipc/mqueue-AFTER.o

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 ipc/mqueue.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 04403fd..2cddf93 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -657,24 +657,28 @@ out:
 static struct file *do_open(struct ipc_namespace *ipc_ns,
 				struct dentry *dentry, int oflag)
 {
+	int ret;
 	const struct cred *cred = current_cred();
 
 	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);
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto err;
 	}
 
 	if (inode_permission(dentry->d_inode, oflag2acc[oflag & O_ACCMODE])) {
-		dput(dentry);
-		mntput(ipc_ns->mq_mnt);
-		return ERR_PTR(-EACCES);
+		ret = -EACCES;
+		goto err;
 	}
 
 	return dentry_open(dentry, ipc_ns->mq_mnt, oflag, cred);
+
+err:
+	dput(dentry);
+	mntput(ipc_ns->mq_mnt);
+	return ERR_PTR(ret);
 }
 
 SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
-- 
1.7.0.87.g0901d

[PATCH 4/6] mqueue: only set error codes if they are really necessary

[André Goddard Rosa]

... postponing assignments until they're needed. Doesn't change code size.

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 ipc/mqueue.c |   77 +++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 2cddf93..906e873 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -184,7 +184,7 @@ static int mqueue_fill_super(struct super_block *sb, void *data, int silent)
 {
 	struct inode *inode;
 	struct ipc_namespace *ns = data;
-	int error = 0;
+	int error;
 
 	sb->s_blocksize = PAGE_CACHE_SIZE;
 	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
@@ -202,7 +202,9 @@ static int mqueue_fill_super(struct super_block *sb, void *data, int silent)
 	if (!sb->s_root) {
 		iput(inode);
 		error = -ENOMEM;
+		goto out;
 	}
+	error = 0;
 
 out:
 	return error;
@@ -621,9 +623,10 @@ static struct file *do_create(struct ipc_namespace *ipc_ns, struct dentry *dir,
 	int ret;
 
 	if (attr) {
-		ret = -EINVAL;
-		if (!mq_attr_ok(ipc_ns, attr))
+		if (!mq_attr_ok(ipc_ns, attr)) {
+			ret = -EINVAL;
 			goto out;
+		}
 		/* store for use during create */
 		dentry->d_fsdata = attr;
 	}
@@ -714,9 +717,10 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 	if (oflag & O_CREAT) {
 		if (dentry->d_inode) {	/* entry already exists */
 			audit_inode(name, dentry);
-			error = -EEXIST;
-			if (oflag & O_EXCL)
+			if (oflag & O_EXCL) {
+				error = -EEXIST;
 				goto out;
+			}
 			filp = do_open(ipc_ns, dentry, oflag);
 		} else {
 			filp = do_create(ipc_ns, ipc_ns->mq_mnt->mnt_root,
@@ -724,9 +728,10 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 						u_attr ? &attr : NULL);
 		}
 	} else {
-		error = -ENOENT;
-		if (!dentry->d_inode)
+		if (!dentry->d_inode) {
+			error = -ENOENT;
 			goto out;
+		}
 		audit_inode(name, dentry);
 		filp = do_open(ipc_ns, dentry, oflag);
 	}
@@ -874,19 +879,24 @@ SYSCALL_DEFINE5(mq_timedsend, mqd_t, mqdes, const char __user *, u_msg_ptr,
 	audit_mq_sendrecv(mqdes, msg_len, msg_prio, p);
 	timeout = prepare_timeout(p);
 
-	ret = -EBADF;
 	filp = fget(mqdes);
-	if (unlikely(!filp))
+	if (unlikely(!filp)) {
+		ret = -EBADF;
 		goto out;
+	}
 
 	inode = filp->f_path.dentry->d_inode;
-	if (unlikely(filp->f_op != &mqueue_file_operations))
+	if (unlikely(filp->f_op != &mqueue_file_operations)) {
+		ret = -EBADF;
 		goto out_fput;
+	}
 	info = MQUEUE_I(inode);
 	audit_inode(NULL, filp->f_path.dentry);
 
-	if (unlikely(!(filp->f_mode & FMODE_WRITE)))
+	if (unlikely(!(filp->f_mode & FMODE_WRITE))) {
+		ret = -EBADF;
 		goto out_fput;
+	}
 
 	if (unlikely(msg_len > info->attr.mq_msgsize)) {
 		ret = -EMSGSIZE;
@@ -963,19 +973,24 @@ SYSCALL_DEFINE5(mq_timedreceive, mqd_t, mqdes, char __user *, u_msg_ptr,
 	audit_mq_sendrecv(mqdes, msg_len, 0, p);
 	timeout = prepare_timeout(p);
 
-	ret = -EBADF;
 	filp = fget(mqdes);
-	if (unlikely(!filp))
+	if (unlikely(!filp)) {
+		ret = -EBADF;
 		goto out;
+	}
 
 	inode = filp->f_path.dentry->d_inode;
-	if (unlikely(filp->f_op != &mqueue_file_operations))
+	if (unlikely(filp->f_op != &mqueue_file_operations)) {
+		ret = -EBADF;
 		goto out_fput;
+	}
 	info = MQUEUE_I(inode);
 	audit_inode(NULL, filp->f_path.dentry);
 
-	if (unlikely(!(filp->f_mode & FMODE_READ)))
+	if (unlikely(!(filp->f_mode & FMODE_READ))) {
+		ret = -EBADF;
 		goto out_fput;
+	}
 
 	/* checks if buffer is big enough */
 	if (unlikely(msg_len < info->attr.mq_msgsize)) {
@@ -1065,13 +1080,14 @@ SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
 
 			/* create the notify skb */
 			nc = alloc_skb(NOTIFY_COOKIE_LEN, GFP_KERNEL);
-			ret = -ENOMEM;
-			if (!nc)
+			if (!nc) {
+				ret = -ENOMEM;
 				goto out;
-			ret = -EFAULT;
+			}
 			if (copy_from_user(nc->data,
 					notification.sigev_value.sival_ptr,
 					NOTIFY_COOKIE_LEN)) {
+				ret = -EFAULT;
 				goto out;
 			}
 
@@ -1080,9 +1096,10 @@ SYSCALL_DEFINE2(mq_notify, mqd_t, mqdes,
 			/* and attach it to the socket */
 retry:
 			filp = fget(notification.sigev_signo);
-			ret = -EBADF;
-			if (!filp)
+			if (!filp) {
+				ret = -EBADF;
 				goto out;
+			}
 			sock = netlink_getsockbyfilp(filp);
 			fput(filp);
 			if (IS_ERR(sock)) {
@@ -1094,7 +1111,7 @@ retry:
 			timeo = MAX_SCHEDULE_TIMEOUT;
 			ret = netlink_attachskb(sock, nc, &timeo, NULL);
 			if (ret == 1)
-		       		goto retry;
+				goto retry;
 			if (ret) {
 				sock = NULL;
 				nc = NULL;
@@ -1103,14 +1120,17 @@ retry:
 		}
 	}
 
-	ret = -EBADF;
 	filp = fget(mqdes);
-	if (!filp)
+	if (!filp) {
+		ret = -EBADF;
 		goto out;
+	}
 
 	inode = filp->f_path.dentry->d_inode;
-	if (unlikely(filp->f_op != &mqueue_file_operations))
+	if (unlikely(filp->f_op != &mqueue_file_operations)) {
+		ret = -EBADF;
 		goto out_fput;
+	}
 	info = MQUEUE_I(inode);
 
 	ret = 0;
@@ -1173,14 +1193,17 @@ SYSCALL_DEFINE3(mq_getsetattr, mqd_t, mqdes,
 			return -EINVAL;
 	}
 
-	ret = -EBADF;
 	filp = fget(mqdes);
-	if (!filp)
+	if (!filp) {
+		ret = -EBADF;
 		goto out;
+	}
 
 	inode = filp->f_path.dentry->d_inode;
-	if (unlikely(filp->f_op != &mqueue_file_operations))
+	if (unlikely(filp->f_op != &mqueue_file_operations)) {
+		ret = -EBADF;
 		goto out_fput;
+	}
 	info = MQUEUE_I(inode);
 
 	spin_lock(&info->lock);
-- 
1.7.0.87.g0901d

[PATCH 5/6] mqueue: fix typo "failues" -> "failures"

[André Goddard Rosa]

Signed-off-by: André Goddard Rosa <andre.goddard@gmail.com>
---
 ipc/mqueue.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 906e873..e47c9eb 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -1297,7 +1297,7 @@ static int __init init_mqueue_fs(void)
 	if (mqueue_inode_cachep == NULL)
 		return -ENOMEM;
 
-	/* ignore failues - they are not fatal */
+	/* ignore failures - they are not fatal */
 	mq_sysctl_table = mq_register_sysctl_table();
 
 	error = register_filesystem(&mqueue_fs_type);
-- 
1.7.0.87.g0901d

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

[André Goddard Rosa]

It can be triggered by the following test program:

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <mqueue.h>
#include <fcntl.h>
#include <errno.h>
#include <sys/stat.h>
#include <sys/resource.h>

int main(int argc, char *argv[])
{
	struct rlimit limit;
	char queue_name[] = "/mq_open/bug";
	char tmp_name[]   = "/tmp/tmp";
	int fd, i = 1;

	if (getrlimit(RLIMIT_NOFILE, &limit) != 0) {
		printf("%s\n", "Failed to get RLIMIT_NOFILE");
		return EXIT_FAILURE;
	}
	printf("Max number of open files is: %d\n", limit.rlim_cur);

	while (i <= limit.rlim_cur) {
		mqd_t queue;

		errno = 0;
		queue = mq_open(queue_name, O_CREAT |O_RDWR, S_IRUSR | S_IWUSR
		    , NULL);
		if (queue != (mqd_t)-1) {
			/* Success opening mqueue, no leak will happen */
			printf("Successfully opened an mqueue[%d]\n", queue);
			printf("mq_close(%d) = %d\n", queue, mq_close(queue));
			return EXIT_SUCCESS;
		}
		/* Failed to open mqueue, maybe a leak is happening... */
		if (errno == EMFILE)
		{
			printf("\nRun out of file descriptors");
			break;
		}
		printf("\rLeaking [%d] files?!?!", i++);
		fflush(stdout);
		usleep(500);
	}
	/* Double check that no file descriptor is available anymore indeed */
	putchar('\n');
	errno = 0;
	fd = open(tmp_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
	if (fd == -1) {
		printf("open() failed: %s\n", strerror(errno));
		if (errno == EMFILE) {
			printf("%s\n", "Cannot open new files, fds exhausted");
			return EXIT_FAILURE;
		}
	} else
		printf("close(%d) = %d\n", fd, close(fd));
	printf("%s\n", "Expected output: kernel is not leaking any fds!");

	return EXIT_SUCCESS;
}

## Preparing for testing

$ touch /tmp/tmp
$ gcc -g main_mq_open_fd_leak.c -lrt

## Linux kernel with the fix applied:

$ ./a.out
Max number of open files is: 1024
Leaking [1024] files?!?!
close(3) = 0
Expected output: kernel is not leaking any fds!

## Linux kernel without the fix:

## Shell execution:

$ ./a.out
Max number of open files is: 1024
Leaking [1019] files?!?!
Run out of file descriptors
Segmentation fault

## Valgrind execution:

$ valgrind --track-fds=yes ./a.out
==2895== Memcheck, a memory error detector
==2895== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
==2895== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info
==2895== Command: ./a.out
==2895==
Max number of open files is: 1024
Leaking [1024] files?!?!
open() failed: Too many open files
Cannot open new files, fds exhausted
==2895==
==2895== FILE DESCRIPTORS: 5 open at exit.
==2895== Open file descriptor 13:
==2895==    <inherited from parent>
==2895==
==2895== Open file descriptor 12:
==2895==    <inherited from parent>
==2895==
==2895== Open file descriptor 2: /dev/pts/1
==2895==    <inherited from parent>
==2895==
==2895== Open file descriptor 1: /dev/pts/1
==2895==    <inherited from parent>
==2895==
==2895== Open file descriptor 0: /dev/pts/1
==2895==    <inherited from parent>
==2895==
==2895==
==2895== HEAP SUMMARY:
==2895==     in use at exit: 0 bytes in 0 blocks
==2895==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
==2895==
==2895== All heap blocks were freed -- no leaks are possible
==2895==
==2895== For counts of detected and suppressed errors, rerun with: -v
==2895== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)

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>
---
 ipc/mqueue.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index e47c9eb..b6cb064 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -710,7 +710,7 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 	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_putfd;
 	}
 	mntget(ipc_ns->mq_mnt);
 
@@ -749,7 +749,6 @@ out:
 	mntput(ipc_ns->mq_mnt);
 out_putfd:
 	put_unused_fd(fd);
-out_err:
 	fd = error;
 out_upsem:
 	mutex_unlock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
-- 
1.7.0.87.g0901d

Re: [PATCH 0/6] Fix file descriptor leak on user-space processes and cleanup

[Andrew Morton]

On Tue, 23 Feb 2010 04:04:22 -0300 Andr__ Goddard Rosa <andre.goddard@gmail.com> wrote:

> Fix a file descriptor leak on user-space processes and perform a cleanup,
> reducing the code size:
> text    data     bss     dec     hex filename
> 9949      72      16   10037    2735 ipc/mqueue-BEFORE.o
> 9885      72      16    9973    26f5 ipc/mqueue-AFTER.o
> 
> Andr__ Goddard Rosa (6):
>   mqueue: remove unneeded info->messages initialization
>   mqueue: apply mathematics distributivity on mq_bytes calculation
>   mqueue: simplify do_open() error handling
>   mqueue: only set error codes if they are really necessary
>   mqueue: fix typo "failues" -> "failures"
>   mqueue: fix mq_open() file descriptor leak on user-space processes

Fixing the leak is far more important than the other five patches, and
we'll want to backport the leak fix into earlier kernels.  So the
bugfix patch should have been the first in the series!

So I've reordered the patches in that fashion and shall tag "mqueue:
fix mq_open() file descriptor leak on user-space processes" as needing
-stable backporting.

The patches apply and build OK with that reordering, but please do
double-check it, thanks.

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

[Américo Wang]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

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.

Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

---

[-- Attachment #2: ipc-mqueue_c-cleanup-failure-path.diff --]
[-- Type: text/plain, Size: 1348 bytes --]

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index c79bd57..8dee7a3 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)))
@@ -705,7 +705,7 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
 	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);
 
@@ -731,24 +731,22 @@ 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:
+	dput(dentry);
+out_unlock:
 	mutex_unlock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
+	if (error)
+		put_unused_fd(fd);
 out_putname:
 	putname(name);
-	return fd;
+	return error;
 }
 
 SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)

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

[Xiaotian Feng]

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

> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>
> ---
>

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

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

[Américo Wang]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

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>

[-- Attachment #2: ipc-mqueue_c-cleanup-failure-path.diff --]
[-- Type: text/plain, Size: 1567 bytes --]

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)

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

[Xiaotian Feng]

On Thu, Feb 25, 2010 at 2:59 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> 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?

This is definitely wrong,

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

filp is assigned by do_open or do_create in mqueue.c, take a look at
the code, if do_open/do_create is failed, kernel is already dput &
mntput...

So I think original patch from André is enough....

> ---------
>
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
>

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

[Xiaotian Feng]

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:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <mqueue.h>
> #include <fcntl.h>
> #include <errno.h>
> #include <sys/stat.h>
> #include <sys/resource.h>
>
> int main(int argc, char *argv[])
> {
>        struct rlimit limit;
>        char queue_name[] = "/mq_open/bug";
>        char tmp_name[]   = "/tmp/tmp";
>        int fd, i = 1;
>
>        if (getrlimit(RLIMIT_NOFILE, &limit) != 0) {
>                printf("%s\n", "Failed to get RLIMIT_NOFILE");
>                return EXIT_FAILURE;
>        }
>        printf("Max number of open files is: %d\n", limit.rlim_cur);
>
>        while (i <= limit.rlim_cur) {
>                mqd_t queue;
>
>                errno = 0;
>                queue = mq_open(queue_name, O_CREAT |O_RDWR, S_IRUSR | S_IWUSR
>                    , NULL);
>                if (queue != (mqd_t)-1) {
>                        /* Success opening mqueue, no leak will happen */
>                        printf("Successfully opened an mqueue[%d]\n", queue);
>                        printf("mq_close(%d) = %d\n", queue, mq_close(queue));
>                        return EXIT_SUCCESS;
>                }
>                /* Failed to open mqueue, maybe a leak is happening... */
>                if (errno == EMFILE)
>                {
>                        printf("\nRun out of file descriptors");
>                        break;
>                }
>                printf("\rLeaking [%d] files?!?!", i++);
>                fflush(stdout);
>                usleep(500);
>        }
>        /* Double check that no file descriptor is available anymore indeed */
>        putchar('\n');
>        errno = 0;
>        fd = open(tmp_name, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
>        if (fd == -1) {
>                printf("open() failed: %s\n", strerror(errno));
>                if (errno == EMFILE) {
>                        printf("%s\n", "Cannot open new files, fds exhausted");
>                        return EXIT_FAILURE;
>                }
>        } else
>                printf("close(%d) = %d\n", fd, close(fd));
>        printf("%s\n", "Expected output: kernel is not leaking any fds!");
>
>        return EXIT_SUCCESS;
> }
>
> ## Preparing for testing
>
> $ touch /tmp/tmp
> $ gcc -g main_mq_open_fd_leak.c -lrt
>
> ## Linux kernel with the fix applied:
>
> $ ./a.out
> Max number of open files is: 1024
> Leaking [1024] files?!?!
> close(3) = 0
> Expected output: kernel is not leaking any fds!
>
> ## Linux kernel without the fix:
>
> ## Shell execution:
>
> $ ./a.out
> Max number of open files is: 1024
> Leaking [1019] files?!?!
> Run out of file descriptors
> Segmentation fault
>
> ## Valgrind execution:
>
> $ valgrind --track-fds=yes ./a.out
> ==2895== Memcheck, a memory error detector
> ==2895== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al.
> ==2895== Using Valgrind-3.6.0.SVN and LibVEX; rerun with -h for copyright info
> ==2895== Command: ./a.out
> ==2895==
> Max number of open files is: 1024
> Leaking [1024] files?!?!
> open() failed: Too many open files
> Cannot open new files, fds exhausted
> ==2895==
> ==2895== FILE DESCRIPTORS: 5 open at exit.
> ==2895== Open file descriptor 13:
> ==2895==    <inherited from parent>
> ==2895==
> ==2895== Open file descriptor 12:
> ==2895==    <inherited from parent>
> ==2895==
> ==2895== Open file descriptor 2: /dev/pts/1
> ==2895==    <inherited from parent>
> ==2895==
> ==2895== Open file descriptor 1: /dev/pts/1
> ==2895==    <inherited from parent>
> ==2895==
> ==2895== Open file descriptor 0: /dev/pts/1
> ==2895==    <inherited from parent>
> ==2895==
> ==2895==
> ==2895== HEAP SUMMARY:
> ==2895==     in use at exit: 0 bytes in 0 blocks
> ==2895==   total heap usage: 0 allocs, 0 frees, 0 bytes allocated
> ==2895==
> ==2895== All heap blocks were freed -- no leaks are possible
> ==2895==
> ==2895== For counts of detected and suppressed errors, rerun with: -v
> ==2895== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 2)
>
> 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>

If kernel failed to lookup the dentry in mqueue,  the fd allocated by
get_unused_fd_flags will be leaked then.
I think this is a good catch ;-)

Acked-by: Xiaotian Feng <xtfeng@gmail.com>

> ---
>  ipc/mqueue.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index e47c9eb..b6cb064 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -710,7 +710,7 @@ SYSCALL_DEFINE4(mq_open, const char __user *, u_name, int, oflag, mode_t, mode,
>        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_putfd;
>        }
>        mntget(ipc_ns->mq_mnt);
>
> @@ -749,7 +749,6 @@ out:
>        mntput(ipc_ns->mq_mnt);
>  out_putfd:
>        put_unused_fd(fd);
> -out_err:
>        fd = error;
>  out_upsem:
>        mutex_unlock(&ipc_ns->mq_mnt->mnt_root->d_inode->i_mutex);
> --
> 1.7.0.87.g0901d
>
> --
> 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/
>

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

[Américo Wang]

On Thu, Feb 25, 2010 at 06:49:09PM +0800, Xiaotian Feng wrote:
>On Thu, Feb 25, 2010 at 2:59 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> 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?
>
>This is definitely wrong,
>
> 	if (IS_ERR(filp)) {
> 		error = PTR_ERR(filp);
>-		goto out_putfd;
>+		goto out;
> 	}
>
>filp is assigned by do_open or do_create in mqueue.c, take a look at
>the code, if do_open/do_create is failed, kernel is already dput &
>mntput...
>

Clearly the original code is a piece of sh*t.

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

[Américo Wang]

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)

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

[André Goddard Rosa]

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é

Re: [PATCH 0/6] Fix file descriptor leak on user-space processes and cleanup

[André Goddard Rosa]

Hi, Andrew!

On Wed, Feb 24, 2010 at 7:01 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 23 Feb 2010 04:04:22 -0300 Andr__ Goddard Rosa <andre.goddard@gmail.com> wrote:
>
>> Fix a file descriptor leak on user-space processes and perform a cleanup,
>> reducing the code size:
>> text    data     bss     dec     hex filename
>> 9949      72      16   10037    2735 ipc/mqueue-BEFORE.o
>> 9885      72      16    9973    26f5 ipc/mqueue-AFTER.o
>>
>> Andr__ Goddard Rosa (6):
>>   mqueue: remove unneeded info->messages initialization
>>   mqueue: apply mathematics distributivity on mq_bytes calculation
>>   mqueue: simplify do_open() error handling
>>   mqueue: only set error codes if they are really necessary
>>   mqueue: fix typo "failues" -> "failures"
>>   mqueue: fix mq_open() file descriptor leak on user-space processes
>
> Fixing the leak is far more important than the other five patches, and
> we'll want to backport the leak fix into earlier kernels.  So the
> bugfix patch should have been the first in the series!

Sure, thank you for that, I'll consider your good advice.

> So I've reordered the patches in that fashion and shall tag "mqueue:
> fix mq_open() file descriptor leak on user-space processes" as needing
> -stable backporting.
>
> The patches apply and build OK with that reordering, but please do
> double-check it, thanks.
>

I have double checked and they look good; thanks for the follow-up
patch pleasing checkpatch.
:)

Do I need to send another patch adding the Acked-by's or is it done by
email system automatically?

Best regards,
André

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

[Américo Wang]

On Thu, Feb 25, 2010 at 12:41:47PM -0300, André Goddard Rosa wrote:
>Hi, Américo!
>
...
>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);
>

Oh, I trusted the current code too much, clearly this needs to be fixed
too. I already checked the 14 callers of dentry_open(), and will send
out a patchset to fix this tomorrow. (And the mqueue part will be based
on your patch.)

Thanks!

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

[Al Viro]

On Thu, Feb 25, 2010 at 09:40:23PM +0800, Am??rico Wang 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;

Sorry, NAK.  Resources are either consumed by opened file (i.e. struct
file now is the holder of mnt/dentry) or released; in either case,
caller has given them up.  Your variant is broken in its current form
and will be more complicated than existing code if you fix it.

> b) Fix an fd leak;

Fixed by original patch.

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

Not really, since ->mnt doesn't change (and remains pinned) for the
entire lifetime of ipc_ns.

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

I wouldn't say it's cleaner that way.

Anyway, I've applied the patchset as posted; if you want to do something
else, do that on top of it.  I really object against your (a), the rest
is more or less a matter of taste.  (a) is just plain wrong - we want
uniform behaviour from the caller's POV and it means that mnt/dentry should
always be given up from caller's POV.  Either moved into new struct file,
or dropped.