From: Stefano Garzarella <sgarzare@redhat.com>
To: "Longpeng(Mike)" <longpeng2@huawei.com>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
arei.gonglei@huawei.com, subo7@huawei.com,
"David S . Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Jorgen Hansen <jhansen@vmware.com>,
Norbert Slusarek <nslusarek@gmx.net>,
Andra Paraschiv <andraprs@amazon.com>,
Colin Ian King <colin.king@canonical.com>,
David Brazdil <dbrazdil@google.com>,
Alexander Popov <alex.popov@linux.com>,
lixianming <lixianming5@huawei.com>
Subject: Re: [RFC] vsock: notify server to shutdown when client has pending signal
Date: Thu, 13 May 2021 11:41:43 +0200 [thread overview]
Message-ID: <20210513094143.pir5vzsludut3xdc@steredhat> (raw)
In-Reply-To: <20210511094127.724-1-longpeng2@huawei.com>
Hi,
thanks for this patch, comments below...
On Tue, May 11, 2021 at 05:41:27PM +0800, Longpeng(Mike) wrote:
>The client's sk_state will be set to TCP_ESTABLISHED if the
>server replay the client's connect request.
>However, if the client has pending signal, its sk_state will
>be set to TCP_CLOSE without notify the server, so the server
>will hold the corrupt connection.
>
> client server
>
>1. sk_state=TCP_SYN_SENT |
>2. call ->connect() |
>3. wait reply |
> | 4. sk_state=TCP_ESTABLISHED
> | 5. insert to connected list
> | 6. reply to the client
>7. sk_state=TCP_ESTABLISHED |
>8. insert to connected list |
>9. *signal pending* <--------------------- the user kill client
>10. sk_state=TCP_CLOSE |
>client is exiting... |
>11. call ->release() |
> virtio_transport_close
> if (!(sk->sk_state == TCP_ESTABLISHED ||
> sk->sk_state == TCP_CLOSING))
> return true; <------------- return at here
>As a result, the server cannot notice the connection is corrupt.
>So the client should notify the peer in this case.
>
>Cc: David S. Miller <davem@davemloft.net>
>Cc: Jakub Kicinski <kuba@kernel.org>
>Cc: Stefano Garzarella <sgarzare@redhat.com>
>Cc: Jorgen Hansen <jhansen@vmware.com>
>Cc: Norbert Slusarek <nslusarek@gmx.net>
>Cc: Andra Paraschiv <andraprs@amazon.com>
>Cc: Colin Ian King <colin.king@canonical.com>
>Cc: David Brazdil <dbrazdil@google.com>
>Cc: Alexander Popov <alex.popov@linux.com>
>Signed-off-by: lixianming <lixianming5@huawei.com>
>Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
>---
> net/vmw_vsock/af_vsock.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index 92a72f0..d5df908 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -1368,6 +1368,7 @@ static int vsock_stream_connect(struct socket *sock, struct sockaddr *addr,
> lock_sock(sk);
>
> if (signal_pending(current)) {
>+ vsock_send_shutdown(sk, SHUTDOWN_MASK);
I see the issue, but I'm not sure is okay to send the shutdown in any
case, think about the server didn't setup the connection.
Maybe is better to set TCP_CLOSING if the socket state was
TCP_ESTABLISHED, so the shutdown will be handled by the
transport->release() as usual.
What do you think?
Anyway, also without the patch, the server should receive a RST if it
sends any data to the client, but of course, is better to let it know
the socket is closed in advance.
Thanks,
Stefano
next prev parent reply other threads:[~2021-05-13 9:41 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-11 9:41 [RFC] vsock: notify server to shutdown when client has pending signal Longpeng(Mike)
2021-05-13 9:41 ` Stefano Garzarella [this message]
2021-05-13 10:35 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2021-05-17 2:18 ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
2021-05-17 10:10 ` Stefano Garzarella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210513094143.pir5vzsludut3xdc@steredhat \
--to=sgarzare@redhat.com \
--cc=alex.popov@linux.com \
--cc=andraprs@amazon.com \
--cc=arei.gonglei@huawei.com \
--cc=colin.king@canonical.com \
--cc=davem@davemloft.net \
--cc=dbrazdil@google.com \
--cc=jhansen@vmware.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lixianming5@huawei.com \
--cc=longpeng2@huawei.com \
--cc=netdev@vger.kernel.org \
--cc=nslusarek@gmx.net \
--cc=subo7@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).