[PATCH] fs/ncpfs: fix error paths and goto statements in ncp_fill_super()

[PATCH] fs/ncpfs: fix error paths and goto statements in ncp_fill_super()

[Djalal Harouni]

The label 'out_bdi' should be followed by bdi_destroy() instead of
fput() which should be after the 'out_fput' label.

If bdi_setup_and_register() fails then jump to the 'out_fput' label
instead of the 'out_bdi' one.

If fget(data.info_fd) fails then jump to the previously fixed 'out_bdi'
label to call bdi_destroy() otherwise the bdi object will not be
destroyed.

Compile tested only.

Signed-off-by: Djalal Harouni <tixxdz@opendz.org>
---
 fs/ncpfs/inode.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ncpfs/inode.c b/fs/ncpfs/inode.c
index 5b5fa33..cbd1a61 100644
--- a/fs/ncpfs/inode.c
+++ b/fs/ncpfs/inode.c
@@ -548,7 +548,7 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
 
 	error = bdi_setup_and_register(&server->bdi, "ncpfs", BDI_CAP_MAP_COPY);
 	if (error)
-		goto out_bdi;
+		goto out_fput;
 
 	server->ncp_filp = ncp_filp;
 	server->ncp_sock = sock;
@@ -559,7 +559,7 @@ static int ncp_fill_super(struct super_block *sb, void *raw_data, int silent)
 		error = -EBADF;
 		server->info_filp = fget(data.info_fd);
 		if (!server->info_filp)
-			goto out_fput;
+			goto out_bdi;
 		error = -ENOTSOCK;
 		sock_inode = server->info_filp->f_path.dentry->d_inode;
 		if (!S_ISSOCK(sock_inode->i_mode))
@@ -746,9 +746,9 @@ out_nls:
 out_fput2:
 	if (server->info_filp)
 		fput(server->info_filp);
-out_fput:
-	bdi_destroy(&server->bdi);
 out_bdi:
+	bdi_destroy(&server->bdi);
+out_fput:
 	/* 23/12/1998 Marcin Dalecki <dalecki@cs.net.pl>:
 	 * 
 	 * The previously used put_filp(ncp_filp); was bogus, since
-- 
1.7.1

Re: [PATCH] fs/ncpfs: fix error paths and goto statements in ncp_fill_super()

[Al Viro]

[davem Cc'd, since there's an ugly socket-related problem in that area]

On Tue, Dec 13, 2011 at 02:47:29AM +0100, Djalal Harouni wrote:
> The label 'out_bdi' should be followed by bdi_destroy() instead of
> fput() which should be after the 'out_fput' label.
> 
> If bdi_setup_and_register() fails then jump to the 'out_fput' label
> instead of the 'out_bdi' one.
> 
> If fget(data.info_fd) fails then jump to the previously fixed 'out_bdi'
> label to call bdi_destroy() otherwise the bdi object will not be
> destroyed.
> 
> Compile tested only.

Applied, but now that I've looked at that code...  It, along with several
other places, overwrites a bunch of fields in sock->sk.  What happens if
some joker feeds that descriptor to, say, svc_addsock()?  It does the
same kind of thing, and both assume that no bonehead programmer would
ever do something that dumb.  Moreover, what if somebody feeds the same fd
to ncpfs mount or svc_addsock twice?

I'd done a quick grep and AFAICS, most of the places doing that kind of thing
is acting on internally created socket, so no such problem for them.  Or
they take care to check that somebody has already done that to socket in
question.  Potentially unpleasant ones:
	drivers/scsi/iscsi_tcp.c:iscsi_sw_tcp_conn_bind()
	fs/dlm/lowcomms.c:process_sctp_notification()
	fs/ncpfs/inode.c:ncp_fill_super()
	net/sunrpc/svcsock.c:svc_addsock()

I'm not familiar with that area at all; not even enough to tell if locking
is wrong in svc_addsock() - I suspect that it is, but...

Dave, am I right assuming that all those places ought to check if somebody
is already parasiting on the socket in question and fail with -EBUSY or
something like that?