From: Jiyong Park <jiyong@google.com>
To: Stefano Garzarella <sgarzare@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
Jason Wang <jasowang@redhat.com>,
adelva@google.com, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vhost/vsock: reset only the h2g connections upon release
Date: Thu, 10 Mar 2022 19:41:54 +0900 [thread overview]
Message-ID: <CALeUXe6heGD9J+5fkLs9TJ7Mn0UT=BSdGNK_wZ4gkor_Ax_SqA@mail.gmail.com> (raw)
In-Reply-To: <20220310085931.cpgc2cv4yg7sd4vu@sgarzare-redhat>
Hi Stefano,
On Thu, Mar 10, 2022 at 5:59 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Hi Jiyong,
>
> On Thu, Mar 10, 2022 at 05:18:54PM +0900, Jiyong Park wrote:
> >Filtering non-h2g connections out when determining orphaned connections.
> >Otherwise, in a nested VM configuration, destroying the nested VM (which
> >often involves the closing of /dev/vhost-vsock if there was h2g
> >connections to the nested VM) kills not only the h2g connections, but
> >also all existing g2h connections to the (outmost) host which are
> >totally unrelated.
> >
> >Tested: Executed the following steps on Cuttlefish (Android running on a
> >VM) [1]: (1) Enter into an `adb shell` session - to have a g2h
> >connection inside the VM, (2) open and then close /dev/vhost-vsock by
> >`exec 3< /dev/vhost-vsock && exec 3<&-`, (3) observe that the adb
> >session is not reset.
> >
> >[1] https://android.googlesource.com/device/google/cuttlefish/
> >
> >Signed-off-by: Jiyong Park <jiyong@google.com>
> >---
> > drivers/vhost/vsock.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> >index 37f0b4274113..2f6d5d66f5ed 100644
> >--- a/drivers/vhost/vsock.c
> >+++ b/drivers/vhost/vsock.c
> >@@ -722,6 +722,10 @@ static void vhost_vsock_reset_orphans(struct sock *sk)
> > * executing.
> > */
> >
> >+ /* Only the h2g connections are reset */
> >+ if (vsk->transport != &vhost_transport.transport)
> >+ return;
> >+
> > /* If the peer is still valid, no need to reset connection */
> > if (vhost_vsock_get(vsk->remote_addr.svm_cid))
> > return;
> >--
> >2.35.1.723.g4982287a31-goog
> >
>
> Thanks for your patch!
>
> Yes, I see the problem and I think I introduced it with the
> multi-transports support (ooops).
>
> So we should add this fixes tag:
>
> Fixes: c0cfa2d8a788 ("vsock: add multi-transports support")
>
>
> IIUC the problem is for all transports that should only cycle on their
> own sockets. Indeed I think there is the same problem if the g2h driver
> will be unloaded (or a reset event is received after a VM migration), it
> will close all sockets of the nested h2g.
>
> So I suggest a more generic solution, modifying
> vsock_for_each_connected_socket() like this (not tested):
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 38baeb189d4e..f04abf662ec6 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -334,7 +334,8 @@ void vsock_remove_sock(struct vsock_sock *vsk)
> }
> EXPORT_SYMBOL_GPL(vsock_remove_sock);
>
> -void vsock_for_each_connected_socket(void (*fn)(struct sock *sk))
> +void vsock_for_each_connected_socket(struct vsock_transport *transport,
> + void (*fn)(struct sock *sk))
> {
> int i;
>
> @@ -343,8 +344,12 @@ void vsock_for_each_connected_socket(void (*fn)(struct sock *sk))
> for (i = 0; i < ARRAY_SIZE(vsock_connected_table); i++) {
> struct vsock_sock *vsk;
> list_for_each_entry(vsk, &vsock_connected_table[i],
> - connected_table)
> + connected_table) {
> + if (vsk->transport != transport)
> + continue;
> +
> fn(sk_vsock(vsk));
> + }
> }
>
>
> And all transports that call it.
>
> Thanks,
> Stefano
>
Thanks for the suggestion, which looks much better. It actually worked well.
By the way, the suggested change will alter the kernel-module interface (KMI),
which will make it difficult to land the change on older releases where we'd
like to keep the KMI stable [1]. Would it be OK if we let the supplied function
(fn) be responsible for checking the transport? I think that there, in
the future,
might be a case where one needs to cycle over all sockets for inspection or so.
I admit that this would be prone to error, though.
Please let me know what you think. I don't have a strong preference. I will
submit a revision as you want.
[1] https://source.android.com/devices/architecture/kernel/generic-kernel-image#kmi-stability
next prev parent reply other threads:[~2022-03-10 10:42 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-10 8:18 [PATCH] vhost/vsock: reset only the h2g connections upon release Jiyong Park
2022-03-10 8:59 ` Stefano Garzarella
2022-03-10 10:41 ` Jiyong Park [this message]
2022-03-10 11:00 ` Stefano Garzarella
2022-03-10 12:01 ` Jiyong Park
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='CALeUXe6heGD9J+5fkLs9TJ7Mn0UT=BSdGNK_wZ4gkor_Ax_SqA@mail.gmail.com' \
--to=jiyong@google.com \
--cc=adelva@google.com \
--cc=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=sgarzare@redhat.com \
--cc=stefanha@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/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).