[PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Cindy Lu]

In function kvm_virtio_pci_vector_use_one(), in the undo label,
the function will get the vector incorrectly while using
VIRTIO_CONFIG_IRQ_IDX
To fix this, we remove this label and simplify the failure process

Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
Cc: qemu-stable@nongnu.org
Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 hw/virtio/virtio-pci.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b138fa127a..565bdb0897 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
     }
     ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
     if (ret < 0) {
-        goto undo;
+        return ret;
     }
     /*
      * If guest supports masking, set up irqfd now.
@@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
         ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
         if (ret < 0) {
             kvm_virtio_pci_vq_vector_release(proxy, vector);
-            goto undo;
+            kvm_virtio_pci_irqfd_release(proxy, n, vector);
+            return ret;
         }
     }
 
     return 0;
-undo:
-
-    vector = virtio_queue_vector(vdev, queue_no);
-    if (vector >= msix_nr_vectors_allocated(dev)) {
-        return ret;
-    }
-    if (vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
-        ret = virtio_pci_get_notifier(proxy, queue_no, &n, &vector);
-        if (ret < 0) {
-            return ret;
-        }
-        kvm_virtio_pci_irqfd_release(proxy, n, vector);
-    }
-    return ret;
 }
 static int kvm_virtio_pci_vector_vq_use(VirtIOPCIProxy *proxy, int nvqs)
 {
-- 
2.43.0

Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Peter Maydell]

On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote:
>
> In function kvm_virtio_pci_vector_use_one(), in the undo label,
> the function will get the vector incorrectly while using
> VIRTIO_CONFIG_IRQ_IDX
> To fix this, we remove this label and simplify the failure process
>
> Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  hw/virtio/virtio-pci.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index b138fa127a..565bdb0897 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
>      }
>      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
>      if (ret < 0) {
> -        goto undo;
> +        return ret;
>      }
>      /*
>       * If guest supports masking, set up irqfd now.
> @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
>          ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
>          if (ret < 0) {
>              kvm_virtio_pci_vq_vector_release(proxy, vector);
> -            goto undo;
> +            kvm_virtio_pci_irqfd_release(proxy, n, vector);

Are you sure this is right? The kvm_virtio_pci_irqfd_use()
just failed, so why do we need to call
kvm_virtio_pci_irqfd_release() ?

thanks
-- PMM

Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Cindy Lu]

On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote:
> >
> > In function kvm_virtio_pci_vector_use_one(), in the undo label,
> > the function will get the vector incorrectly while using
> > VIRTIO_CONFIG_IRQ_IDX
> > To fix this, we remove this label and simplify the failure process
> >
> > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  hw/virtio/virtio-pci.c | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index b138fa127a..565bdb0897 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> >      }
> >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> >      if (ret < 0) {
> > -        goto undo;
> > +        return ret;
> >      }
> >      /*
> >       * If guest supports masking, set up irqfd now.
> > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> >          ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> >          if (ret < 0) {
> >              kvm_virtio_pci_vq_vector_release(proxy, vector);
> > -            goto undo;
> > +            kvm_virtio_pci_irqfd_release(proxy, n, vector);
>
> Are you sure this is right? The kvm_virtio_pci_irqfd_use()
> just failed, so why do we need to call
> kvm_virtio_pci_irqfd_release() ?
>
> thanks
> -- PMM
>
This version should be correct.  when kvm_virtio_pci_irqfd_use() fail
we need to call kvm_virtio_pci_vq_vector_release() and
kvm_virtio_pci_irqfd_release()
but for kvm_virtio_pci_vq_vector_use fail we can simple return,

in old version there is a error in failure process.
while the kvm_virtio_pci_vq_vector_use fail it  call the
kvm_virtio_pci_irqfd_release,but at this time this is irqfd
is not using now

I have do the qtest and sanity test for this patch
Thanks
cindy

Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Peter Maydell]

On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote:
>
> On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > In function kvm_virtio_pci_vector_use_one(), in the undo label,
> > > the function will get the vector incorrectly while using
> > > VIRTIO_CONFIG_IRQ_IDX
> > > To fix this, we remove this label and simplify the failure process
> > >
> > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> > > Cc: qemu-stable@nongnu.org
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  hw/virtio/virtio-pci.c | 19 +++----------------
> > >  1 file changed, 3 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > index b138fa127a..565bdb0897 100644
> > > --- a/hw/virtio/virtio-pci.c
> > > +++ b/hw/virtio/virtio-pci.c
> > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > >      }
> > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > >      if (ret < 0) {
> > > -        goto undo;
> > > +        return ret;
> > >      }
> > >      /*
> > >       * If guest supports masking, set up irqfd now.
> > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > >          ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> > >          if (ret < 0) {
> > >              kvm_virtio_pci_vq_vector_release(proxy, vector);
> > > -            goto undo;
> > > +            kvm_virtio_pci_irqfd_release(proxy, n, vector);
> >
> > Are you sure this is right? The kvm_virtio_pci_irqfd_use()
> > just failed, so why do we need to call
> > kvm_virtio_pci_irqfd_release() ?

> This version should be correct.  when kvm_virtio_pci_irqfd_use() fail
> we need to call kvm_virtio_pci_vq_vector_release() and
> kvm_virtio_pci_irqfd_release()
> but for kvm_virtio_pci_vq_vector_use fail we can simple return,

But *why* do we need to call kvm_virtio_pci_irqfd_release()?

In most API designs, this kind of pairing of "get/use/allocate
something" and "free/release something" function only
requires you to do the "release" if the "get" succeeded,
because if the "get" fails it's supposed to fail in way that
means "I didn't do anything". Is this API not following that
standard pattern ?

> in old version there is a error in failure process.
> while the kvm_virtio_pci_vq_vector_use fail it  call the
> kvm_virtio_pci_irqfd_release,but at this time this is irqfd
> is not using now

thanks
-- PMM

Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Michael S. Tsirkin]

On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote:
> On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote:
> >
> > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote:
> > > >
> > > > In function kvm_virtio_pci_vector_use_one(), in the undo label,
> > > > the function will get the vector incorrectly while using
> > > > VIRTIO_CONFIG_IRQ_IDX
> > > > To fix this, we remove this label and simplify the failure process

And then what happens?  It's unclear whether it's a real or
theoretical issue.

> > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> > > > Cc: qemu-stable@nongnu.org
> > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio-pci.c | 19 +++----------------
> > > >  1 file changed, 3 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > index b138fa127a..565bdb0897 100644
> > > > --- a/hw/virtio/virtio-pci.c
> > > > +++ b/hw/virtio/virtio-pci.c
> > > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > >      }
> > > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > > >      if (ret < 0) {
> > > > -        goto undo;
> > > > +        return ret;
> > > >      }
> > > >      /*
> > > >       * If guest supports masking, set up irqfd now.
> > > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > >          ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> > > >          if (ret < 0) {
> > > >              kvm_virtio_pci_vq_vector_release(proxy, vector);
> > > > -            goto undo;
> > > > +            kvm_virtio_pci_irqfd_release(proxy, n, vector);
> > >
> > > Are you sure this is right? The kvm_virtio_pci_irqfd_use()
> > > just failed, so why do we need to call
> > > kvm_virtio_pci_irqfd_release() ?
> 
> > This version should be correct.  when kvm_virtio_pci_irqfd_use() fail
> > we need to call kvm_virtio_pci_vq_vector_release() and
> > kvm_virtio_pci_irqfd_release()
> > but for kvm_virtio_pci_vq_vector_use fail we can simple return,
> 
> But *why* do we need to call kvm_virtio_pci_irqfd_release()?
> 
> In most API designs, this kind of pairing of "get/use/allocate
> something" and "free/release something" function only
> requires you to do the "release" if the "get" succeeded,
> because if the "get" fails it's supposed to fail in way that
> means "I didn't do anything". Is this API not following that
> standard pattern ?


I am just as puzzled.

> > in old version there is a error in failure process.
> > while the kvm_virtio_pci_vq_vector_use fail it  call the
> > kvm_virtio_pci_irqfd_release,but at this time this is irqfd
> > is not using now
> 
> thanks
> -- PMM

Re: [PATCH] virtio-pci: Fix the failure process in kvm_virtio_pci_vector_use_one()

[Cindy Lu]

On Wed, Apr 17, 2024 at 2:38 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 16, 2024 at 02:14:57PM +0100, Peter Maydell wrote:
> > On Tue, 16 Apr 2024 at 13:41, Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > On Tue, Apr 16, 2024 at 8:30 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > > >
> > > > On Tue, 16 Apr 2024 at 13:29, Cindy Lu <lulu@redhat.com> wrote:
> > > > >
> > > > > In function kvm_virtio_pci_vector_use_one(), in the undo label,
> > > > > the function will get the vector incorrectly while using
> > > > > VIRTIO_CONFIG_IRQ_IDX
> > > > > To fix this, we remove this label and simplify the failure process
>
> And then what happens?  It's unclear whether it's a real or
> theoretical issue.
>
> > > > > Fixes: f9a09ca3ea ("vhost: add support for configure interrupt")
> > > > > Cc: qemu-stable@nongnu.org
> > > > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > > > ---
> > > > >  hw/virtio/virtio-pci.c | 19 +++----------------
> > > > >  1 file changed, 3 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > > > index b138fa127a..565bdb0897 100644
> > > > > --- a/hw/virtio/virtio-pci.c
> > > > > +++ b/hw/virtio/virtio-pci.c
> > > > > @@ -892,7 +892,7 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > > >      }
> > > > >      ret = kvm_virtio_pci_vq_vector_use(proxy, vector);
> > > > >      if (ret < 0) {
> > > > > -        goto undo;
> > > > > +        return ret;
> > > > >      }
> > > > >      /*
> > > > >       * If guest supports masking, set up irqfd now.
> > > > > @@ -902,25 +902,12 @@ static int kvm_virtio_pci_vector_use_one(VirtIOPCIProxy *proxy, int queue_no)
> > > > >          ret = kvm_virtio_pci_irqfd_use(proxy, n, vector);
> > > > >          if (ret < 0) {
> > > > >              kvm_virtio_pci_vq_vector_release(proxy, vector);
> > > > > -            goto undo;
> > > > > +            kvm_virtio_pci_irqfd_release(proxy, n, vector);
> > > >
> > > > Are you sure this is right? The kvm_virtio_pci_irqfd_use()
> > > > just failed, so why do we need to call
> > > > kvm_virtio_pci_irqfd_release() ?
> >
> > > This version should be correct.  when kvm_virtio_pci_irqfd_use() fail
> > > we need to call kvm_virtio_pci_vq_vector_release() and
> > > kvm_virtio_pci_irqfd_release()
> > > but for kvm_virtio_pci_vq_vector_use fail we can simple return,
> >
> > But *why* do we need to call kvm_virtio_pci_irqfd_release()?
> >
> > In most API designs, this kind of pairing of "get/use/allocate
> > something" and "free/release something" function only
> > requires you to do the "release" if the "get" succeeded,
> > because if the "get" fails it's supposed to fail in way that
> > means "I didn't do anything". Is this API not following that
> > standard pattern ?
>
>
> I am just as puzzled.
>
got it. I made a mistake here, I will send the new version
Thanks
cindy
> > > in old version there is a error in failure process.
> > > while the kvm_virtio_pci_vq_vector_use fail it  call the
> > > kvm_virtio_pci_irqfd_release,but at this time this is irqfd
> > > is not using now
> >
> > thanks
> > -- PMM
>