hw/usb/hcd-ohci: Fix memory leak in ohci_service_iso_td()

[PATCH] hw/usb/hcd-ohci: Fix memory leak in ohci_service_iso_td()

Posted by munkhuu0825@gmail.com 3 weeks, 4 days ago

From: heropd <munkhuu0825@gmail.com>

Fix a memory leak in ohci_service_iso_td() by calling
usb_packet_cleanup() before freeing USBPacket.

Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>
---
 hw/usb/hcd-ohci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 6ed8046fc2..43c49a42ec 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -748,6 +748,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
     usb_handle_packet(dev, pkt);
     if (pkt->status == USB_RET_ASYNC) {
         usb_device_flush_ep_queue(dev, ep);
+        usb_packet_cleanup(pkt);
         g_free(pkt);
         return 1;
     }
@@ -756,6 +757,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
     } else {
         ret = pkt->status;
     }
+    usb_packet_cleanup(pkt);
     g_free(pkt);
 
     trace_usb_ohci_iso_td_so(start_offset, end_offset, start_addr, end_addr,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] hw/usb/hcd-ohci: Fix memory leak in ohci_service_iso_td()

Posted by Peter Maydell 3 weeks, 4 days ago

On Tue, 5 May 2026 at 12:01, <munkhuu0825@gmail.com> wrote:
>
> From: heropd <munkhuu0825@gmail.com>
>
> Fix a memory leak in ohci_service_iso_td() by calling
> usb_packet_cleanup() before freeing USBPacket.
>
> Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3463


> ---
>  hw/usb/hcd-ohci.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 6ed8046fc2..43c49a42ec 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -748,6 +748,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>      usb_handle_packet(dev, pkt);
>      if (pkt->status == USB_RET_ASYNC) {
>          usb_device_flush_ep_queue(dev, ep);
> +        usb_packet_cleanup(pkt);
>          g_free(pkt);
>          return 1;
>      }

I don't really understand the USB subsystem, but something seems
odd here. The case when usb_handle_packet() can set the
p->status to USB_RET_ASYNC is when it has added it to a queue,
either directly ("QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);")
or by calling usb_queue_one(). The usb_device_flush_ep_queue()
does not (as far as I can tell) remove the queued packet. So it
seems wrong to me that this code path frees the packet, and
also if we call usb_packet_cleanup() it will do
    assert(!usb_packet_is_inflight(p));
which I think will fire.

It's possible that we can't ever actually get to this code --
the usb_handle_packet() has a claim "hcd drivers cannot handle
async for isoc" and some assertions about that, which suggests
maybe that the intent is that we won't ever return USB_RET_ASYNC
for an iso TD. But if so, we should be asserting that.

> @@ -756,6 +757,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>      } else {
>          ret = pkt->status;
>      }
> +    usb_packet_cleanup(pkt);
>      g_free(pkt);

This one's probably OK -- is it the one that fixes the repro
case in the bug report?

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-ohci: Fix memory leak in ohci_service_iso_td()

Posted by Мөнхбаатар Энхбаатар 3 weeks, 3 days ago


> On 5 May 2026, at 21:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Tue, 5 May 2026 at 12:01, <munkhuu0825@gmail.com <mailto:munkhuu0825@gmail.com>> wrote:
>> 
>> From: heropd <munkhuu0825@gmail.com <mailto:munkhuu0825@gmail.com>>
>> 
>> Fix a memory leak in ohci_service_iso_td() by calling
>> usb_packet_cleanup() before freeing USBPacket.
>> 
>> Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com <mailto:munkhuu0825@gmail.com>>
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3463
> 
> 
>> ---
>> hw/usb/hcd-ohci.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
>> index 6ed8046fc2..43c49a42ec 100644
>> --- a/hw/usb/hcd-ohci.c
>> +++ b/hw/usb/hcd-ohci.c
>> @@ -748,6 +748,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>>     usb_handle_packet(dev, pkt);
>>     if (pkt->status == USB_RET_ASYNC) {
>>         usb_device_flush_ep_queue(dev, ep);
>> +        usb_packet_cleanup(pkt);
>>         g_free(pkt);
>>         return 1;
>>     }
> 
> I don't really understand the USB subsystem, but something seems
> odd here. The case when usb_handle_packet() can set the
> p->status to USB_RET_ASYNC is when it has added it to a queue,
> either directly ("QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);")
> or by calling usb_queue_one(). The usb_device_flush_ep_queue()
> does not (as far as I can tell) remove the queued packet. So it
> seems wrong to me that this code path frees the packet, and
> also if we call usb_packet_cleanup() it will do
>    assert(!usb_packet_is_inflight(p));
> which I think will fire.
> 
> It's possible that we can't ever actually get to this code --
> the usb_handle_packet() has a claim "hcd drivers cannot handle
> async for isoc" and some assertions about that, which suggests
> maybe that the intent is that we won't ever return USB_RET_ASYNC
> for an iso TD. But if so, we should be asserting that.
> 

