QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
@ 2019-07-30 17:45 Guenter Roeck
  2019-07-31 11:08 ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2019-07-30 17:45 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Paolo Bonzini, qemu-devel, Guenter Roeck, Marc-André Lureau

The following assert is seen once in a while while resetting the
Linux kernel.

qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
	Assertion `dev != NULL' failed.

The call to usb_ep_get() originates from ehci_execute().
Analysis and debugging shows that p->queue->dev can indeed be NULL
in this function. Add check for this condition and return an error
if it is seen.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/usb/hcd-ehci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 62dab05..c759f3e 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1348,6 +1348,11 @@ static int ehci_execute(EHCIPacket *p, const char *action)
         return -1;
     }
 
+    if (p->queue->dev == NULL) {
+        ehci_trace_guest_bug(p->queue->ehci, "No device attached to queue\n");
+        return -1;
+    }
+
     if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
         ehci_trace_guest_bug(p->queue->ehci,
                              "guest requested more bytes than allowed");
-- 
2.7.4



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-07-30 17:45 [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get Guenter Roeck
@ 2019-07-31 11:08 ` Philippe Mathieu-Daudé
  2019-07-31 21:11   ` Guenter Roeck
  2019-08-02 14:11   ` Gerd Hoffmann
  0 siblings, 2 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-31 11:08 UTC (permalink / raw)
  To: Guenter Roeck, Gerd Hoffmann
  Cc: Paolo Bonzini, qemu-devel, Marc-André Lureau

On 7/30/19 7:45 PM, Guenter Roeck wrote:
> The following assert is seen once in a while while resetting the
> Linux kernel.
> 
> qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> 	Assertion `dev != NULL' failed.
> 
> The call to usb_ep_get() originates from ehci_execute().
> Analysis and debugging shows that p->queue->dev can indeed be NULL
> in this function. Add check for this condition and return an error
> if it is seen.

Your patch is not wrong as it corrects your case, but I wonder why we
get there. This assert seems to have catched a bug.

Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
ehci_finalize()? Then we shouldn't need this patch.

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/usb/hcd-ehci.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 62dab05..c759f3e 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1348,6 +1348,11 @@ static int ehci_execute(EHCIPacket *p, const char *action)
>          return -1;
>      }
>  
> +    if (p->queue->dev == NULL) {
> +        ehci_trace_guest_bug(p->queue->ehci, "No device attached to queue\n");
> +        return -1;
> +    }
> +
>      if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
>          ehci_trace_guest_bug(p->queue->ehci,
>                               "guest requested more bytes than allowed");
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-07-31 11:08 ` Philippe Mathieu-Daudé
@ 2019-07-31 21:11   ` Guenter Roeck
  2019-08-02 14:11   ` Gerd Hoffmann
  1 sibling, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2019-07-31 21:11 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Paolo Bonzini, Gerd Hoffmann, Marc-André Lureau, qemu-devel

On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > The following assert is seen once in a while while resetting the
> > Linux kernel.
> > 
> > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > 	Assertion `dev != NULL' failed.
> > 
> > The call to usb_ep_get() originates from ehci_execute().
> > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > in this function. Add check for this condition and return an error
> > if it is seen.
> 
> Your patch is not wrong as it corrects your case, but I wonder why we
> get there. This assert seems to have catched a bug.
> 
> Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> ehci_finalize()? Then we shouldn't need this patch.
> 


If you send me an alternate patch, I'll be happy to insert it
into my build to give it some test coverage.

Thanks,
Guenter

> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/usb/hcd-ehci.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 62dab05..c759f3e 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -1348,6 +1348,11 @@ static int ehci_execute(EHCIPacket *p, const char *action)
> >          return -1;
> >      }
> >  
> > +    if (p->queue->dev == NULL) {
> > +        ehci_trace_guest_bug(p->queue->ehci, "No device attached to queue\n");
> > +        return -1;
> > +    }
> > +
> >      if (get_field(p->qtd.token, QTD_TOKEN_TBYTES) > BUFF_SIZE) {
> >          ehci_trace_guest_bug(p->queue->ehci,
> >                               "guest requested more bytes than allowed");
> > 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-07-31 11:08 ` Philippe Mathieu-Daudé
  2019-07-31 21:11   ` Guenter Roeck
@ 2019-08-02 14:11   ` Gerd Hoffmann
  2019-08-02 16:46     ` Guenter Roeck
  2019-08-06 13:23     ` Guenter Roeck
  1 sibling, 2 replies; 9+ messages in thread
From: Gerd Hoffmann @ 2019-08-02 14:11 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Paolo Bonzini, qemu-devel, Guenter Roeck, Marc-André Lureau

