Stefano Garzarella
2023-Aug-22 09:36 UTC
[RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket
On Mon, Aug 14, 2023 at 10:40:17PM +0300, Arseniy Krasnov wrote:> > >On 04.08.2023 18:02, Stefano Garzarella wrote: >> On Fri, Aug 04, 2023 at 05:34:20PM +0300, Arseniy Krasnov wrote: >>> >>> >>> On 04.08.2023 17:28, Stefano Garzarella wrote: >>>> On Fri, Aug 04, 2023 at 03:46:47PM +0300, Arseniy Krasnov wrote: >>>>> Hi Stefano, >>>>> >>>>> On 02.08.2023 10:46, Stefano Garzarella wrote: >>>>>> On Tue, Aug 01, 2023 at 05:17:26PM +0300, Arseniy Krasnov wrote: >>>>>>> POSIX requires to send SIGPIPE on write to SOCK_STREAM socket which was >>>>>>> shutdowned with SHUT_WR flag or its peer was shutdowned with SHUT_RD >>>>>>> flag. Also we must not send SIGPIPE if MSG_NOSIGNAL flag is set. >>>>>>> >>>>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov at sberdevices.ru> >>>>>>> --- >>>>>>> net/vmw_vsock/af_vsock.c | 3 +++ >>>>>>> 1 file changed, 3 insertions(+) >>>>>>> >>>>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c >>>>>>> index 020cf17ab7e4..013b65241b65 100644 >>>>>>> --- a/net/vmw_vsock/af_vsock.c >>>>>>> +++ b/net/vmw_vsock/af_vsock.c >>>>>>> @@ -1921,6 +1921,9 @@ static int vsock_connectible_sendmsg(struct socket *sock, struct msghdr *msg, >>>>>>> ??????????? err = total_written; >>>>>>> ????} >>>>>>> out: >>>>>>> +??? if (sk->sk_type == SOCK_STREAM) >>>>>>> +??????? err = sk_stream_error(sk, msg->msg_flags, err); >>>>>> >>>>>> Do you know why we don't need this for SOCK_SEQPACKET and SOCK_DGRAM? >>>>> >>>>> Yes, here is my explanation: >>>>> >>>>> This function checks that input error is SIGPIPE, and if so it sends SIGPIPE to the 'current' thread >>>>> (except case when MSG_NOSIGNAL flag is set). This behaviour is described in POSIX: >>>>> >>>>> Page 367 (description of defines from sys/socket.h): >>>>> MSG_NOSIGNAL: No SIGPIPE generated when an attempt to send is made on a stream- >>>>> oriented socket that is no longer connected. >>>>> >>>>> Page 497 (description of SOCK_STREAM): >>>>> A SIGPIPE signal is raised if a thread sends on a broken stream (one that is >>>>> no longer connected). >>>> >>>> Okay, but I think we should do also for SEQPACKET: >>>> >>>> https://pubs.opengroup.org/onlinepubs/009696699/functions/xsh_chap02_10.html >>>> >>>> In 2.10.6 Socket Types: >>>> >>>> "The SOCK_SEQPACKET socket type is similar to the SOCK_STREAM type, and >>>> is also connection-oriented. The only difference between these types is >>>> that record boundaries ..." >>>> >>>> Then in? 2.10.14 Signals: >>>> >>>> "The SIGPIPE signal shall be sent to a thread that attempts to send data >>>> on a socket that is no longer able to send. In addition, the send >>>> operation fails with the error [EPIPE]." >>>> >>>> It's honestly not super clear, but I assume the problem is similar with >>>> seqpacket since it's connection-oriented, or did I miss something? >>>> >>>> For example in sctp_sendmsg() IIUC we raise a SIGPIPE regardless of >>>> whether the socket is STREAM or SEQPACKET. >>> >>> Hm, yes, you're right. Seems check for socket type is not needed in this case, >>> as this function is only for connection oriented sockets. >> >> Ack! >> >>> >>>> >>>>> >>>>> Page 1802 (description of 'send()' call): >>>>> MSG_NOSIGNAL >>>>> >>>>> Requests not to send the SIGPIPE signal if an attempt to >>>>> send is made on a stream-oriented socket that is no >>>>> longer connected. The [EPIPE] error shall still be >>>>> returned >>>>> >>>>> And the same for 'sendto()' and 'sendmsg()' >>>>> >>>>> Link to the POSIX document: >>>>> https://www.open-std.org/jtc1/sc22/open/n4217.pdf >>>>> >>>>> TCP (I think we must rely on it), KCM, SMC sockets (all of them are stream) work in the same >>>>> way by calling this function. AF_UNIX also works in the same way, but it implements SIGPIPE handling >>>>> without this function. >>>> >>>> I'm okay calling this function. >>>> >>>>> >>>>> The only thing that confused me a little bit, that sockets above returns EPIPE when >>>>> we have only SEND_SHUTDOWN set, but for AF_VSOCK EPIPE is returned for RCV_SHUTDOWN >>>>> also, but I think it is related to this patchset. >>>> >>>> Do you mean that it is NOT related to this patchset? >>> >>> Yes, **NOT** >> >> Got it, so if you have time when you're back, let's check also that >> (not for this series as you mentioned). > >^^^ >Hello Stefano, so: > >there is some confusion with check for RCV_SHUTDOWN: it presents in AF_UNIX, but missed >in TCP (it checks only for SEND_SHUTDOWN). I performed simple test which tries >to send data to peer which already called shutdown(SHUT_RD) - AF_UNIX and TCP behave >differently. AF_UNIX sends SIGPIPE, while TCP allows to send data. > >I suggest to not touch this check for AF_VSOCK (e.g. continue work as AF_UNIX), >because I don't see strong motivation/argument to remove it.Yep, I agree! However, I think it's a fairly borderline case, so unless we have a specific request, I wouldn't spend too much time on it. Thanks for looking at it! Stefano