linux-kernel - Re: [PATCH] vhost/vsock: reset only the h2g connections upon release

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALeUXe6heGD9J+5fkLs9TJ7Mn0UT=BSdGNK_wZ4gkor_Ax_SqA@mail.gmail.com>
Date:   Thu, 10 Mar 2022 19:41:54 +0900
From:   Jiyong Park <jiyong@...gle.com>
To:     Stefano Garzarella <sgarzare@...hat.com>
Cc:     Stefan Hajnoczi <stefanha@...hat.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>, adelva@...gle.com,
        kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vhost/vsock: reset only the h2g connections upon release

Hi Stefano,

On Thu, Mar 10, 2022 at 5:59 PM Stefano Garzarella <sgarzare@...hat.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@...gle.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