On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > The following assert is seen once in a while while resetting the
> > Linux kernel.
> > 
> > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > 	Assertion `dev != NULL' failed.
> > 
> > The call to usb_ep_get() originates from ehci_execute().
> > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > in this function. Add check for this condition and return an error
> > if it is seen.
> 
> Your patch is not wrong as it corrects your case, but I wonder why we
> get there. This assert seems to have catched a bug.

https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.

> Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> ehci_finalize()? Then we shouldn't need this patch.

The two ehci_queues_rip_all() calls in ehci_reset() should clean up everything
properly.

Can you try the patch below to see whenever a ehci_find_device failure is the
root cause?

thanks,
  Gerd

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 62dab0592fa2..2b0a57772ed5 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
         q->dev = ehci_find_device(q->ehci,
                                   get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
     }
+    if (q->dev == NULL) {
+        fprintf(stderr, "%s: device %d not found\n", __func__,
+                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
+    }
 
     if (async && (q->qh.epchar & QH_EPCHAR_H)) {
 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-08-02 14:11   ` Gerd Hoffmann
@ 2019-08-02 16:46     ` Guenter Roeck
  2019-08-02 17:28       ` Guenter Roeck
  2019-08-06 13:23     ` Guenter Roeck
  1 sibling, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2019-08-02 16:46 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
	qemu-devel, Marc-André Lureau

On Fri, Aug 02, 2019 at 04:11:49PM +0200, Gerd Hoffmann wrote:
> On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> > On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > > The following assert is seen once in a while while resetting the
> > > Linux kernel.
> > > 
> > > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > > 	Assertion `dev != NULL' failed.
> > > 
> > > The call to usb_ep_get() originates from ehci_execute().
> > > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > > in this function. Add check for this condition and return an error
> > > if it is seen.
> > 
> > Your patch is not wrong as it corrects your case, but I wonder why we
> > get there. This assert seems to have catched a bug.
> 
> https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.
> 
> > Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> > ehci_finalize()? Then we shouldn't need this patch.
> 
> The two ehci_queues_rip_all() calls in ehci_reset() should clean up everything
> properly.
> 
> Can you try the patch below to see whenever a ehci_find_device failure is the
> root cause?
> 
> thanks,
>   Gerd
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 62dab0592fa2..2b0a57772ed5 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
>          q->dev = ehci_find_device(q->ehci,
>                                    get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
>      }
> +    if (q->dev == NULL) {
> +        fprintf(stderr, "%s: device %d not found\n", __func__,
> +                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> +    }

I had tried that, but this does happen as standard behavior for some
architectures (I didn't write down where exactly since I thought it
must be normal). But, sure, I'll add a log message.

Guenter

>  
>      if (async && (q->qh.epchar & QH_EPCHAR_H)) {
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-08-02 16:46     ` Guenter Roeck
@ 2019-08-02 17:28       ` Guenter Roeck
  0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2019-08-02 17:28 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
	qemu-devel, Marc-André Lureau

On Fri, Aug 02, 2019 at 09:46:26AM -0700, Guenter Roeck wrote:
> On Fri, Aug 02, 2019 at 04:11:49PM +0200, Gerd Hoffmann wrote:
> > On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > > > The following assert is seen once in a while while resetting the
> > > > Linux kernel.
> > > > 
> > > > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > > > 	Assertion `dev != NULL' failed.
> > > > 
> > > > The call to usb_ep_get() originates from ehci_execute().
> > > > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > > > in this function. Add check for this condition and return an error
> > > > if it is seen.
> > > 
> > > Your patch is not wrong as it corrects your case, but I wonder why we
> > > get there. This assert seems to have catched a bug.
> > 
> > https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.
> > 
> > > Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> > > ehci_finalize()? Then we shouldn't need this patch.
> > 
> > The two ehci_queues_rip_all() calls in ehci_reset() should clean up everything
> > properly.
> > 
> > Can you try the patch below to see whenever a ehci_find_device failure is the
> > root cause?
> > 
> > thanks,
> >   Gerd
> > 
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 62dab0592fa2..2b0a57772ed5 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
> >          q->dev = ehci_find_device(q->ehci,
> >                                    get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> >      }
> > +    if (q->dev == NULL) {
> > +        fprintf(stderr, "%s: device %d not found\n", __func__,
> > +                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> > +    }
> 
> I had tried that, but this does happen as standard behavior for some
> architectures (I didn't write down where exactly since I thought it
> must be normal). But, sure, I'll add a log message.
> 

With the log message added, I see it a lot when booting riscv64 images
from usb-ehci.

ehci_state_fetchqh: device 0 not found

It looks like this happens for each usb access (a whopping 800+ times
for a simple boot test). So it is definitely a very common condition.
The relevant qemu command line is something like

	-usb -device usb-ehci,id=ehci -device usb-storage,bus=ehci.0,drive=d0 \
	-drive file=rootfs.ext2,if=none,id=d0,format=raw

The image works fine otherwise, so I thought that the condition is normal.

Guenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-08-02 14:11   ` Gerd Hoffmann
  2019-08-02 16:46     ` Guenter Roeck
@ 2019-08-06 13:23     ` Guenter Roeck
  2019-08-13 11:42       ` Gerd Hoffmann
  1 sibling, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2019-08-06 13:23 UTC (permalink / raw)
  To: Gerd Hoffmann, Philippe Mathieu-Daudé
  Cc: Paolo Bonzini, qemu-devel, Marc-André Lureau

On 8/2/19 7:11 AM, Gerd Hoffmann wrote:
> On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
>> On 7/30/19 7:45 PM, Guenter Roeck wrote:
>>> The following assert is seen once in a while while resetting the
>>> Linux kernel.
>>>
>>> qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
>>> 	Assertion `dev != NULL' failed.
>>>
>>> The call to usb_ep_get() originates from ehci_execute().
>>> Analysis and debugging shows that p->queue->dev can indeed be NULL
>>> in this function. Add check for this condition and return an error
>>> if it is seen.
>>
>> Your patch is not wrong as it corrects your case, but I wonder why we
>> get there. This assert seems to have catched a bug.
> 
> https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.
> 
>> Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
>> ehci_finalize()? Then we shouldn't need this patch.
> 
> The two ehci_queues_rip_all() calls in ehci_reset() should clean up everything
> properly.
> 
> Can you try the patch below to see whenever a ehci_find_device failure is the
> root cause?
> 
> thanks,
>    Gerd
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 62dab0592fa2..2b0a57772ed5 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
>           q->dev = ehci_find_device(q->ehci,
>                                     get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
>       }
> +    if (q->dev == NULL) {
> +        fprintf(stderr, "%s: device %d not found\n", __func__,
> +                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> +    }
>   
Turns out I end up seeing that message hundreds of times early on each boot,
no matter which architecture. It is quite obviously a normal operating condition.

Guenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-08-06 13:23     ` Guenter Roeck
@ 2019-08-13 11:42       ` Gerd Hoffmann
  2019-08-14 14:41         ` Guenter Roeck
  0 siblings, 1 reply; 9+ messages in thread
From: Gerd Hoffmann @ 2019-08-13 11:42 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
	qemu-devel, Marc-André Lureau

On Tue, Aug 06, 2019 at 06:23:38AM -0700, Guenter Roeck wrote:
> On 8/2/19 7:11 AM, Gerd Hoffmann wrote:
> > On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > > > The following assert is seen once in a while while resetting the
> > > > Linux kernel.
> > > > 
> > > > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > > > 	Assertion `dev != NULL' failed.
> > > > 
> > > > The call to usb_ep_get() originates from ehci_execute().
> > > > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > > > in this function. Add check for this condition and return an error
> > > > if it is seen.
> > > 
> > > Your patch is not wrong as it corrects your case, but I wonder why we
> > > get there. This assert seems to have catched a bug.
> > 
> > https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.
> > 
> > > Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> > > ehci_finalize()? Then we shouldn't need this patch.
> > 
> > The two ehci_queues_rip_all() calls in ehci_reset() should clean up everything
> > properly.
> > 
> > Can you try the patch below to see whenever a ehci_find_device failure is the
> > root cause?
> > 
> > thanks,
> >    Gerd
> > 
> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > index 62dab0592fa2..2b0a57772ed5 100644
> > --- a/hw/usb/hcd-ehci.c
> > +++ b/hw/usb/hcd-ehci.c
> > @@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
> >           q->dev = ehci_find_device(q->ehci,
> >                                     get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> >       }
> > +    if (q->dev == NULL) {
> > +        fprintf(stderr, "%s: device %d not found\n", __func__,
> > +                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> > +    }
> Turns out I end up seeing that message hundreds of times early on each boot,
> no matter which architecture. It is quite obviously a normal operating condition.

Yep, as long as the queue is not active this is completely harmless.
So we need to check a bit later.  In execute() looks a bit too late
though, we don't have a good backup plan then.

Does the patch below solve the problem without bad side effects?

thanks,
  Gerd

From 5980eaad23f675a2d509d0c55e288793619761bc Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 13 Aug 2019 13:37:09 +0200
Subject: [PATCH] ehci: try fix queue->dev null ptr dereference

Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-ehci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 62dab0592fa2..5f089f30054b 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -1834,6 +1834,9 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
             ehci_set_state(q->ehci, q->async, EST_EXECUTING);
             break;
         }
+    } else if (q->dev == NULL) {
+        ehci_trace_guest_bug(q->ehci, "no device attached to queue");
+        ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
     } else {
         p = ehci_alloc_packet(q);
         p->qtdaddr = q->qtdaddr;
-- 
2.18.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get
  2019-08-13 11:42       ` Gerd Hoffmann
@ 2019-08-14 14:41         ` Guenter Roeck
  0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2019-08-14 14:41 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: Paolo Bonzini, Philippe Mathieu-Daudé,
	qemu-devel, Marc-André Lureau

On Tue, Aug 13, 2019 at 01:42:03PM +0200, Gerd Hoffmann wrote:
> On Tue, Aug 06, 2019 at 06:23:38AM -0700, Guenter Roeck wrote:
> > On 8/2/19 7:11 AM, Gerd Hoffmann wrote:
> > > On Wed, Jul 31, 2019 at 01:08:50PM +0200, Philippe Mathieu-Daudé wrote:
> > > > On 7/30/19 7:45 PM, Guenter Roeck wrote:
> > > > > The following assert is seen once in a while while resetting the
> > > > > Linux kernel.
> > > > > 
> > > > > qemu-system-x86_64: hw/usb/core.c:734: usb_ep_get:
> > > > > 	Assertion `dev != NULL' failed.
> > > > > 
> > > > > The call to usb_ep_get() originates from ehci_execute().
> > > > > Analysis and debugging shows that p->queue->dev can indeed be NULL
> > > > > in this function. Add check for this condition and return an error
> > > > > if it is seen.
> > > > 
> > > > Your patch is not wrong as it corrects your case, but I wonder why we
> > > > get there. This assert seems to have catched a bug.
> > > 
> > > https://bugzilla.redhat.com//show_bug.cgi?id=1715801 maybe.
> > > 
> > > > Gerd, shouldn't we call usb_packet_cleanup() in ehci_reset() rather than
> > > > ehci_finalize()? Then we shouldn't need this patch.
> > > 
> > > The two ehci_queues_rip_all() calls in ehci_reset() should clean up everything
> > > properly.
> > > 
> > > Can you try the patch below to see whenever a ehci_find_device failure is the
> > > root cause?
> > > 
> > > thanks,
> > >    Gerd
> > > 
> > > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> > > index 62dab0592fa2..2b0a57772ed5 100644
> > > --- a/hw/usb/hcd-ehci.c
> > > +++ b/hw/usb/hcd-ehci.c
> > > @@ -1644,6 +1644,10 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async)
> > >           q->dev = ehci_find_device(q->ehci,
> > >                                     get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> > >       }
> > > +    if (q->dev == NULL) {
> > > +        fprintf(stderr, "%s: device %d not found\n", __func__,
> > > +                get_field(q->qh.epchar, QH_EPCHAR_DEVADDR));
> > > +    }
> > Turns out I end up seeing that message hundreds of times early on each boot,
> > no matter which architecture. It is quite obviously a normal operating condition.
> 
> Yep, as long as the queue is not active this is completely harmless.
> So we need to check a bit later.  In execute() looks a bit too late
> though, we don't have a good backup plan then.
> 
> Does the patch below solve the problem without bad side effects?
> 
I reverted my patch and applied the patch below to my builds of qemu, for both
v4.0 and v4.1, and installed it in my test bed. I'll let you know how it goes.

Thanks,
Guenter

> thanks,
>   Gerd
> 
> From 5980eaad23f675a2d509d0c55e288793619761bc Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Tue, 13 Aug 2019 13:37:09 +0200
> Subject: [PATCH] ehci: try fix queue->dev null ptr dereference
> 
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/hcd-ehci.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 62dab0592fa2..5f089f30054b 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -1834,6 +1834,9 @@ static int ehci_state_fetchqtd(EHCIQueue *q)
>              ehci_set_state(q->ehci, q->async, EST_EXECUTING);
>              break;
>          }
> +    } else if (q->dev == NULL) {
> +        ehci_trace_guest_bug(q->ehci, "no device attached to queue");
> +        ehci_set_state(q->ehci, q->async, EST_HORIZONTALQH);
>      } else {
>          p = ehci_alloc_packet(q);
>          p->qtdaddr = q->qtdaddr;
> -- 
> 2.18.1
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-30 17:45 [Qemu-devel] [PATCH] ehci: Ensure that device is not NULL before calling usb_ep_get Guenter Roeck
2019-07-31 11:08 ` Philippe Mathieu-Daudé
2019-07-31 21:11   ` Guenter Roeck
2019-08-02 14:11   ` Gerd Hoffmann
2019-08-02 16:46     ` Guenter Roeck
2019-08-02 17:28       ` Guenter Roeck
2019-08-06 13:23     ` Guenter Roeck
2019-08-13 11:42       ` Gerd Hoffmann
2019-08-14 14:41         ` Guenter Roeck

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox