Re: Possible race condition in ohci-hcd.c

On Po, 2007-03-12 at 16:06 -0400, Alan Stern wrote:
> On Sun, 11 Mar 2007, Milan Plzik wrote:
> 
> >   Hello, 
> > 
> >   I noticed that there could potentially be a sort of race condition in
> > ohci-hcd.c file, related to interrupt handling. This caused Oops when
> > removing ohci_hcd driver on one device I was working with - OHCI cell on
> > SAMCOP chip used in ipaq h5000 devices. Please Cc: me as I am not
> > subscribed to linux-usb-devel. Bugreport below:
> > 
> >   Summary: ohci-hcd module can possibly crash when unloading due race 
> >   condition.
> > 
> >   Description:
> >   In usb_remove_hcd there is possible place where race condition can
> > occur. Problematic is caused by usb_hcd_poll_rh_status, which is
> > normally called periodically from timer, or from ohci_irq handler.
> > Removing HCD uses roughly following algorithm:
> > 
> >  1) delete rh_timer, so it should not be called again
> >  2) call ohci_stop, which does some magic and then disables interrupts.
> > 
> >   The problem is within the "some magic" part. Its execution takes some
> > time and within this time, OHCI irq can possibly occur. This results in
> > call of usb_hcd_poll_rh_status, which again calls timer_mod and
> > re-enables rh_timer. Consecutively, we now have ohci driver which is
> > short before unloading, but has active timer. This results in paging
> > fault.
> > 
> >   It is possible that the interrupt occurs because of ohci_usb_reset
> > called just prior to disabling interrupts, but I have no idea whether
> > this is real. This problem also might or might not be visible depending
> > on debugging turned on or off.
> 
> Yes, this looks like a real bug.  Does it happen reproducibly on your 
> device?  

  Yes, the crash was fully deterministic - to reproduce it, it was
enough to do rmmod ohci-hcd (and maybe have one device on the bus
active, I don't remember exactly:).

> Does the patch below fix it?

  Again yes, thanks:). I removed my old hotfix and rmmod-ing did not
cause crash. To be sure, I also tried to reproduce the bug without patch
and it crashed.

> 
> Alan Stern

	Milan Plzik

> 
> Index: usb-2.6/drivers/usb/core/hcd.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/hcd.c
> +++ usb-2.6/drivers/usb/core/hcd.c
> @@ -544,6 +544,8 @@ void usb_hcd_poll_rh_status(struct usb_h
>  	unsigned long	flags;
>  	char		buffer[4];	/* Any root hubs with > 31 ports? */
>  
> +	if (unlikely(!hcd->rh_registered))
> +		return;
>  	if (!hcd->uses_new_polling && !hcd->status_urb)
>  		return;
>  
> @@ -1670,12 +1672,12 @@ void usb_remove_hcd(struct usb_hcd *hcd)
>  	usb_disconnect(&hcd->self.root_hub);
>  	mutex_unlock(&usb_bus_list_lock);
>  
> -	hcd->poll_rh = 0;
> -	del_timer_sync(&hcd->rh_timer);
> -
>  	hcd->driver->stop(hcd);
>  	hcd->state = HC_STATE_HALT;
>  
> +	hcd->poll_rh = 0;
> +	del_timer_sync(&hcd->rh_timer);
> +
>  	if (hcd->irq >= 0)
>  		free_irq(hcd->irq, hcd);
>  	usb_deregister_bus(&hcd->self);
> 


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel