David Miller <[email protected]> writes: > From: [email protected] (Eric W. Biederman) > Date: Sun, 24 Apr 2011 04:54:57 -0700 > >> +static int unix_seqpacket_recvmsg(struct kiocb *iocb, struct socket *sock, >> + struct msghdr *msg, size_t size, >> + int flags) >> +{ >> + struct sock *sk = sock->sk; >> + >> + if (sk->sk_state != TCP_ESTABLISHED) >> + return -ENOTCONN; > > As for unix_seqpacket_sendmsg(), you need to add a check for sock_error() > or similar here otherwise -ECONNRESET is not reported correctly. > > In fact, recvmsg() is even harder than sendmsg() to handle correctly, > because we have to also properly report EOF on seqpacket sockets which > have RCV_SHUTDOWN set. > > So a lot more work has to go into this change to make it fix the bug > without also breaking existing semantics.
Really? When I read through the code I am failing to see the issues you are seeing. When the other socket in an established connection calls unix_shutdown or unix_release_sock. sk->sk_shutdown is changed, but sk_state is left at TCP_ESTABLISHED. Therefore we do not need a special case in unix_seqpacket_recvmsg to handle the RCV_SHUTDOWN case because in any case where that applies we will be in TCP_ESTABLISHED and we will simply call unix_dgram_recvmsg. As for ECONNRESET when I look a look at the code it appears to be another variant of the other side calling shutdown or close. So if it applies we should remain in TCP_ESTABLISHED, and unix_seqpacket_recvmsg should not need to do anything. So looking at this the only times I can see that sk_state would not be TCP_ESTABLISHED in a unix domain seqpacket socket are. - On a listening socket, where calling recvmsg is what this patch is meant to address. - Before we call connect or listen. Which appears to be equally broken today. The only errors I can see happening in the case we are not connected today are blocking forever or returning -EINTR if we timeout. Adding sock_error() handling into the new unix_seqpacket_recvmsg makes a fair amount of sense but adding a new call to sock_error in that path seems marginally more likely to change error codes and break existing apps. We already have a few other unconditional error codes before we check sk_err in unix_dgram_recvmsg. > Anyways, see: > > commit 6e14891f4d16f8a9e0bc3a8408f73b3aed93ab0a > Author: James Morris <[email protected]> > Date: Fri Nov 19 07:02:41 2004 -0800 > > [AF_UNIX]: Don't lose ECONNRESET in unix_seqpacket_sendmsg() > > The fix for SELinux w/SOCK_SEQPACKET had an error, > noted by Alan Cox. This fixes it. > > Signed-off-by: James Morris <[email protected]> > Signed-off-by: David S. Miller <[email protected]> Looking into it. That patch appears to have been unnecessary. We never transition out of the state TCP_ESTABLISHED once we get there, and we can never get ECONNRESET unless we are connected. Arguably we could reduce unix_seqpacket_sendmsg to simply static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock, struct msghdr *msg, size_t len) { if (msg->msgnamelen) msg->msgnamelen = 0; return unix_dgram_sendmsg(kiocb, sock, msg, len); } But I think having the explicit TCP_ESTABLISHED check makes for better maintainability, of unix_dgram_sendmesg. So having gone through all of that it looks like my patch needs a comment saying that once we are in TCP_ESTABLISHED we cannot leave, and that nothing can happen before we are TCP_ESTABLISHED. We can use sock_error to check sk_err, as it seems good hygiene but it also appears pointless. Especially for recvmsg where ECONNRESET never applies. Eric > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c > index 16faa9d..8902c4a 100644 > --- a/net/unix/af_unix.c > +++ b/net/unix/af_unix.c > @@ -1513,13 +1513,18 @@ out_err: > static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock, > struct msghdr *msg, size_t len) > { > + int err; > struct sock *sk = sock->sk; > > + err = sock_error(sk); > + if (err) > + return err; > + > if (sk->sk_state != TCP_ESTABLISHED) > return -ENOTCONN; > > - if (msg->msg_name || msg->msg_namelen) > - return -EINVAL; > + if (msg->msg_namelen) > + msg->msg_namelen = 0; > > return unix_dgram_sendmsg(kiocb, sock, msg, len); > } _______________________________________________ stable mailing list [email protected] http://linux.kernel.org/mailman/listinfo/stable