Thanks that makes sense,
I tested the USB_RET_ASYNC path locally and usb_packet_cleanup() does indeed hit the inflight
assertion there
OHCI ISO ASYNC branch reached
Assertion failed: (!usb_packet_is_inflight(p)), function usb_packet_cleanup, file core.c, line 657.

>> @@ -756,6 +757,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>>     } else {
>>         ret = pkt->status;
>>     }
>> +    usb_packet_cleanup(pkt);
>>     g_free(pkt);
> 
> This one's probably OK -- is it the one that fixes the repro
> case in the bug report?

Yes this one should fix repro bug in report

> 
> thanks
> -- PMM

For the async path, I can leave it unchanged for this fix, or add an assertion since ISO TDs are not expected to complete asynchronously.

Thanks,
Munkhbaatar

Re: [PATCH] hw/usb/hcd-ohci: Fix memory leak in ohci_service_iso_td()

Posted by Peter Maydell 3 weeks, 3 days ago

On Wed, 6 May 2026 at 05:13, Мөнхбаатар Энхбаатар <munkhuu0825@gmail.com> wrote:
>
>
>
> On 5 May 2026, at 21:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 5 May 2026 at 12:01, <munkhuu0825@gmail.com> wrote:
>
>
> From: heropd <munkhuu0825@gmail.com>
>
> Fix a memory leak in ohci_service_iso_td() by calling
> usb_packet_cleanup() before freeing USBPacket.
>
> Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>
>
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3463
>
>
> ---
> hw/usb/hcd-ohci.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 6ed8046fc2..43c49a42ec 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -748,6 +748,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>     usb_handle_packet(dev, pkt);
>     if (pkt->status == USB_RET_ASYNC) {
>         usb_device_flush_ep_queue(dev, ep);
> +        usb_packet_cleanup(pkt);
>         g_free(pkt);
>         return 1;
>     }
>
>
> I don't really understand the USB subsystem, but something seems
> odd here. The case when usb_handle_packet() can set the
> p->status to USB_RET_ASYNC is when it has added it to a queue,
> either directly ("QTAILQ_INSERT_TAIL(&p->ep->queue, p, queue);")
> or by calling usb_queue_one(). The usb_device_flush_ep_queue()
> does not (as far as I can tell) remove the queued packet. So it
> seems wrong to me that this code path frees the packet, and
> also if we call usb_packet_cleanup() it will do
>    assert(!usb_packet_is_inflight(p));
> which I think will fire.
>
> It's possible that we can't ever actually get to this code --
> the usb_handle_packet() has a claim "hcd drivers cannot handle
> async for isoc" and some assertions about that, which suggests
> maybe that the intent is that we won't ever return USB_RET_ASYNC
> for an iso TD. But if so, we should be asserting that.
>
>
> Thanks that makes sense,
> I tested the USB_RET_ASYNC path locally and usb_packet_cleanup() does indeed hit the inflight
> assertion there
> OHCI ISO ASYNC branch reached
> Assertion failed: (!usb_packet_is_inflight(p)), function usb_packet_cleanup, file core.c, line 657.
>
> @@ -756,6 +757,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>     } else {
>         ret = pkt->status;
>     }
> +    usb_packet_cleanup(pkt);
>     g_free(pkt);
>
>
> This one's probably OK -- is it the one that fixes the repro
> case in the bug report?
>
>
> Yes this one should fix repro bug in report

> For the async path, I can leave it unchanged for this fix, or add
> an assertion since ISO TDs are not expected to complete asynchronously.

I think we should do this as two patches:
(1) change the "if (pkt->status == USB_RET_ASYNC) { ... }"
    to something like:
   /* The USB core guarantees to never defer ISO TDs */
   assert(pkt->status != USB_RET_ASYNC);

(2) fix the leak with the usb_packet_cleanup() call that's
    currently the second hunk in this patch

That way if I'm wrong about the USB subsystem's guarantees
we'll be able to just revert patch 1 without undoing the
fix for the leak.

thanks
-- PMM

[PATCH v2 0/2] hw/usb/hcd-ohci: Fix ISO TD USBPacket handling

Posted by Munkhbaatar Enkhbaatar 3 weeks, 3 days ago

This series updates OHCI ISO TD USBPacket handling.

Changes since v1:
- Split the original patch into two patches.
- Replace the USB_RET_ASYNC handling with an assertion.
- Keep usb_packet_cleanup() only on the synchronous completion path.

Munkhbaatar Enkhbaatar (2):
  hw/usb/hcd-ohci: Assert isochronous TDs are never deferred
  hw/usb/hcd-ohci: Clean up USBPacket before freeing ISO TD packet

 hw/usb/hcd-ohci.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.50.1 (Apple Git-155)

