Re: [PATCH] fs/ncpfs: fix error paths and goto statements inncp_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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/