[PATCH 0/2] Move net backend cleanup to NIC cleanup

[PATCH 0/2] Move net backend cleanup to NIC cleanup

[Eugenio Pérez]

Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
structures if peer nic is present") effectively delayed the backend
cleanup, allowing the frontend or the guest to access it resources as
long as the frontend NIC is still visible to the guest.

However it does not clean up the resources until the qemu process is
over.  This causes an effective leak if the device is deleted with
device_del, as there is no way to close the vdpa device.  This makes
impossible to re-add that device to this or other QEMU instances until
the first instance of QEMU is finished.

Move the cleanup from qemu_cleanup to the NIC deletion.

Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
Acked-by: Jason Wang <jasowang@redhat.com>
Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>

Eugenio Pérez (2):
  net: parameterize the removing client from nc list
  net: move backend cleanup to NIC cleanup

 net/net.c        | 30 ++++++++++++++++++++----------
 net/vhost-vdpa.c |  8 --------
 2 files changed, 20 insertions(+), 18 deletions(-)

-- 
2.39.3

[PATCH 1/2] net: parameterize the removing client from nc list

[Eugenio Pérez]

This change is used in later commits so we can avoid the removal of the
netclient if it is delayed.

No functional change intended.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 net/net.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/net.c b/net/net.c
index 0520bc1681..11e19d3bed 100644
--- a/net/net.c
+++ b/net/net.c
@@ -375,9 +375,12 @@ NetClientState *qemu_get_peer(NetClientState *nc, int queue_index)
     return ncs->peer;
 }
 
-static void qemu_cleanup_net_client(NetClientState *nc)
+static void qemu_cleanup_net_client(NetClientState *nc,
+                                    bool remove_from_net_clients)
 {
-    QTAILQ_REMOVE(&net_clients, nc, next);
+    if (remove_from_net_clients) {
+        QTAILQ_REMOVE(&net_clients, nc, next);
+    }
 
     if (nc->info->cleanup) {
         nc->info->cleanup(nc);
@@ -436,14 +439,14 @@ void qemu_del_net_client(NetClientState *nc)
         }
 
         for (i = 0; i < queues; i++) {
-            qemu_cleanup_net_client(ncs[i]);
+            qemu_cleanup_net_client(ncs[i], true);
         }
 
         return;
     }
 
     for (i = 0; i < queues; i++) {
-        qemu_cleanup_net_client(ncs[i]);
+        qemu_cleanup_net_client(ncs[i], true);
         qemu_free_net_client(ncs[i]);
     }
 }
@@ -468,7 +471,7 @@ void qemu_del_nic(NICState *nic)
     for (i = queues - 1; i >= 0; i--) {
         NetClientState *nc = qemu_get_subqueue(nic, i);
 
-        qemu_cleanup_net_client(nc);
+        qemu_cleanup_net_client(nc, true);
         qemu_free_net_client(nc);
     }
 
-- 
2.39.3

[PATCH 2/2] net: move backend cleanup to NIC cleanup

[Eugenio Pérez]

Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
structures if peer nic is present") effectively delayed the backend
cleanup, allowing the frontend or the guest to access it resources as
long as the frontend is still visible to the guest.

However it does not clean up the resources until the qemu process is
over.  This causes an effective leak if the device is deleted with
device_del, as there is no way to close the vdpa device.  This makes
impossible to re-add that device to this or other QEMU instances until
the first instance of QEMU is finished.

Move the cleanup from qemu_cleanup to the NIC deletion.

Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
Acked-by: Jason Wang <jasowang@redhat.com>
Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
Carring the Acked-by Jason as it was given when I proposed this one year
ago to qemu-security@nongnu.org off list, trying to solve CVE-2023-3301.
---
 net/net.c        | 19 +++++++++++++------
 net/vhost-vdpa.c |  8 --------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/net/net.c b/net/net.c
index 11e19d3bed..fb6e130c62 100644
--- a/net/net.c
+++ b/net/net.c
@@ -422,7 +422,13 @@ void qemu_del_net_client(NetClientState *nc)
         object_unparent(OBJECT(nf));
     }
 
-    /* If there is a peer NIC, delete and cleanup client, but do not free. */
+    /*
+     * If there is a peer NIC, transfer ownership to it.  Delete the client
+     * from net_client list but do not cleanup nor free.  This way NIC can
+     * still access to members of the backend.
+     *
+     * The cleanup and free will be done when the NIC is free.
+     */
     if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_NIC) {
         NICState *nic = qemu_get_nic(nc->peer);
         if (nic->peer_deleted) {
@@ -432,16 +438,13 @@ void qemu_del_net_client(NetClientState *nc)
 
         for (i = 0; i < queues; i++) {
             ncs[i]->peer->link_down = true;
+            QTAILQ_REMOVE(&net_clients, ncs[i], next);
         }
 
         if (nc->peer->info->link_status_changed) {
             nc->peer->info->link_status_changed(nc->peer);
         }
 
-        for (i = 0; i < queues; i++) {
-            qemu_cleanup_net_client(ncs[i], true);
-        }
-
         return;
     }
 
@@ -459,8 +462,12 @@ void qemu_del_nic(NICState *nic)
 
     for (i = 0; i < queues; i++) {
         NetClientState *nc = qemu_get_subqueue(nic, i);
-        /* If this is a peer NIC and peer has already been deleted, free it now. */
+        /*
+         * If this is a peer NIC and peer has already been deleted, clean it up
+         * and free it now.
+         */
         if (nic->peer_deleted) {
+            qemu_cleanup_net_client(nc->peer, false);
             qemu_free_net_client(nc->peer);
         } else if (nc->peer) {
             /* if there are RX packets pending, complete them */
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 3726ee5d67..64825136a3 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -221,14 +221,6 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
 
-    /*
-     * If a peer NIC is attached, do not cleanup anything.
-     * Cleanup will happen as a part of qemu_cleanup() -> net_cleanup()
-     * when the guest is shutting down.
-     */
-    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_NIC) {
-        return;
-    }
     munmap(s->cvq_cmd_out_buffer, vhost_vdpa_net_cvq_cmd_page_len());
     munmap(s->status, vhost_vdpa_net_cvq_cmd_page_len());
     if (s->vhost_net) {
-- 
2.39.3

Re: [PATCH 0/2] Move net backend cleanup to NIC cleanup

[Jason Wang]

On Mon, Jan 29, 2024 at 9:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
> structures if peer nic is present") effectively delayed the backend
> cleanup, allowing the frontend or the guest to access it resources as
> long as the frontend NIC is still visible to the guest.
>
> However it does not clean up the resources until the qemu process is
> over.  This causes an effective leak if the device is deleted with
> device_del, as there is no way to close the vdpa device.  This makes
> impossible to re-add that device to this or other QEMU instances until
> the first instance of QEMU is finished.
>
> Move the cleanup from qemu_cleanup to the NIC deletion.
>
> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> Acked-by: Jason Wang <jasowang@redhat.com>
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> Eugenio Pérez (2):
>   net: parameterize the removing client from nc list
>   net: move backend cleanup to NIC cleanup
>
>  net/net.c        | 30 ++++++++++++++++++++----------
>  net/vhost-vdpa.c |  8 --------
>  2 files changed, 20 insertions(+), 18 deletions(-)
>
> --

Queued.

Thanks

Re: [PATCH 0/2] Move net backend cleanup to NIC cleanup

[Si-Wei Liu]

Hi Jason,

It seems this series wasn't applied successfully, I still cannot see it 
from the latest tree. Any idea?

In any case the fix LGTM.

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>

Thanks,
-Siwei

On 1/31/2024 9:43 PM, Jason Wang wrote:
> On Mon, Jan 29, 2024 at 9:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>> Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
>> structures if peer nic is present") effectively delayed the backend
>> cleanup, allowing the frontend or the guest to access it resources as
>> long as the frontend NIC is still visible to the guest.
>>
>> However it does not clean up the resources until the qemu process is
>> over.  This causes an effective leak if the device is deleted with
>> device_del, as there is no way to close the vdpa device.  This makes
>> impossible to re-add that device to this or other QEMU instances until
>> the first instance of QEMU is finished.
>>
>> Move the cleanup from qemu_cleanup to the NIC deletion.
>>
>> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
>> Acked-by: Jason Wang <jasowang@redhat.com>
>> Reported-by: Lei Yang <leiyang@redhat.com>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>
>> Eugenio Pérez (2):
>>    net: parameterize the removing client from nc list
>>    net: move backend cleanup to NIC cleanup
>>
>>   net/net.c        | 30 ++++++++++++++++++++----------
>>   net/vhost-vdpa.c |  8 --------
>>   2 files changed, 20 insertions(+), 18 deletions(-)
>>
>> --
> Queued.
>
> Thanks
>

Re: [PATCH 0/2] Move net backend cleanup to NIC cleanup

[Jason Wang]

On Tue, Sep 10, 2024 at 11:41 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
> Hi Jason,
>
> It seems this series wasn't applied successfully, I still cannot see it
> from the latest tree. Any idea?

It breaks make check.

Eugenio, would you want to fix and resend the series?

Thanks

>
> In any case the fix LGTM.
>
> Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
> Thanks,
> -Siwei
>
> On 1/31/2024 9:43 PM, Jason Wang wrote:
> > On Mon, Jan 29, 2024 at 9:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >> Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
> >> structures if peer nic is present") effectively delayed the backend
> >> cleanup, allowing the frontend or the guest to access it resources as
> >> long as the frontend NIC is still visible to the guest.
> >>
> >> However it does not clean up the resources until the qemu process is
> >> over.  This causes an effective leak if the device is deleted with
> >> device_del, as there is no way to close the vdpa device.  This makes
> >> impossible to re-add that device to this or other QEMU instances until
> >> the first instance of QEMU is finished.
> >>
> >> Move the cleanup from qemu_cleanup to the NIC deletion.
> >>
> >> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> >> Acked-by: Jason Wang <jasowang@redhat.com>
> >> Reported-by: Lei Yang <leiyang@redhat.com>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>
> >> Eugenio Pérez (2):
> >>    net: parameterize the removing client from nc list
> >>    net: move backend cleanup to NIC cleanup
> >>
> >>   net/net.c        | 30 ++++++++++++++++++++----------
> >>   net/vhost-vdpa.c |  8 --------
> >>   2 files changed, 20 insertions(+), 18 deletions(-)
> >>
> >> --
> > Queued.
> >
> > Thanks
> >
>

Re: [PATCH 0/2] Move net backend cleanup to NIC cleanup

[Eugenio Perez Martin]

On Tue, Sep 10, 2024 at 5:46 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Tue, Sep 10, 2024 at 11:41 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >
> > Hi Jason,
> >
> > It seems this series wasn't applied successfully, I still cannot see it
> > from the latest tree. Any idea?
>
> It breaks make check.
>
> Eugenio, would you want to fix and resend the series?
>

I'm trying to reproduce but with no luck :(.

For the record this is the failed log. Is it possible to try to
reproduce it again in the machine / env it crashed?

▶  10/354 ERROR:../tests/qtest/qos-test.
c:191:subprocess_run_one_test:
child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess
[1494462]) failed unexpectedly ERROR
 10/354 qemu:qtest+qtest-x86_64 / qtest-x86_64/qos-test
        ERROR           14.19s   killed by signal 6 SIGABRT
>>> PYTHON=/home/devel/git/qemu/build/pyvenv/bin/python3 G_TEST_DBUS_DAEMON=/home/devel/git/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-x86_64 MALLOC_PERTURB_=82 /home/devel/git/qemu/build/tests/qtest/qos-test --tap -k
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
✀  ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
Vhost user backend fails to broadcast fake RARP
../tests/qtest/libqtest.c:204: kill_qemu() detected QEMU death from
signal 11 (Segmentation fault) (core dumped)
../tests/qtest/libqtest.c:204: kill_qemu() detected QEMU death from
signal 11 (Segmentation fault) (core dumped)
**
ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child
process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess
[1494462]) failed unexpectedly
> Thanks
>
> >
> > In any case the fix LGTM.
> >
> > Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >
> > Thanks,
> > -Siwei
> >
> > On 1/31/2024 9:43 PM, Jason Wang wrote:
> > > On Mon, Jan 29, 2024 at 9:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >> Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
> > >> structures if peer nic is present") effectively delayed the backend
> > >> cleanup, allowing the frontend or the guest to access it resources as
> > >> long as the frontend NIC is still visible to the guest.
> > >>
> > >> However it does not clean up the resources until the qemu process is
> > >> over.  This causes an effective leak if the device is deleted with
> > >> device_del, as there is no way to close the vdpa device.  This makes
> > >> impossible to re-add that device to this or other QEMU instances until
> > >> the first instance of QEMU is finished.
> > >>
> > >> Move the cleanup from qemu_cleanup to the NIC deletion.
> > >>
> > >> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> > >> Acked-by: Jason Wang <jasowang@redhat.com>
> > >> Reported-by: Lei Yang <leiyang@redhat.com>
> > >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >>
> > >> Eugenio Pérez (2):
> > >>    net: parameterize the removing client from nc list
> > >>    net: move backend cleanup to NIC cleanup
> > >>
> > >>   net/net.c        | 30 ++++++++++++++++++++----------
> > >>   net/vhost-vdpa.c |  8 --------
> > >>   2 files changed, 20 insertions(+), 18 deletions(-)
> > >>
> > >> --
> > > Queued.
> > >
> > > Thanks
> > >
> >
>

Re: [PATCH 0/2] Move net backend cleanup to NIC cleanup

[Eugenio Perez Martin]

On Wed, Sep 11, 2024 at 11:04 AM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Tue, Sep 10, 2024 at 5:46 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 11:41 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> > >
> > > Hi Jason,
> > >
> > > It seems this series wasn't applied successfully, I still cannot see it
> > > from the latest tree. Any idea?
> >
> > It breaks make check.
> >
> > Eugenio, would you want to fix and resend the series?
> >
>
> I'm trying to reproduce but with no luck :(.
>

I'm able to reproduce consistently now.

> For the record this is the failed log. Is it possible to try to
> reproduce it again in the machine / env it crashed?
>
> ▶  10/354 ERROR:../tests/qtest/qos-test.
> c:191:subprocess_run_one_test:
> child process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess
> [1494462]) failed unexpectedly ERROR
>  10/354 qemu:qtest+qtest-x86_64 / qtest-x86_64/qos-test
>         ERROR           14.19s   killed by signal 6 SIGABRT
> >>> PYTHON=/home/devel/git/qemu/build/pyvenv/bin/python3 G_TEST_DBUS_DAEMON=/home/devel/git/qemu/tests/dbus-vmstate-daemon.sh QTEST_QEMU_STORAGE_DAEMON_BINARY=./storage-daemon/qemu-storage-daemon QTEST_QEMU_IMG=./qemu-img QTEST_QEMU_BINARY=./qemu-system-x86_64 MALLOC_PERTURB_=82 /home/devel/git/qemu/build/tests/qtest/qos-test --tap -k
> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> ✀  ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> Vhost user backend fails to broadcast fake RARP
> ../tests/qtest/libqtest.c:204: kill_qemu() detected QEMU death from
> signal 11 (Segmentation fault) (core dumped)
> ../tests/qtest/libqtest.c:204: kill_qemu() detected QEMU death from
> signal 11 (Segmentation fault) (core dumped)
> **
> ERROR:../tests/qtest/qos-test.c:191:subprocess_run_one_test: child
> process (/x86_64/pc/i440FX-pcihost/pci-bus-pc/pci-bus/virtio-net-pci/virtio-net/virtio-net-tests/vhost-user/migrate/subprocess
> [1494462]) failed unexpectedly
> > Thanks
> >
> > >
> > > In any case the fix LGTM.
> > >
> > > Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > >
> > > Thanks,
> > > -Siwei
> > >
> > > On 1/31/2024 9:43 PM, Jason Wang wrote:
> > > > On Mon, Jan 29, 2024 at 9:24 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > > >> Commit a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net
> > > >> structures if peer nic is present") effectively delayed the backend
> > > >> cleanup, allowing the frontend or the guest to access it resources as
> > > >> long as the frontend NIC is still visible to the guest.
> > > >>
> > > >> However it does not clean up the resources until the qemu process is
> > > >> over.  This causes an effective leak if the device is deleted with
> > > >> device_del, as there is no way to close the vdpa device.  This makes
> > > >> impossible to re-add that device to this or other QEMU instances until
> > > >> the first instance of QEMU is finished.
> > > >>
> > > >> Move the cleanup from qemu_cleanup to the NIC deletion.
> > > >>
> > > >> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> > > >> Acked-by: Jason Wang <jasowang@redhat.com>
> > > >> Reported-by: Lei Yang <leiyang@redhat.com>
> > > >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > >>
> > > >> Eugenio Pérez (2):
> > > >>    net: parameterize the removing client from nc list
> > > >>    net: move backend cleanup to NIC cleanup
> > > >>
> > > >>   net/net.c        | 30 ++++++++++++++++++++----------
> > > >>   net/vhost-vdpa.c |  8 --------
> > > >>   2 files changed, 20 insertions(+), 18 deletions(-)
> > > >>
> > > >> --
> > > > Queued.
> > > >
> > > > Thanks
> > > >
> > >
> >