[PATCH v2 1/2] hw/usb/hcd-ohci: Assert isochronous TDs are never deferred

Posted by Munkhbaatar Enkhbaatar 3 weeks, 3 days ago


Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>
---
 hw/usb/hcd-ohci.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 6ed8046fc2..8f4de0066e 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -746,11 +746,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
     usb_packet_setup(pkt, pid, ep, 0, addr, false, int_req);
     usb_packet_addbuf(pkt, buf, len);
     usb_handle_packet(dev, pkt);
-    if (pkt->status == USB_RET_ASYNC) {
-        usb_device_flush_ep_queue(dev, ep);
-        g_free(pkt);
-        return 1;
-    }
+
+    /* The USB core guarantees to never defer ISO TDs. */
+    assert(pkt->status != USB_RET_ASYNC);
+
     if (pkt->status == USB_RET_SUCCESS) {
         ret = pkt->actual_length;
     } else {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/2] hw/usb/hcd-ohci: Assert isochronous TDs are never deferred

Posted by Peter Maydell 3 weeks, 2 days ago

On Wed, 6 May 2026 at 16:23, Munkhbaatar Enkhbaatar
<munkhuu0825@gmail.com> wrote:
>
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>
> ---
>  hw/usb/hcd-ohci.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 6ed8046fc2..8f4de0066e 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -746,11 +746,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>      usb_packet_setup(pkt, pid, ep, 0, addr, false, int_req);
>      usb_packet_addbuf(pkt, buf, len);
>      usb_handle_packet(dev, pkt);
> -    if (pkt->status == USB_RET_ASYNC) {
> -        usb_device_flush_ep_queue(dev, ep);
> -        g_free(pkt);
> -        return 1;
> -    }
> +
> +    /* The USB core guarantees to never defer ISO TDs. */
> +    assert(pkt->status != USB_RET_ASYNC);

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v2 2/2] hw/usb/hcd-ohci: Clean up USBPacket before freeing ISO TD packet

Posted by Munkhbaatar Enkhbaatar 3 weeks, 3 days ago

ohci_service_iso_td() allocates a USBPacket and frees it after synchronous
completion, but it does not call usb_packet_cleanup() first.

Call usb_packet_cleanup() before g_free() so resources owned by USBPacket
are released.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3463

Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>
---
 hw/usb/hcd-ohci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 8f4de0066e..40ebafb4dd 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -755,6 +755,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
     } else {
         ret = pkt->status;
     }
+    usb_packet_cleanup(pkt);
     g_free(pkt);
 
     trace_usb_ohci_iso_td_so(start_offset, end_offset, start_addr, end_addr,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 2/2] hw/usb/hcd-ohci: Clean up USBPacket before freeing ISO TD packet

Posted by Peter Maydell 3 weeks, 2 days ago

On Wed, 6 May 2026 at 16:23, Munkhbaatar Enkhbaatar
<munkhuu0825@gmail.com> wrote:
>
> ohci_service_iso_td() allocates a USBPacket and frees it after synchronous
> completion, but it does not call usb_packet_cleanup() first.
>
> Call usb_packet_cleanup() before g_free() so resources owned by USBPacket
> are released.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3463
>
> Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>
> ---
>  hw/usb/hcd-ohci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 8f4de0066e..40ebafb4dd 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -755,6 +755,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>      } else {
>          ret = pkt->status;
>      }
> +    usb_packet_cleanup(pkt);
>      g_free(pkt);
>
>      trace_usb_ohci_iso_td_so(start_offset, end_offset, start_addr, end_addr,
> --
> 2.50.1 (Apple Git-155)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH] hw/usb/hcd-ohci: Fix memory leak in ohci_service_iso_td()

Posted by Marc-André Lureau 3 weeks, 4 days ago

+BALATON Zoltan

On Tue, May 5, 2026 at 3:01 PM <munkhuu0825@gmail.com> wrote:
>
> From: heropd <munkhuu0825@gmail.com>
>
> Fix a memory leak in ohci_service_iso_td() by calling
> usb_packet_cleanup() before freeing USBPacket.
>
> Signed-off-by: Munkhbaatar Enkhbaatar <munkhuu0825@gmail.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/usb/hcd-ohci.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 6ed8046fc2..43c49a42ec 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -748,6 +748,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>      usb_handle_packet(dev, pkt);
>      if (pkt->status == USB_RET_ASYNC) {
>          usb_device_flush_ep_queue(dev, ep);
> +        usb_packet_cleanup(pkt);
>          g_free(pkt);
>          return 1;
>      }
> @@ -756,6 +757,7 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed)
>      } else {
>          ret = pkt->status;
>      }
> +    usb_packet_cleanup(pkt);
>      g_free(pkt);
>
>      trace_usb_ohci_iso_td_so(start_offset, end_offset, start_addr, end_addr,
> --
> 2.50.1 (Apple Git-155)
>
>


-- 
Marc-André Lureau