1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true on EAGAIN/EINTR

Shameer Kolothum posted 1 patch 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260623075101.17770-1-skolothumtho@nvidia.com
Maintainers: Eric Auger <eric.auger@redhat.com>, Peter Maydell <peter.maydell@linaro.org>
There is a newer version of this series
hw/arm/smmuv3-accel.c   | 6 ++++--
hw/arm/tegra241-cmdqv.c | 4 +++-
2 files changed, 7 insertions(+), 3 deletions(-)
[PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true on EAGAIN/EINTR
Posted by Shameer Kolothum 1 month ago 
smmuv3_accel_event_read_validate() returns true for EAGAIN/EINTR, but
no data has been read into the buffer. Callers treat true as success and
proceed to use the uninitialized buffer. Return false instead and guard
warn_report_err_once() against NULL Error in both callers.

Resolves: Coverity CID 1660057
Fixes: d4aea0f75b ("hw/arm/smmuv3-accel: Introduce common helper for veventq read")
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 hw/arm/smmuv3-accel.c   | 6 ++++--
 hw/arm/tegra241-cmdqv.c | 4 +++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
index 80900c2521..6d402d2ba6 100644
--- a/hw/arm/smmuv3-accel.c
+++ b/hw/arm/smmuv3-accel.c
@@ -451,7 +451,7 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
     bytes = read(veventq->veventq_fd, buf, size);
     if (bytes <= 0) {
         if (errno == EAGAIN || errno == EINTR) {
-            return true;
+            return false;
         }
         error_setg(errp, "vEVENTQ(type %u id %u): read failed (%m)", type, id);
         return false;
@@ -491,7 +491,9 @@ static void smmuv3_accel_event_read(void *opaque)
     if (!smmuv3_accel_event_read_validate(veventq,
                                           IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
                                           sizeof(buf), &local_err)) {
-        warn_report_err_once(local_err);
+        if (local_err) {
+            warn_report_err_once(local_err);
+        }
         return;
     }
     smmuv3_propagate_event(s, (Evt *)&buf.vevent);
diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
index 29c488e0e4..bb0b417e90 100644
--- a/hw/arm/tegra241-cmdqv.c
+++ b/hw/arm/tegra241-cmdqv.c
@@ -845,7 +845,9 @@ static void tegra241_cmdqv_event_read(void *opaque)
     if (!smmuv3_accel_event_read_validate(veventq,
                                           IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
                                           &buf, sizeof(buf), &local_err)) {
-        warn_report_err_once(local_err);
+        if (local_err) {
+            warn_report_err_once(local_err);
+        }
         return;
     }
 
-- 
2.43.0
Re: [PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true on EAGAIN/EINTR
Posted by Eric Auger 1 month ago 
Hi Shameer,

On 6/23/26 9:51 AM, Shameer Kolothum wrote:
> smmuv3_accel_event_read_validate() returns true for EAGAIN/EINTR, but
> no data has been read into the buffer. Callers treat true as success and
> proceed to use the uninitialized buffer. Return false instead and guard
> warn_report_err_once() against NULL Error in both callers.
>
> Resolves: Coverity CID 1660057
> Fixes: d4aea0f75b ("hw/arm/smmuv3-accel: Introduce common helper for veventq read")
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>  hw/arm/smmuv3-accel.c   | 6 ++++--
>  hw/arm/tegra241-cmdqv.c | 4 +++-
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> index 80900c2521..6d402d2ba6 100644
> --- a/hw/arm/smmuv3-accel.c
> +++ b/hw/arm/smmuv3-accel.c
> @@ -451,7 +451,7 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
>      bytes = read(veventq->veventq_fd, buf, size);
>      if (bytes <= 0) {
>          if (errno == EAGAIN || errno == EINTR) {
> -            return true;
> +            return false;
>          }
>          error_setg(errp, "vEVENTQ(type %u id %u): read failed (%m)", type, id);
>          return false;
> @@ -491,7 +491,9 @@ static void smmuv3_accel_event_read(void *opaque)
>      if (!smmuv3_accel_event_read_validate(veventq,
>                                            IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
>                                            sizeof(buf), &local_err)) {
> -        warn_report_err_once(local_err);
> +        if (local_err) {
> +            warn_report_err_once(local_err);
> +        }
>          return;
>      }
>      smmuv3_propagate_event(s, (Evt *)&buf.vevent);
> diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> index 29c488e0e4..bb0b417e90 100644
> --- a/hw/arm/tegra241-cmdqv.c
> +++ b/hw/arm/tegra241-cmdqv.c
> @@ -845,7 +845,9 @@ static void tegra241_cmdqv_event_read(void *opaque)
>      if (!smmuv3_accel_event_read_validate(veventq,
>                                            IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
>                                            &buf, sizeof(buf), &local_err)) {
> -        warn_report_err_once(local_err);
> +        if (local_err) {
This is not aligned with the general policy. If a function taking an
error handle fails (and returns false), it should set errp.
Don't you have a way to initialize buf in smmuv3_accel_event_read() and
only call smmuv3_propagate_event() if the hdr of vevnt is checked valid?

Thanks

Eric
> +            warn_report_err_once(local_err);
> +        }
>          return;
>      }
>
Re: [PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true on EAGAIN/EINTR
Posted by Peter Maydell 1 month ago 
On Tue, 23 Jun 2026 at 09:32, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Shameer,
>
> On 6/23/26 9:51 AM, Shameer Kolothum wrote:
> > smmuv3_accel_event_read_validate() returns true for EAGAIN/EINTR, but
> > no data has been read into the buffer. Callers treat true as success and
> > proceed to use the uninitialized buffer. Return false instead and guard
> > warn_report_err_once() against NULL Error in both callers.
> >
> > Resolves: Coverity CID 1660057
> > Fixes: d4aea0f75b ("hw/arm/smmuv3-accel: Introduce common helper for veventq read")
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> > ---
> >  hw/arm/smmuv3-accel.c   | 6 ++++--
> >  hw/arm/tegra241-cmdqv.c | 4 +++-
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
> > index 80900c2521..6d402d2ba6 100644
> > --- a/hw/arm/smmuv3-accel.c
> > +++ b/hw/arm/smmuv3-accel.c
> > @@ -451,7 +451,7 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
> >      bytes = read(veventq->veventq_fd, buf, size);
> >      if (bytes <= 0) {
> >          if (errno == EAGAIN || errno == EINTR) {
> > -            return true;
> > +            return false;
> >          }
> >          error_setg(errp, "vEVENTQ(type %u id %u): read failed (%m)", type, id);
> >          return false;
> > @@ -491,7 +491,9 @@ static void smmuv3_accel_event_read(void *opaque)
> >      if (!smmuv3_accel_event_read_validate(veventq,
> >                                            IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
> >                                            sizeof(buf), &local_err)) {
> > -        warn_report_err_once(local_err);
> > +        if (local_err) {
> > +            warn_report_err_once(local_err);
> > +        }
> >          return;
> >      }
> >      smmuv3_propagate_event(s, (Evt *)&buf.vevent);
> > diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
> > index 29c488e0e4..bb0b417e90 100644
> > --- a/hw/arm/tegra241-cmdqv.c
> > +++ b/hw/arm/tegra241-cmdqv.c
> > @@ -845,7 +845,9 @@ static void tegra241_cmdqv_event_read(void *opaque)
> >      if (!smmuv3_accel_event_read_validate(veventq,
> >                                            IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
> >                                            &buf, sizeof(buf), &local_err)) {
> > -        warn_report_err_once(local_err);
> > +        if (local_err) {
> This is not aligned with the general policy. If a function taking an
> error handle fails (and returns false), it should set errp.
> Don't you have a way to initialize buf in smmuv3_accel_event_read() and
> only call smmuv3_propagate_event() if the hdr of vevnt is checked valid?

I think fundamentally the function has three return states:
 - success
 - failure
 - try again later

Probably the simplest thing is to not use a 'bool' for the return
type but instead something that has 3 values. Then you can set errp
on the "failure" case, and the caller can tell which of the three
cases it is dealing with.

-- PMM
RE: [PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true on EAGAIN/EINTR
Posted by Shameer Kolothum Thodi 1 month ago 


> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: 23 June 2026 09:47
> To: eric.auger@redhat.com
> Cc: Shameer Kolothum Thodi <skolothumtho@nvidia.com>; qemu-
> arm@nongnu.org; qemu-devel@nongnu.org; Nicolin Chen
> <nicolinc@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>; Jiandi An <jan@nvidia.com>
> Subject: Re: [PATCH] hw/arm/smmuv3-accel: Fix veventq read returning true
> on EAGAIN/EINTR
> 
> On Tue, 23 Jun 2026 at 09:32, Eric Auger <eric.auger@redhat.com> wrote:
)
> > >      if (!smmuv3_accel_event_read_validate(veventq,
> > >                                            IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
> > >                                            &buf, sizeof(buf), &local_err)) {
> > > -        warn_report_err_once(local_err);
> > > +        if (local_err) {
> > This is not aligned with the general policy. If a function taking an
> > error handle fails (and returns false), it should set errp.
> > Don't you have a way to initialize buf in smmuv3_accel_event_read()
> > and only call smmuv3_propagate_event() if the hdr of vevnt is checked
> valid?
> 
> I think fundamentally the function has three return states:
>  - success
>  - failure
>  - try again later
> 
> Probably the simplest thing is to not use a 'bool' for the return type but
> instead something that has 3 values. Then you can set errp on the "failure"
> case, and the caller can tell which of the three cases it is dealing with.

Ok. How about this one below. I used int and documented ret types.
Could switch to enum if that’s preferred. Please let me know.

Thanks,
Shameer

diff --git a/hw/arm/smmuv3-accel.h b/hw/arm/smmuv3-accel.h
index dd755c394d..ea11d513cc 100644
--- a/hw/arm/smmuv3-accel.h
+++ b/hw/arm/smmuv3-accel.h
@@ -97,8 +97,8 @@ bool smmuv3_accel_issue_inv_cmd(SMMUv3State *s, void *cmd, SMMUDevice *sdev,
                                 Error **errp);
 void smmuv3_accel_idr_override(SMMUv3State *s);
 bool smmuv3_accel_alloc_veventq(SMMUv3State *s, Error **errp);
-bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
-                                      void *buf, size_t size, Error **errp);
+int smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
+                                     void *buf, size_t size, Error **errp);
 void smmuv3_accel_reset(SMMUv3State *s);
 SMMUv3AccelCmdqvType smmuv3_accel_cmdqv_type(Object *obj);

diff --git a/hw/arm/smmuv3-accel-stubs.c b/hw/arm/smmuv3-accel-stubs.c
index 147ae06163..b8dd7e7b89 100644
--- a/hw/arm/smmuv3-accel-stubs.c
+++ b/hw/arm/smmuv3-accel-stubs.c
@@ -47,10 +47,10 @@ bool smmuv3_accel_alloc_veventq(SMMUv3State *s, Error **errp)
     return true;
 }

-bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
-                                      void *buf, size_t size, Error **errp)
+int smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
+                                     void *buf, size_t size, Error **errp)
 {
-    return true;
+    return 0;
 }


diff --git a/hw/arm/smmuv3-accel.c b/hw/arm/smmuv3-accel.c
index 80900c2521..6df19f0e0e 100644
--- a/hw/arm/smmuv3-accel.c
+++ b/hw/arm/smmuv3-accel.c
@@ -440,8 +440,13 @@ bool smmuv3_accel_issue_inv_cmd(SMMUv3State *bs, void *cmd, SMMUDevice *sdev,
                    sizeof(Cmd), &entry_num, cmd, errp);
 }

-bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
-                                      void *buf, size_t size, Error **errp)
+/*
+ * Returns  0 on success (buf is populated and valid).
+ * Returns  1 if the read should be retried (EAGAIN/EINTR).
+ * Returns -1 on error with @errp set.
+ */
+int smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
+                                     void *buf, size_t size, Error **errp)
 {
     uint32_t last_seq = veventq->last_event_seq;
     uint32_t id = veventq->veventq_id;
@@ -451,22 +456,22 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
     bytes = read(veventq->veventq_fd, buf, size);
     if (bytes <= 0) {
         if (errno == EAGAIN || errno == EINTR) {
-            return true;
+            return 1;
         }
         error_setg(errp, "vEVENTQ(type %u id %u): read failed (%m)", type, id);
-        return false;
+        return -1;
     }
     hdr = (struct iommufd_vevent_header *)buf;
     if (bytes == sizeof(*hdr) &&
         (hdr->flags & IOMMU_VEVENTQ_FLAG_LOST_EVENTS)) {
         error_setg(errp, "vEVENTQ(type %u id %u): overflowed", type, id);
         veventq->event_start = false;
-        return false;
+        return -1;
     }
     if (bytes < size) {
         error_setg(errp, "vEVENTQ(type %u id %u): short read(%zd/%zd bytes)",
                           type, id, bytes, size);
-        return false;
+        return -1;
     }
     /* Check sequence in hdr for lost events if any */
     if (veventq->event_start && (hdr->sequence - last_seq != 1)) {
@@ -475,7 +480,7 @@ bool smmuv3_accel_event_read_validate(IOMMUFDVeventq *veventq, uint32_t type,
     }
     veventq->last_event_seq = hdr->sequence;
     veventq->event_start = true;
-    return true;
+    return 0;
 }

 static void smmuv3_accel_event_read(void *opaque)
@@ -487,13 +492,18 @@ static void smmuv3_accel_event_read(void *opaque)
         struct iommu_vevent_arm_smmuv3 vevent;
     } buf;
     Error *local_err = NULL;
+    int ret;

-    if (!smmuv3_accel_event_read_validate(veventq,
-                                          IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
-                                          sizeof(buf), &local_err)) {
+    ret = smmuv3_accel_event_read_validate(veventq,
+                                           IOMMU_VEVENTQ_TYPE_ARM_SMMUV3, &buf,
+                                           sizeof(buf), &local_err);
+    if (ret < 0) {
         warn_report_err_once(local_err);
         return;
     }
+    if (ret > 0) {
+        return; /* EAGAIN/EINTR */
+    }
     smmuv3_propagate_event(s, (Evt *)&buf.vevent);
 }

diff --git a/hw/arm/tegra241-cmdqv.c b/hw/arm/tegra241-cmdqv.c
index 29c488e0e4..7223aa9d1d 100644
--- a/hw/arm/tegra241-cmdqv.c
+++ b/hw/arm/tegra241-cmdqv.c
@@ -841,13 +841,18 @@ static void tegra241_cmdqv_event_read(void *opaque)
         struct iommu_vevent_tegra241_cmdqv vevent;
     } buf;
     Error *local_err = NULL;
+    int ret;

-    if (!smmuv3_accel_event_read_validate(veventq,
-                                          IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
-                                          &buf, sizeof(buf), &local_err)) {
+    ret = smmuv3_accel_event_read_validate(veventq,
+                                           IOMMU_VEVENTQ_TYPE_TEGRA241_CMDQV,
+                                           &buf, sizeof(buf), &local_err);
+    if (ret < 0) {
         warn_report_err_once(local_err);
         return;
     }
+    if (ret > 0) {
+        return; /* EAGAIN/EINTR */
+    }

     if (buf.vevent.lvcmdq_err_map[0] || buf.vevent.lvcmdq_err_map[1]) {
         cmdqv->vintf_cmdq_err_map[0] =
--

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.