From mboxrd@z Thu Jan  1 00:00:00 1970
From: Oleg Nesterov <oleg@redhat.com>
Subject: Re: [patch for 2.6.29? 1/3] pipe_rdwr_fasync: fix the error
	handling to prevent the leak/crash
Date: Wed, 4 Mar 2009 21:51:28 +0100
Message-ID: <20090304205128.GA19304@redhat.com>
References: <200903042012.n24KCUlA030022@imap1.linux-foundation.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org,
	andi@firstfloor.org, corbet@lwn.net, stable@kernel.org
To: akpm@linux-foundation.org
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:56676 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1754356AbZCDUyn (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Wed, 4 Mar 2009 15:54:43 -0500
Content-Disposition: inline
In-Reply-To: <200903042012.n24KCUlA030022@imap1.linux-foundation.org>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On 03/04, Andrew Morton wrote:
>
> From: Oleg Nesterov <oleg@redhat.com>
>
> If the second fasync_helper() fails, pipe_rdwr_fasync() returns the error
> but leaves the file on ->fasync_readers.
>
> This was always wrong, but since 233e70f4228e78eb2f80dc6650f65d3ae3dbf17c
> "saner FASYNC handling on file close" we have the new problem.  Because in
> this case setfl() doesn't set FASYNC bit, __fput() will not do
> ->fasync(0), and we leak fasync_struct with ->fa_file pointing to the
> freed file.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: <stable@kernel.org>		[2.6.28.x]
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>

Just in case...

This bug is minor. fasync_helper() can only fail if
kmem_cache_alloc(fasync_cache, GFP_KERNEL) fails, this should "never" happen.

Perhaps -stable doesn't need this fix.

> 
>  fs/pipe.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff -puN fs/pipe.c~pipe_rdwr_fasync-fix-the-error-handling-to-prevent-the-leak-crash fs/pipe.c
> --- a/fs/pipe.c~pipe_rdwr_fasync-fix-the-error-handling-to-prevent-the-leak-crash
> +++ a/fs/pipe.c
> @@ -693,11 +693,12 @@ pipe_rdwr_fasync(int fd, struct file *fi
>  	int retval;
>  
>  	mutex_lock(&inode->i_mutex);
> -
>  	retval = fasync_helper(fd, filp, on, &pipe->fasync_readers);
> -	if (retval >= 0)
> +	if (retval >= 0) {
>  		retval = fasync_helper(fd, filp, on, &pipe->fasync_writers);
> -
> +		if (retval < 0) /* this can happen only if on == T */
> +			fasync_helper(-1, filp, 0, &pipe->fasync_readers);
> +	}
>  	mutex_unlock(&inode->i_mutex);
>  	return retval;
>  }
> _