Re: race in usbnet.c in full RT

Re: race in usbnet.c in full RT

[Christoph Hellwig]

On Wed, Jun 08, 2005 at 02:21:39PM +0400, Eugeny S. Mints wrote:
> in non-RT case spin_lock_irqsave (&dev->txq.lock, flags) disables 
> interrupts and thus code from usb_submit_urb() call upto 
> __skb_queue_tail (&dev->txq, skb) executes atomically. But in RT case 
> interrupts are not disabled and usb_submit_urb() triggers an interrupt 
> which may cause tx_complete() execution before __skb_queue_tail () call. 
> And since skb->list gets initialized just at __skb_queue_tail(), call to 
> tx_complete() (via defer_bh() which thus executes before 
> __skb_queue_tail) dereferences NULL (skb->list) pointer.
> 
> Thus looks tx_complete() and usbnet_start_xmit() require a 
> serialization. Please find proposed fix attached though not sure the 
> patch will apply cleanly to the latest kernel.

Please fix whatever patch you use for "full RT mode" to not break valid
assupmtions in drivers.

race in usbnet.c in full RT

[Eugeny S. Mints]

[-- Attachment #1: Type: text/plain, Size: 2140 bytes --]

Hi,

seems there is a race in drivers/net/usbnet.c in full RT mode. To be 
honest I haven't hardly checked this on the latest kernel and latest RT 
patch but just took a look at usbnet.c and latest RT patch and haven't 
observed any related changes.

The  usbnet_start_xmit() contains the following code:

	...
	spin_lock_irqsave (&dev->txq.lock, flags);

#ifdef	CONFIG_USB_NET1080
	if (info->flags & FLAG_FRAMING_NC) {
		header->packet_id = cpu_to_le16 ((u16)dev->dev_packet_id++);
		put_unaligned (header->packet_id, &trailer->packet_id);
#if 0
		devdbg (dev, "frame >tx h %d p %d id %d",
			header->hdr_len, header->packet_len,
			header->packet_id);
#endif
	}
#endif	/* CONFIG_USB_NET1080 */

	switch ((retval = usb_submit_urb (urb, GFP_ATOMIC))) {
	case -EPIPE:
		netif_stop_queue (net);
		defer_kevent (dev, EVENT_TX_HALT);
		break;
	default:
		devdbg (dev, "tx: submit urb err %d", retval);
		break;
	case 0:
		net->trans_start = jiffies;
		__skb_queue_tail (&dev->txq, skb);
		if (dev->txq.qlen >= TX_QLEN (dev))
			netif_stop_queue (net);
	}
	spin_unlock_irqrestore (&dev->txq.lock, flags);
	...

THe race in full RT is between tx_complete() routine and 
__skb_queue_tail (&dev->txq, skb): skb->list gets initialized in 
__skb_queue_tail() but may be dereferenced before initialization at 
defer_bh() called from tx_complete() (since tx_complete() and 
usbnet_start_xmit() are completely asynchronous routines).

in non-RT case spin_lock_irqsave (&dev->txq.lock, flags) disables 
interrupts and thus code from usb_submit_urb() call upto 
__skb_queue_tail (&dev->txq, skb) executes atomically. But in RT case 
interrupts are not disabled and usb_submit_urb() triggers an interrupt 
which may cause tx_complete() execution before __skb_queue_tail () call. 
And since skb->list gets initialized just at __skb_queue_tail(), call to 
tx_complete() (via defer_bh() which thus executes before 
__skb_queue_tail) dereferences NULL (skb->list) pointer.

Thus looks tx_complete() and usbnet_start_xmit() require a 
serialization. Please find proposed fix attached though not sure the 
patch will apply cleanly to the latest kernel.

	Eugeny

[-- Attachment #2: usbnet.c.patch --]
[-- Type: text/plain, Size: 298 bytes --]

--- a/drivers/usb/net/usbnet.c	2004-12-25 00:34:58.000000000 +0300
+++ b/drivers/usb/net/usbnet.c	2005-05-26 21:02:58.000000000 +0400
@@ -2820,6 +2820,8 @@
 
 	urb->dev = NULL;
 	entry->state = tx_done;
+	spin_lock_rt (&dev->txq.lock);
+	spin_unlock_rt(&dev->txq.lock);
 	defer_bh (dev, skb);
 }
 

Re: race in usbnet.c in full RT

[Ingo Molnar]

* Eugeny S. Mints <emints@ru.mvista.com> wrote:

> seems there is a race in drivers/net/usbnet.c in full RT mode. To be 
> honest I haven't hardly checked this on the latest kernel and latest 
> RT patch but just took a look at usbnet.c and latest RT patch and 
> haven't observed any related changes.

thanks, i've applied your patch to my tree. Note that your patch is 
specific to the -RT kernel (both in terms of semantics and in term of 
API dependence), so it does not make any sense to apply it upstream.  
David, please ignore it.

	Ingo

Re: race in usbnet.c in full RT

[Eugeny S. Mints]

Ingo Molnar wrote:
> * Eugeny S. Mints <emints@ru.mvista.com> wrote:
> 
> 
>>seems there is a race in drivers/net/usbnet.c in full RT mode. To be 
>>honest I haven't hardly checked this on the latest kernel and latest 
>>RT patch but just took a look at usbnet.c and latest RT patch and 
>>haven't observed any related changes.
> 
> 
> thanks, i've applied your patch to my tree. Note that your patch is 
> specific to the -RT kernel (both in terms of semantics and in term of 
> API dependence), so it does not make any sense to apply it upstream.  
exactly. I put David in CC just to be sure usbnet.c has not been 
rewritten in the recent kerenls in sense of locking scheme.
	Eugeny
> David, please ignore it.
> 
> 	Ingo
> 
> 
> 

Re: race in usbnet.c in full RT

[Steven Rostedt]

On Wed, 2005-06-08 at 12:34 +0200, Ingo Molnar wrote:
> * Eugeny S. Mints <emints@ru.mvista.com> wrote:
> 
> > seems there is a race in drivers/net/usbnet.c in full RT mode. To be 
> > honest I haven't hardly checked this on the latest kernel and latest 
> > RT patch but just took a look at usbnet.c and latest RT patch and 
> > haven't observed any related changes.
> 
> thanks, i've applied your patch to my tree. Note that your patch is 
> specific to the -RT kernel (both in terms of semantics and in term of 
> API dependence), so it does not make any sense to apply it upstream.  
> David, please ignore it.
> 

Is this action only take place on the same CPU, or is this also an SMP
problem?  I would think if this is a race with full RT, that this may
also be a race with SMP, unless the race is guaranteed to always happen
on the same CPU. Then this is only a RT problem.

-- Steve

Re: race in usbnet.c in full RT

[Eugeny S. Mints]

Steven Rostedt wrote:
> On Wed, 2005-06-08 at 12:34 +0200, Ingo Molnar wrote:
> 
>>* Eugeny S. Mints <emints@ru.mvista.com> wrote:
>>
>>
>>>seems there is a race in drivers/net/usbnet.c in full RT mode. To be 
>>>honest I haven't hardly checked this on the latest kernel and latest 
>>>RT patch but just took a look at usbnet.c and latest RT patch and 
>>>haven't observed any related changes.
>>
>>thanks, i've applied your patch to my tree. Note that your patch is 
>>specific to the -RT kernel (both in terms of semantics and in term of 
>>API dependence), so it does not make any sense to apply it upstream.  
>>David, please ignore it.
>>
> 
> 
> Is this action only take place on the same CPU, or is this also an SMP
> problem?  I would think if this is a race with full RT, that this may
> also be a race with SMP, unless the race is guaranteed to always happen
> on the same CPU. Then this is only a RT problem.
thanks, good point. looks like it could be SMP problem but probably 
David is able to say it for sure as usb host code expert.
David?
	Eugeny
> 
> -- Steve
> 
> 
> 
>