* [RFC PATCH v1 0/2] vsock: handle writes to shutdowned socket @ 2023-08-01 14:17 Arseniy Krasnov 2023-08-01 14:17 ` [RFC PATCH v1 1/2] vsock: send SIGPIPE on write " Arseniy Krasnov 2023-08-01 14:17 ` [RFC PATCH v1 2/2] test/vsock: shutdowned socket test Arseniy Krasnov 0 siblings, 2 replies; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-01 14:17 UTC (permalink / raw) To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa, avkrasnov, Arseniy Krasnov Hello, this small patchset adds POSIX compliant behaviour on writes to the socket which was shutdowned with 'shutdown()' (both sides - local with SHUT_WR flag, peer - with SHUT_RD flag). According POSIX we must send SIGPIPE in such cases (but SIGPIPE is not send when MSG_NOSIGNAL is set). First patch is implemented in the same way as net/ipv4/tcp.c:tcp_sendmsg_locked(). It uses 'sk_stream_error()' function which handles EPIPE error. Another way is to use code from net/unix/af_unix.c:unix_stream_sendmsg() where same logic from 'sk_stream_error()' is implemented "from scratch", but it doesn't check 'sk_err' field. I think error from this field has more priority to be returned from syscall. So I guess it is better to reuse currently implemented 'sk_stream_error()' function. Test is also added. Head for this patchset is: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=9d0cd5d25f7d45bce01bbb3193b54ac24b3a60f3 Arseniy Krasnov (2): vsock: send SIGPIPE on write to shutdowned socket test/vsock: shutdowned socket test net/vmw_vsock/af_vsock.c | 3 + tools/testing/vsock/vsock_test.c | 138 +++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+) -- 2.25.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-01 14:17 [RFC PATCH v1 0/2] vsock: handle writes to shutdowned socket Arseniy Krasnov @ 2023-08-01 14:17 ` Arseniy Krasnov 2023-08-02 7:46 ` Stefano Garzarella 2023-08-01 14:17 ` [RFC PATCH v1 2/2] test/vsock: shutdowned socket test Arseniy Krasnov 1 sibling, 1 reply; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-01 14:17 UTC (permalink / raw) To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa, avkrasnov, Arseniy Krasnov 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@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); + release_sock(sk); return err; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-01 14:17 ` [RFC PATCH v1 1/2] vsock: send SIGPIPE on write " Arseniy Krasnov @ 2023-08-02 7:46 ` Stefano Garzarella 2023-08-04 12:46 ` Arseniy Krasnov 0 siblings, 1 reply; 16+ messages in thread From: Stefano Garzarella @ 2023-08-02 7:46 UTC (permalink / raw) To: Arseniy Krasnov Cc: Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa 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@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? Thanks, Stefano >+ > release_sock(sk); > return err; > } >-- >2.25.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-02 7:46 ` Stefano Garzarella @ 2023-08-04 12:46 ` Arseniy Krasnov 2023-08-04 14:28 ` Stefano Garzarella 0 siblings, 1 reply; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-04 12:46 UTC (permalink / raw) To: Stefano Garzarella, Arseniy Krasnov Cc: Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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). 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. 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. Thanks, Arseniy > > Thanks, > Stefano > >> + >> release_sock(sk); >> return err; >> } >> -- >> 2.25.1 >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-04 12:46 ` Arseniy Krasnov @ 2023-08-04 14:28 ` Stefano Garzarella 2023-08-04 14:34 ` Arseniy Krasnov 2023-08-14 19:46 ` Arseniy Krasnov 0 siblings, 2 replies; 16+ messages in thread From: Stefano Garzarella @ 2023-08-04 14:28 UTC (permalink / raw) To: Arseniy Krasnov Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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. > >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? Thanks, Stefano ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-04 14:28 ` Stefano Garzarella @ 2023-08-04 14:34 ` Arseniy Krasnov 2023-08-04 15:02 ` Stefano Garzarella 2023-08-14 19:46 ` Arseniy Krasnov 1 sibling, 1 reply; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-04 14:34 UTC (permalink / raw) To: Stefano Garzarella Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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. > >> >> 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** > > Thanks, > Stefano > Thanks, Arseniy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-04 14:34 ` Arseniy Krasnov @ 2023-08-04 15:02 ` Stefano Garzarella 2023-08-04 16:24 ` Arseniy Krasnov 2023-08-14 19:40 ` Arseniy Krasnov 0 siblings, 2 replies; 16+ messages in thread From: Stefano Garzarella @ 2023-08-04 15:02 UTC (permalink / raw) To: Arseniy Krasnov Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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). Thanks, Stefano ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-04 15:02 ` Stefano Garzarella @ 2023-08-04 16:24 ` Arseniy Krasnov 2023-08-14 19:40 ` Arseniy Krasnov 1 sibling, 0 replies; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-04 16:24 UTC (permalink / raw) To: Stefano Garzarella Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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). Sure! Thanks, Arseniy > > Thanks, > Stefano > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-04 15:02 ` Stefano Garzarella 2023-08-04 16:24 ` Arseniy Krasnov @ 2023-08-14 19:40 ` Arseniy Krasnov 2023-08-22 9:36 ` Stefano Garzarella 1 sibling, 1 reply; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-14 19:40 UTC (permalink / raw) To: Stefano Garzarella, Arseniy Krasnov Cc: Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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. Thanks, Arseniy > > Thanks, > Stefano > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-14 19:40 ` Arseniy Krasnov @ 2023-08-22 9:36 ` Stefano Garzarella 0 siblings, 0 replies; 16+ messages in thread From: Stefano Garzarella @ 2023-08-22 9:36 UTC (permalink / raw) To: Arseniy Krasnov Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-04 14:28 ` Stefano Garzarella 2023-08-04 14:34 ` Arseniy Krasnov @ 2023-08-14 19:46 ` Arseniy Krasnov 2023-08-22 9:39 ` Stefano Garzarella 1 sibling, 1 reply; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-14 19:46 UTC (permalink / raw) To: Stefano Garzarella, Arseniy Krasnov Cc: Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel 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@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. Update about sending SIGPIPE for SOCK_SEQPACKET, I checked POSIX doc and kernel sources more deeply: 1) I checked four types of sockets, which sends SIGPIPE for SOCK_SEQPACKET or not ('YES' if this socket sends SIGPIPE in SOCK_SEQPACKET case): net/kcm/: YES net/unix/: NO net/sctp/: YES net/caif/: NO Looking for this, I think it is impossible to get the right answer, as there is some mess - everyone implements it as wish. 2) I opened POSIX spec again, and here are details about returning EPIPE from pages for 'send()', 'sendto()', 'sendmsg()': [EPIPE] The socket is shut down for writing, or the socket is connection-mode and is no longer connected. In the latter case, and if the socket is of type SOCK_STREAM, the SIGPIPE signal is generated to the calling thread So my opinion is that we need to send SIGPIPE only for SOCK_STREAM. Another question is how to interpret this from above (but again - SIGPIPE is related for SOCK_STREAM only): **" and is no longer connected"** IIUC, if we follow POSIX strictly, this check must be like: /* socket is shut down for writing or no longer connected. */ if (sk->sk_shutdown & SEND_SHUTDOWN || vsk->peer_shutdown & RCV_SHUTDOWN || sock_flag(SOCK_DONE)) { err = -EPIPE; goto out; } ... out: /* Handle -EPIPE for stream socket which is no longer connected. */ if (sk->sk_type == SOCK_STREAM && sock_flag(SOCK_DONE)) err = sk_stream_error(); From the other side, we can just follow TCP/AF_UNIX implementations as both are popular types of socket. In this case I suggest to implement this check like (e.g. without sock_flag(SOCK_DONE)): if (sk->sk_shutdown & SEND_SHUTDOWN || vsk->peer_shutdown & RCV_SHUTDOWN) { err = -EPIPE; goto out; } ... out: if (sk->sk_type == SOCK_STREAM) err = sk_stream_error(); What do you think? Thanks, Arseniy > >> >> 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? > > Thanks, > Stefano > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-14 19:46 ` Arseniy Krasnov @ 2023-08-22 9:39 ` Stefano Garzarella 2023-08-22 12:51 ` Arseniy Krasnov 0 siblings, 1 reply; 16+ messages in thread From: Stefano Garzarella @ 2023-08-22 9:39 UTC (permalink / raw) To: Arseniy Krasnov Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel On Mon, Aug 14, 2023 at 10:46:05PM +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@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. > >Update about sending SIGPIPE for SOCK_SEQPACKET, I checked POSIX doc and kernel sources more deeply: > > >1) > >I checked four types of sockets, which sends SIGPIPE for SOCK_SEQPACKET or not ('YES' if >this socket sends SIGPIPE in SOCK_SEQPACKET case): > >net/kcm/: YES >net/unix/: NO >net/sctp/: YES >net/caif/: NO > >Looking for this, I think it is impossible to get the right answer, as there is some >mess - everyone implements it as wish. Eheh, I had the same impression! > >2) > >I opened POSIX spec again, and here are details about returning EPIPE from pages >for 'send()', 'sendto()', 'sendmsg()': > >[EPIPE] The socket is shut down for writing, or the socket is connection-mode and is >no longer connected. In the latter case, and if the socket is of type >SOCK_STREAM, the SIGPIPE signal is generated to the calling thread > >So my opinion is that we need to send SIGPIPE only for SOCK_STREAM. Another question >is how to interpret this from above (but again - SIGPIPE is related for SOCK_STREAM >only): > >**" and is no longer connected"** > >IIUC, if we follow POSIX strictly, this check must be like: > >/* socket is shut down for writing or no longer connected. */ >if (sk->sk_shutdown & SEND_SHUTDOWN || > vsk->peer_shutdown & RCV_SHUTDOWN || > sock_flag(SOCK_DONE)) { > err = -EPIPE; > goto out; >} > >... > >out: > /* Handle -EPIPE for stream socket which is no longer connected. */ > if (sk->sk_type == SOCK_STREAM && > sock_flag(SOCK_DONE)) > err = sk_stream_error(); > > > >From the other side, we can just follow TCP/AF_UNIX implementations as both are >popular types of socket. In this case I suggest to implement this check like >(e.g. without sock_flag(SOCK_DONE)): > > >if (sk->sk_shutdown & SEND_SHUTDOWN || > vsk->peer_shutdown & RCV_SHUTDOWN) { > err = -EPIPE; > goto out; >} > >... > >out: > if (sk->sk_type == SOCK_STREAM) > err = sk_stream_error(); > >What do you think? I'd follow TCP/AF_UNIX implementations, but it is up to you ;-) Thanks, Stefano ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 1/2] vsock: send SIGPIPE on write to shutdowned socket 2023-08-22 9:39 ` Stefano Garzarella @ 2023-08-22 12:51 ` Arseniy Krasnov 0 siblings, 0 replies; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-22 12:51 UTC (permalink / raw) To: Stefano Garzarella Cc: Arseniy Krasnov, Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel On 22.08.2023 12:39, Stefano Garzarella wrote: > On Mon, Aug 14, 2023 at 10:46:05PM +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@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. >> >> Update about sending SIGPIPE for SOCK_SEQPACKET, I checked POSIX doc and kernel sources more deeply: >> >> >> 1) >> >> I checked four types of sockets, which sends SIGPIPE for SOCK_SEQPACKET or not ('YES' if >> this socket sends SIGPIPE in SOCK_SEQPACKET case): >> >> net/kcm/: YES >> net/unix/: NO >> net/sctp/: YES >> net/caif/: NO >> >> Looking for this, I think it is impossible to get the right answer, as there is some >> mess - everyone implements it as wish. > > Eheh, I had the same impression! > >> >> 2) >> >> I opened POSIX spec again, and here are details about returning EPIPE from pages >> for 'send()', 'sendto()', 'sendmsg()': >> >> [EPIPE] The socket is shut down for writing, or the socket is connection-mode and is >> no longer connected. In the latter case, and if the socket is of type >> SOCK_STREAM, the SIGPIPE signal is generated to the calling thread >> >> So my opinion is that we need to send SIGPIPE only for SOCK_STREAM. Another question >> is how to interpret this from above (but again - SIGPIPE is related for SOCK_STREAM >> only): >> >> **" and is no longer connected"** >> >> IIUC, if we follow POSIX strictly, this check must be like: >> >> /* socket is shut down for writing or no longer connected. */ >> if (sk->sk_shutdown & SEND_SHUTDOWN || >> vsk->peer_shutdown & RCV_SHUTDOWN || >> sock_flag(SOCK_DONE)) { >> err = -EPIPE; >> goto out; >> } >> >> ... >> >> out: >> /* Handle -EPIPE for stream socket which is no longer connected. */ >> if (sk->sk_type == SOCK_STREAM && >> sock_flag(SOCK_DONE)) >> err = sk_stream_error(); >> >> >> >> From the other side, we can just follow TCP/AF_UNIX implementations as both are >> popular types of socket. In this case I suggest to implement this check like >> (e.g. without sock_flag(SOCK_DONE)): >> >> >> if (sk->sk_shutdown & SEND_SHUTDOWN || >> vsk->peer_shutdown & RCV_SHUTDOWN) { >> err = -EPIPE; >> goto out; >> } >> >> ... >> >> out: >> if (sk->sk_type == SOCK_STREAM) >> err = sk_stream_error(); >> >> What do you think? > > I'd follow TCP/AF_UNIX implementations, but it is up to you ;-) Got it, I'll use this approach Thanks, Arseniy > > Thanks, > Stefano > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH v1 2/2] test/vsock: shutdowned socket test 2023-08-01 14:17 [RFC PATCH v1 0/2] vsock: handle writes to shutdowned socket Arseniy Krasnov 2023-08-01 14:17 ` [RFC PATCH v1 1/2] vsock: send SIGPIPE on write " Arseniy Krasnov @ 2023-08-01 14:17 ` Arseniy Krasnov 2023-08-02 8:00 ` Stefano Garzarella 1 sibling, 1 reply; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-01 14:17 UTC (permalink / raw) To: Stefan Hajnoczi, Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman Cc: kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa, avkrasnov, Arseniy Krasnov This adds two tests for 'shutdown()' call. It checks that SIGPIPE is sent when MSG_NOSIGNAL is not set and vice versa. Both flags SHUT_WR and SHUT_RD are tested. Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- tools/testing/vsock/vsock_test.c | 138 +++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c index 90718c2fd4ea..21d40a8d881c 100644 --- a/tools/testing/vsock/vsock_test.c +++ b/tools/testing/vsock/vsock_test.c @@ -19,6 +19,7 @@ #include <time.h> #include <sys/mman.h> #include <poll.h> +#include <signal.h> #include "timeout.h" #include "control.h" @@ -1170,6 +1171,133 @@ static void test_seqpacket_msg_peek_server(const struct test_opts *opts) return test_msg_peek_server(opts, true); } +static bool have_sigpipe; + +static void sigpipe(int signo) +{ + have_sigpipe = true; +} + +static void test_stream_check_sigpipe(int fd) +{ + ssize_t res; + + have_sigpipe = false; + + res = send(fd, "A", 1, 0); + if (res != -1) { + fprintf(stderr, "expected send(2) failure, got %zi\n", res); + exit(EXIT_FAILURE); + } + + if (!have_sigpipe) { + fprintf(stderr, "SIGPIPE expected\n"); + exit(EXIT_FAILURE); + } + + have_sigpipe = false; + + res = send(fd, "A", 1, MSG_NOSIGNAL); + if (res != -1) { + fprintf(stderr, "expected send(2) failure, got %zi\n", res); + exit(EXIT_FAILURE); + } + + if (have_sigpipe) { + fprintf(stderr, "SIGPIPE not expected\n"); + exit(EXIT_FAILURE); + } +} + +static void test_stream_shutwr_client(const struct test_opts *opts) +{ + int fd; + + struct sigaction act = { + .sa_handler = sigpipe, + }; + + sigaction(SIGPIPE, &act, NULL); + + fd = vsock_stream_connect(opts->peer_cid, 1234); + if (fd < 0) { + perror("connect"); + exit(EXIT_FAILURE); + } + + if (shutdown(fd, SHUT_WR)) { + perror("shutdown"); + exit(EXIT_FAILURE); + } + + test_stream_check_sigpipe(fd); + + control_writeln("CLIENTDONE"); + + close(fd); +} + +static void test_stream_shutwr_server(const struct test_opts *opts) +{ + int fd; + + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); + if (fd < 0) { + perror("accept"); + exit(EXIT_FAILURE); + } + + control_expectln("CLIENTDONE"); + + close(fd); +} + +static void test_stream_shutrd_client(const struct test_opts *opts) +{ + int fd; + + struct sigaction act = { + .sa_handler = sigpipe, + }; + + sigaction(SIGPIPE, &act, NULL); + + fd = vsock_stream_connect(opts->peer_cid, 1234); + if (fd < 0) { + perror("connect"); + exit(EXIT_FAILURE); + } + + control_expectln("SHUTRDDONE"); + + test_stream_check_sigpipe(fd); + + control_writeln("CLIENTDONE"); + + close(fd); +} + +static void test_stream_shutrd_server(const struct test_opts *opts) +{ + int fd; + + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); + if (fd < 0) { + perror("accept"); + exit(EXIT_FAILURE); + } + + if (shutdown(fd, SHUT_RD)) { + perror("shutdown"); + exit(EXIT_FAILURE); + } + + control_writeln("SHUTRDDONE"); + control_expectln("CLIENTDONE"); + + close(fd); +} + static struct test_case test_cases[] = { { .name = "SOCK_STREAM connection reset", @@ -1250,6 +1378,16 @@ static struct test_case test_cases[] = { .run_client = test_seqpacket_msg_peek_client, .run_server = test_seqpacket_msg_peek_server, }, + { + .name = "SOCK_STREAM SHUT_WR", + .run_client = test_stream_shutwr_client, + .run_server = test_stream_shutwr_server, + }, + { + .name = "SOCK_STREAM SHUT_RD", + .run_client = test_stream_shutrd_client, + .run_server = test_stream_shutrd_server, + }, {}, }; -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 2/2] test/vsock: shutdowned socket test 2023-08-01 14:17 ` [RFC PATCH v1 2/2] test/vsock: shutdowned socket test Arseniy Krasnov @ 2023-08-02 8:00 ` Stefano Garzarella 2023-08-04 12:48 ` Arseniy Krasnov 0 siblings, 1 reply; 16+ messages in thread From: Stefano Garzarella @ 2023-08-02 8:00 UTC (permalink / raw) To: Arseniy Krasnov Cc: Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel, oxffffaa On Tue, Aug 01, 2023 at 05:17:27PM +0300, Arseniy Krasnov wrote: >This adds two tests for 'shutdown()' call. It checks that SIGPIPE is >sent when MSG_NOSIGNAL is not set and vice versa. Both flags SHUT_WR >and SHUT_RD are tested. > >Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >--- > tools/testing/vsock/vsock_test.c | 138 +++++++++++++++++++++++++++++++ > 1 file changed, 138 insertions(+) > >diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >index 90718c2fd4ea..21d40a8d881c 100644 >--- a/tools/testing/vsock/vsock_test.c >+++ b/tools/testing/vsock/vsock_test.c >@@ -19,6 +19,7 @@ > #include <time.h> > #include <sys/mman.h> > #include <poll.h> >+#include <signal.h> > > #include "timeout.h" > #include "control.h" >@@ -1170,6 +1171,133 @@ static void test_seqpacket_msg_peek_server(const struct test_opts *opts) > return test_msg_peek_server(opts, true); > } > >+static bool have_sigpipe; ^ We should define it as `volatile sig_atomic_t`: the behavior is undefined if the signal handler refers to any object [CX] [Option Start] other than errno [Option End] with static storage duration other than by assigning a value to an object declared as volatile sig_atomic_t https://pubs.opengroup.org/onlinepubs/9699919799/functions/signal.html The rest LGTM! Thanks, Stefano >+ >+static void sigpipe(int signo) >+{ >+ have_sigpipe = true; >+} >+ >+static void test_stream_check_sigpipe(int fd) >+{ >+ ssize_t res; >+ >+ have_sigpipe = false; >+ >+ res = send(fd, "A", 1, 0); >+ if (res != -1) { >+ fprintf(stderr, "expected send(2) failure, got %zi\n", res); >+ exit(EXIT_FAILURE); >+ } >+ >+ if (!have_sigpipe) { >+ fprintf(stderr, "SIGPIPE expected\n"); >+ exit(EXIT_FAILURE); >+ } >+ >+ have_sigpipe = false; >+ >+ res = send(fd, "A", 1, MSG_NOSIGNAL); >+ if (res != -1) { >+ fprintf(stderr, "expected send(2) failure, got %zi\n", res); >+ exit(EXIT_FAILURE); >+ } >+ >+ if (have_sigpipe) { >+ fprintf(stderr, "SIGPIPE not expected\n"); >+ exit(EXIT_FAILURE); >+ } >+} >+ >+static void test_stream_shutwr_client(const struct test_opts *opts) >+{ >+ int fd; >+ >+ struct sigaction act = { >+ .sa_handler = sigpipe, >+ }; >+ >+ sigaction(SIGPIPE, &act, NULL); >+ >+ fd = vsock_stream_connect(opts->peer_cid, 1234); >+ if (fd < 0) { >+ perror("connect"); >+ exit(EXIT_FAILURE); >+ } >+ >+ if (shutdown(fd, SHUT_WR)) { >+ perror("shutdown"); >+ exit(EXIT_FAILURE); >+ } >+ >+ test_stream_check_sigpipe(fd); >+ >+ control_writeln("CLIENTDONE"); >+ >+ close(fd); >+} >+ >+static void test_stream_shutwr_server(const struct test_opts *opts) >+{ >+ int fd; >+ >+ fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >+ if (fd < 0) { >+ perror("accept"); >+ exit(EXIT_FAILURE); >+ } >+ >+ control_expectln("CLIENTDONE"); >+ >+ close(fd); >+} >+ >+static void test_stream_shutrd_client(const struct test_opts *opts) >+{ >+ int fd; >+ >+ struct sigaction act = { >+ .sa_handler = sigpipe, >+ }; >+ >+ sigaction(SIGPIPE, &act, NULL); >+ >+ fd = vsock_stream_connect(opts->peer_cid, 1234); >+ if (fd < 0) { >+ perror("connect"); >+ exit(EXIT_FAILURE); >+ } >+ >+ control_expectln("SHUTRDDONE"); >+ >+ test_stream_check_sigpipe(fd); >+ >+ control_writeln("CLIENTDONE"); >+ >+ close(fd); >+} >+ >+static void test_stream_shutrd_server(const struct test_opts *opts) >+{ >+ int fd; >+ >+ fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >+ if (fd < 0) { >+ perror("accept"); >+ exit(EXIT_FAILURE); >+ } >+ >+ if (shutdown(fd, SHUT_RD)) { >+ perror("shutdown"); >+ exit(EXIT_FAILURE); >+ } >+ >+ control_writeln("SHUTRDDONE"); >+ control_expectln("CLIENTDONE"); >+ >+ close(fd); >+} >+ > static struct test_case test_cases[] = { > { > .name = "SOCK_STREAM connection reset", >@@ -1250,6 +1378,16 @@ static struct test_case test_cases[] = { > .run_client = test_seqpacket_msg_peek_client, > .run_server = test_seqpacket_msg_peek_server, > }, >+ { >+ .name = "SOCK_STREAM SHUT_WR", >+ .run_client = test_stream_shutwr_client, >+ .run_server = test_stream_shutwr_server, >+ }, >+ { >+ .name = "SOCK_STREAM SHUT_RD", >+ .run_client = test_stream_shutrd_client, >+ .run_server = test_stream_shutrd_server, >+ }, > {}, > }; > >-- >2.25.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH v1 2/2] test/vsock: shutdowned socket test 2023-08-02 8:00 ` Stefano Garzarella @ 2023-08-04 12:48 ` Arseniy Krasnov 0 siblings, 0 replies; 16+ messages in thread From: Arseniy Krasnov @ 2023-08-04 12:48 UTC (permalink / raw) To: Stefano Garzarella, Arseniy Krasnov Cc: Stefan Hajnoczi, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Michael S. Tsirkin, Jason Wang, Bobby Eshleman, kvm, virtualization, netdev, linux-kernel, kernel On 02.08.2023 11:00, Stefano Garzarella wrote: > On Tue, Aug 01, 2023 at 05:17:27PM +0300, Arseniy Krasnov wrote: >> This adds two tests for 'shutdown()' call. It checks that SIGPIPE is >> sent when MSG_NOSIGNAL is not set and vice versa. Both flags SHUT_WR >> and SHUT_RD are tested. >> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >> --- >> tools/testing/vsock/vsock_test.c | 138 +++++++++++++++++++++++++++++++ >> 1 file changed, 138 insertions(+) >> >> diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c >> index 90718c2fd4ea..21d40a8d881c 100644 >> --- a/tools/testing/vsock/vsock_test.c >> +++ b/tools/testing/vsock/vsock_test.c >> @@ -19,6 +19,7 @@ >> #include <time.h> >> #include <sys/mman.h> >> #include <poll.h> >> +#include <signal.h> >> >> #include "timeout.h" >> #include "control.h" >> @@ -1170,6 +1171,133 @@ static void test_seqpacket_msg_peek_server(const struct test_opts *opts) >> return test_msg_peek_server(opts, true); >> } >> >> +static bool have_sigpipe; > ^ > We should define it as `volatile sig_atomic_t`: > > the behavior is undefined if the signal handler refers to any object > [CX] [Option Start] other than errno [Option End] with static storage > duration other than by assigning a value to an object declared as > volatile sig_atomic_t Yes, exactly, I forgot about sig_atomic_t. I'll fix it. Thanks, Arseniy > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/signal.html > > The rest LGTM! > > Thanks, > Stefano > >> + >> +static void sigpipe(int signo) >> +{ >> + have_sigpipe = true; >> +} >> + >> +static void test_stream_check_sigpipe(int fd) >> +{ >> + ssize_t res; >> + >> + have_sigpipe = false; >> + >> + res = send(fd, "A", 1, 0); >> + if (res != -1) { >> + fprintf(stderr, "expected send(2) failure, got %zi\n", res); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (!have_sigpipe) { >> + fprintf(stderr, "SIGPIPE expected\n"); >> + exit(EXIT_FAILURE); >> + } >> + >> + have_sigpipe = false; >> + >> + res = send(fd, "A", 1, MSG_NOSIGNAL); >> + if (res != -1) { >> + fprintf(stderr, "expected send(2) failure, got %zi\n", res); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (have_sigpipe) { >> + fprintf(stderr, "SIGPIPE not expected\n"); >> + exit(EXIT_FAILURE); >> + } >> +} >> + >> +static void test_stream_shutwr_client(const struct test_opts *opts) >> +{ >> + int fd; >> + >> + struct sigaction act = { >> + .sa_handler = sigpipe, >> + }; >> + >> + sigaction(SIGPIPE, &act, NULL); >> + >> + fd = vsock_stream_connect(opts->peer_cid, 1234); >> + if (fd < 0) { >> + perror("connect"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (shutdown(fd, SHUT_WR)) { >> + perror("shutdown"); >> + exit(EXIT_FAILURE); >> + } >> + >> + test_stream_check_sigpipe(fd); >> + >> + control_writeln("CLIENTDONE"); >> + >> + close(fd); >> +} >> + >> +static void test_stream_shutwr_server(const struct test_opts *opts) >> +{ >> + int fd; >> + >> + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >> + if (fd < 0) { >> + perror("accept"); >> + exit(EXIT_FAILURE); >> + } >> + >> + control_expectln("CLIENTDONE"); >> + >> + close(fd); >> +} >> + >> +static void test_stream_shutrd_client(const struct test_opts *opts) >> +{ >> + int fd; >> + >> + struct sigaction act = { >> + .sa_handler = sigpipe, >> + }; >> + >> + sigaction(SIGPIPE, &act, NULL); >> + >> + fd = vsock_stream_connect(opts->peer_cid, 1234); >> + if (fd < 0) { >> + perror("connect"); >> + exit(EXIT_FAILURE); >> + } >> + >> + control_expectln("SHUTRDDONE"); >> + >> + test_stream_check_sigpipe(fd); >> + >> + control_writeln("CLIENTDONE"); >> + >> + close(fd); >> +} >> + >> +static void test_stream_shutrd_server(const struct test_opts *opts) >> +{ >> + int fd; >> + >> + fd = vsock_stream_accept(VMADDR_CID_ANY, 1234, NULL); >> + if (fd < 0) { >> + perror("accept"); >> + exit(EXIT_FAILURE); >> + } >> + >> + if (shutdown(fd, SHUT_RD)) { >> + perror("shutdown"); >> + exit(EXIT_FAILURE); >> + } >> + >> + control_writeln("SHUTRDDONE"); >> + control_expectln("CLIENTDONE"); >> + >> + close(fd); >> +} >> + >> static struct test_case test_cases[] = { >> { >> .name = "SOCK_STREAM connection reset", >> @@ -1250,6 +1378,16 @@ static struct test_case test_cases[] = { >> .run_client = test_seqpacket_msg_peek_client, >> .run_server = test_seqpacket_msg_peek_server, >> }, >> + { >> + .name = "SOCK_STREAM SHUT_WR", >> + .run_client = test_stream_shutwr_client, >> + .run_server = test_stream_shutwr_server, >> + }, >> + { >> + .name = "SOCK_STREAM SHUT_RD", >> + .run_client = test_stream_shutrd_client, >> + .run_server = test_stream_shutrd_server, >> + }, >> {}, >> }; >> >> -- >> 2.25.1 >> > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-08-22 12:57 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-01 14:17 [RFC PATCH v1 0/2] vsock: handle writes to shutdowned socket Arseniy Krasnov 2023-08-01 14:17 ` [RFC PATCH v1 1/2] vsock: send SIGPIPE on write " Arseniy Krasnov 2023-08-02 7:46 ` Stefano Garzarella 2023-08-04 12:46 ` Arseniy Krasnov 2023-08-04 14:28 ` Stefano Garzarella 2023-08-04 14:34 ` Arseniy Krasnov 2023-08-04 15:02 ` Stefano Garzarella 2023-08-04 16:24 ` Arseniy Krasnov 2023-08-14 19:40 ` Arseniy Krasnov 2023-08-22 9:36 ` Stefano Garzarella 2023-08-14 19:46 ` Arseniy Krasnov 2023-08-22 9:39 ` Stefano Garzarella 2023-08-22 12:51 ` Arseniy Krasnov 2023-08-01 14:17 ` [RFC PATCH v1 2/2] test/vsock: shutdowned socket test Arseniy Krasnov 2023-08-02 8:00 ` Stefano Garzarella 2023-08-04 12:48 ` Arseniy Krasnov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).