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

[PATCH v2 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.

v2:
Remove NIC peer also at net_cleanup. vhost-user trust all the
backends are clean before qemu removes char devices.

This is not a requisite introduced by this commit as
system/runstate.c:qemu_cleanup shows.

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

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

-- 
2.46.0

[PATCH v2 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.

Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
Acked-by: Jason Wang <jasowang@redhat.com>
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 fc1125111c..9f96509b2a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -381,9 +381,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);
@@ -442,14 +445,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]);
     }
 }
@@ -474,7 +477,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.46.0

[PATCH v2 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 and to
net_cleanup.

Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
Reported-by: Lei Yang <leiyang@redhat.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
v2: Remove NIC peer also at net_cleanup. vhost-user trust all the
backends are clean before qemu removes char devices.

This is not a requisite introduced by this commit as
system/runstate.c:qemu_cleanup shows.
---
 net/net.c        | 33 +++++++++++++++++++++++++++------
 net/vhost-vdpa.c |  8 --------
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/net/net.c b/net/net.c
index 9f96509b2a..d097ac3e74 100644
--- a/net/net.c
+++ b/net/net.c
@@ -428,7 +428,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) {
@@ -438,16 +444,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;
     }

@@ -465,8 +468,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 */
@@ -1686,6 +1693,9 @@ void net_cleanup(void)
      * of the latest NET_CLIENT_DRIVER_NIC, and operate on *p as we walk
      * the list.
      *
+     * However, the NIC may have peers that trust to be clean beyond this
+     * point.  For example, if they have been removed with device_del.
+     *
      * The 'nc' variable isn't part of the list traversal; it's purely
      * for convenience as too much '(*p)->' has a tendency to make the
      * readers' eyes bleed.
@@ -1693,6 +1703,17 @@ void net_cleanup(void)
     while (*p) {
         nc = *p;
         if (nc->info->type == NET_CLIENT_DRIVER_NIC) {
+            NICState *nic = qemu_get_nic(nc);
+
+            if (nic->peer_deleted) {
+                int queues = MAX(nic->conf->peers.queues, 1);
+
+                for (int i = 0; i < queues; i++) {
+                    NetClientState *nc = qemu_get_subqueue(nic, i);
+                    qemu_cleanup_net_client(nc->peer, false);
+                }
+            }
+
             /* Skip NET_CLIENT_DRIVER_NIC entries */
             p = &QTAILQ_NEXT(nc, next);
         } else {
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 46b02c50be..e1a1c91e48 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -224,14 +224,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.46.0

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

[Si-Wei Liu]

On 9/12/2024 9:54 AM, Eugenio Pérez 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 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 and to
> net_cleanup.
>
> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
> v2: Remove NIC peer also at net_cleanup. vhost-user trust all the
> backends are clean before qemu removes char devices.
>
> This is not a requisite introduced by this commit as
> system/runstate.c:qemu_cleanup shows.
> ---
>   net/net.c        | 33 +++++++++++++++++++++++++++------
>   net/vhost-vdpa.c |  8 --------
>   2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/net/net.c b/net/net.c
> index 9f96509b2a..d097ac3e74 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -428,7 +428,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) {
> @@ -438,16 +444,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;
>       }
>
> @@ -465,8 +468,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 */
> @@ -1686,6 +1693,9 @@ void net_cleanup(void)
>        * of the latest NET_CLIENT_DRIVER_NIC, and operate on *p as we walk
>        * the list.
>        *
> +     * However, the NIC may have peers that trust to be clean beyond this
> +     * point.  For example, if they have been removed with device_del.
> +     *
>        * The 'nc' variable isn't part of the list traversal; it's purely
>        * for convenience as too much '(*p)->' has a tendency to make the
>        * readers' eyes bleed.
> @@ -1693,6 +1703,17 @@ void net_cleanup(void)
>       while (*p) {
>           nc = *p;
>           if (nc->info->type == NET_CLIENT_DRIVER_NIC) {
> +            NICState *nic = qemu_get_nic(nc);
> +
> +            if (nic->peer_deleted) {
> +                int queues = MAX(nic->conf->peers.queues, 1);
> +
> +                for (int i = 0; i < queues; i++) {
> +                    NetClientState *nc = qemu_get_subqueue(nic, i);
> +                    qemu_cleanup_net_client(nc->peer, false);
> +                }
> +            }
> +
>               /* Skip NET_CLIENT_DRIVER_NIC entries */
>               p = &QTAILQ_NEXT(nc, next);
>           } else {
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 46b02c50be..e1a1c91e48 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -224,14 +224,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.46.0
>

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

[Jason Wang]

On Wed, Sep 18, 2024 at 11:57 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 9/12/2024 9:54 AM, Eugenio Pérez 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 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 and to
> > net_cleanup.
> >
> > Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> > Reported-by: Lei Yang <leiyang@redhat.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Reviewed-by: Si-Wei Liu <si-wei.liu@oracle.com>

Queued.

Thanks

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

[Jason Wang]

On Fri, Sep 13, 2024 at 12:54 AM 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 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 and to
> net_cleanup.
>
> Fixes: a0d7215e33 ("vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present")
> Reported-by: Lei Yang <leiyang@redhat.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> v2: Remove NIC peer also at net_cleanup. vhost-user trust all the
> backends are clean before qemu removes char devices.
>
> This is not a requisite introduced by this commit as
> system/runstate.c:qemu_cleanup shows.
> ---

Unfortunately, this fails the build:

https://gitlab.com/jasowang/qemu/-/jobs/8138832559

Thanks

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

[Michael Tokarev]

12.09.2024 19:54, Eugenio Pérez 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.
> 
> v2:
> Remove NIC peer also at net_cleanup. vhost-user trust all the
> backends are clean before qemu removes char devices.
> 
> This is not a requisite introduced by this commit as
> system/runstate.c:qemu_cleanup shows.
> 
> Eugenio Pérez (2):
>    net: parameterize the removing client from nc list
>    net: move backend cleanup to NIC cleanup
> 
>   net/net.c        | 44 ++++++++++++++++++++++++++++++++++----------
>   net/vhost-vdpa.c |  8 --------
>   2 files changed, 34 insertions(+), 18 deletions(-)
Hi!

It looks like this series has been forgotten.  Is it still needed?

In order for it to build, a single line in patch 2 needs to be
changed (in net_cleanup()), from:

+                for (int i = 0; i < queues; i++) {
+                    NetClientState *nc = qemu_get_subqueue(nic, i);
+                    qemu_cleanup_net_client(nc->peer, false);
+                }

to

+                for (int i = 0; i < queues; i++) {
+                    nc = qemu_get_subqueue(nic, i);
+                    qemu_cleanup_net_client(nc->peer, false);
+                }

so there's no variable shadowing anymore.

Should this series be resent (a v3), or can this be fixed at apply time?

Thanks,

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

[Eugenio Perez Martin]

On Fri, Dec 20, 2024 at 10:02 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>
> 12.09.2024 19:54, Eugenio Pérez 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.
> >
> > v2:
> > Remove NIC peer also at net_cleanup. vhost-user trust all the
> > backends are clean before qemu removes char devices.
> >
> > This is not a requisite introduced by this commit as
> > system/runstate.c:qemu_cleanup shows.
> >
> > Eugenio Pérez (2):
> >    net: parameterize the removing client from nc list
> >    net: move backend cleanup to NIC cleanup
> >
> >   net/net.c        | 44 ++++++++++++++++++++++++++++++++++----------
> >   net/vhost-vdpa.c |  8 --------
> >   2 files changed, 34 insertions(+), 18 deletions(-)
> Hi!
>
> It looks like this series has been forgotten.  Is it still needed?
>
> In order for it to build, a single line in patch 2 needs to be
> changed (in net_cleanup()), from:
>
> +                for (int i = 0; i < queues; i++) {
> +                    NetClientState *nc = qemu_get_subqueue(nic, i);
> +                    qemu_cleanup_net_client(nc->peer, false);
> +                }
>
> to
>
> +                for (int i = 0; i < queues; i++) {
> +                    nc = qemu_get_subqueue(nic, i);
> +                    qemu_cleanup_net_client(nc->peer, false);
> +                }
>
> so there's no variable shadowing anymore.
>
> Should this series be resent (a v3), or can this be fixed at apply time?
>

I guess it is better to resend a v3, so we don't have a compiler
warning (or error). Jonah, maybe do you want to move this one forward?

Thanks!

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

[Jonah Palmer]

On 12/23/24 11:33 AM, Eugenio Perez Martin wrote:
> On Fri, Dec 20, 2024 at 10:02 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
>>
>> 12.09.2024 19:54, Eugenio Pérez 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.
>>>
>>> v2:
>>> Remove NIC peer also at net_cleanup. vhost-user trust all the
>>> backends are clean before qemu removes char devices.
>>>
>>> This is not a requisite introduced by this commit as
>>> system/runstate.c:qemu_cleanup shows.
>>>
>>> Eugenio Pérez (2):
>>>     net: parameterize the removing client from nc list
>>>     net: move backend cleanup to NIC cleanup
>>>
>>>    net/net.c        | 44 ++++++++++++++++++++++++++++++++++----------
>>>    net/vhost-vdpa.c |  8 --------
>>>    2 files changed, 34 insertions(+), 18 deletions(-)
>> Hi!
>>
>> It looks like this series has been forgotten.  Is it still needed?
>>
>> In order for it to build, a single line in patch 2 needs to be
>> changed (in net_cleanup()), from:
>>
>> +                for (int i = 0; i < queues; i++) {
>> +                    NetClientState *nc = qemu_get_subqueue(nic, i);
>> +                    qemu_cleanup_net_client(nc->peer, false);
>> +                }
>>
>> to
>>
>> +                for (int i = 0; i < queues; i++) {
>> +                    nc = qemu_get_subqueue(nic, i);
>> +                    qemu_cleanup_net_client(nc->peer, false);
>> +                }
>>
>> so there's no variable shadowing anymore.
>>
>> Should this series be resent (a v3), or can this be fixed at apply time?
>>
> 
> I guess it is better to resend a v3, so we don't have a compiler
> warning (or error). Jonah, maybe do you want to move this one forward?
> 
> Thanks!
> 
Just sent out the v3 with the quick fix.

Ugh, and I just realized I forgot to CC Michael Tokarev in the v3 
series. My apologies Michael!

Jonah