Patchwork sky2: make sure ethernet header is in transmit skb

login
register
mail settings
Submitter Stephen Hemminger
Date Dec. 26, 2009, 10:05 p.m.
Message ID <20091226140544.5600b3d1@nehalam>
Download mbox | patch
Permalink /patch/41839/
State Deferred
Delegated to: David Miller
Headers show

Comments

Stephen Hemminger - Dec. 26, 2009, 10:05 p.m.
When DHCP sends a packet using AF_PACKET, it is possible for driver
to receive a transmit packet with the Ethernet header off in the
skb fragments.  The hardware doesn't handle 0 size DMA's well,
so it gets confused.

Other drivers may have same problem, I really think this ought
to be done at higher level.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Patch against net-2.6, but same patch applies to -stable as well
(with fuzz).

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 27, 2009, 3:44 a.m.
From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Sat, 26 Dec 2009 14:05:44 -0800

> Other drivers may have same problem, I really think this ought
> to be done at higher level.

I tend to agree with you, and I thought we had handled all
cases.  Let's simply make AF_PACKET linearize the link
level header before sending things out to the transmit path.

I can work on this if you want.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 27, 2009, 4:11 a.m.
From: David Miller <davem@davemloft.net>
Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST)

> From: Stephen Hemminger <shemminger@linux-foundation.org>
> Date: Sat, 26 Dec 2009 14:05:44 -0800
> 
>> Other drivers may have same problem, I really think this ought
>> to be done at higher level.
> 
> I tend to agree with you, and I thought we had handled all
> cases.  Let's simply make AF_PACKET linearize the link
> level header before sending things out to the transmit path.
> 
> I can work on this if you want.

Actually Stephen, I took a look and I can't see how AF_PACKET
can create this situation.

It always copies into the linear area of the SKB it allocates
for sendmsg() processing.  Whether the data comes from sendmsg
data or the mmap() ring buffer.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 4, 2010, 5:32 a.m.
From: David Miller <davem@davemloft.net>
Date: Sat, 26 Dec 2009 20:11:07 -0800 (PST)

> From: David Miller <davem@davemloft.net>
> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST)
> 
>> From: Stephen Hemminger <shemminger@linux-foundation.org>
>> Date: Sat, 26 Dec 2009 14:05:44 -0800
>> 
>>> Other drivers may have same problem, I really think this ought
>>> to be done at higher level.
>> 
>> I tend to agree with you, and I thought we had handled all
>> cases.  Let's simply make AF_PACKET linearize the link
>> level header before sending things out to the transmit path.
>> 
>> I can work on this if you want.
> 
> Actually Stephen, I took a look and I can't see how AF_PACKET
> can create this situation.
> 
> It always copies into the linear area of the SKB it allocates
> for sendmsg() processing.  Whether the data comes from sendmsg
> data or the mmap() ring buffer.

Stephen can you get a backtrace of the code path which triggers
this?  I want to fix it at a higher level too, but I can't
do that until I know where it actually happens.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Hemminger - Jan. 4, 2010, 4:40 p.m.
On Sun, 03 Jan 2010 21:32:58 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: David Miller <davem@davemloft.net>
> Date: Sat, 26 Dec 2009 20:11:07 -0800 (PST)
> 
> > From: David Miller <davem@davemloft.net>
> > Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST)
> > 
> >> From: Stephen Hemminger <shemminger@linux-foundation.org>
> >> Date: Sat, 26 Dec 2009 14:05:44 -0800
> >> 
> >>> Other drivers may have same problem, I really think this ought
> >>> to be done at higher level.
> >> 
> >> I tend to agree with you, and I thought we had handled all
> >> cases.  Let's simply make AF_PACKET linearize the link
> >> level header before sending things out to the transmit path.
> >> 
> >> I can work on this if you want.
> > 
> > Actually Stephen, I took a look and I can't see how AF_PACKET
> > can create this situation.
> > 
> > It always copies into the linear area of the SKB it allocates
> > for sendmsg() processing.  Whether the data comes from sendmsg
> > data or the mmap() ring buffer.
> 
> Stephen can you get a backtrace of the code path which triggers
> this?  I want to fix it at a higher level too, but I can't
> do that until I know where it actually happens.

Ignore it, the problem is outside the sky2 driver in some other
place causing corrupt skb's. I never reproduced this (with added BUG_ON
and WARN_ON), only seen by Michael.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Breuer - Jan. 4, 2010, 5:02 p.m.
On 1/4/2010 11:40 AM, Stephen Hemminger wrote:
> On Sun, 03 Jan 2010 21:32:58 -0800 (PST)
> David Miller<davem@davemloft.net>  wrote:
>
>    
>> From: David Miller<davem@davemloft.net>
>> Date: Sat, 26 Dec 2009 20:11:07 -0800 (PST)
>>
>>      
>>> From: David Miller<davem@davemloft.net>
>>> Date: Sat, 26 Dec 2009 19:44:18 -0800 (PST)
>>>
>>>        
>>>> From: Stephen Hemminger<shemminger@linux-foundation.org>
>>>> Date: Sat, 26 Dec 2009 14:05:44 -0800
>>>>
>>>>          
>>>>> Other drivers may have same problem, I really think this ought
>>>>> to be done at higher level.
>>>>>            
>>>> I tend to agree with you, and I thought we had handled all
>>>> cases.  Let's simply make AF_PACKET linearize the link
>>>> level header before sending things out to the transmit path.
>>>>
>>>> I can work on this if you want.
>>>>          
>>> Actually Stephen, I took a look and I can't see how AF_PACKET
>>> can create this situation.
>>>
>>> It always copies into the linear area of the SKB it allocates
>>> for sendmsg() processing.  Whether the data comes from sendmsg
>>> data or the mmap() ring buffer.
>>>        
>> Stephen can you get a backtrace of the code path which triggers
>> this?  I want to fix it at a higher level too, but I can't
>> do that until I know where it actually happens.
>>      
> Ignore it, the problem is outside the sky2 driver in some other
> place causing corrupt skb's. I never reproduced this (with added BUG_ON
> and WARN_ON), only seen by Michael.
>    
I've posted several oops with explanations:
http://lkml.org/lkml/2009/12/5/60
http://lkml.org/lkml/2009/12/21/268
http://lkml.org/lkml/2009/12/23/316

In a nutshell, my system was hanging (sometimes with a viewable oops, 
sometimes not (unrelated KMS issues). The hangs (with watchdog reboot) 
happened when under load and when any attached device sent in a DHCP 
request/offer.

The hang was 100% reproducible when running Microsoft Backup vrom a win7 
box via SAMBA onto the affected server. Stephen's patch stopped the hang 
and oops.

Please let me know what I can do to help.

Under the same load conditions (but no longer associated with DHCP) I'm 
now seeing multiple soft interrupt errors coming from sky2. This seems 
to be a race condition somewhere as it only occurs when a mingetty is 
run on tty1 prior to generating load on the sky2 driver. It could be a 
wild goose chase, but I think something is getting corrupted by either 
devpts or console when mingetty issues a vhangup on pts0.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/net/sky2.c	2009-12-26 13:39:34.289252942 -0800
+++ b/drivers/net/sky2.c	2009-12-26 13:59:44.548877690 -0800
@@ -1654,6 +1654,14 @@  static netdev_tx_t sky2_xmit_frame(struc
  	if (unlikely(tx_avail(sky2) < tx_le_req(skb)))
   		return NETDEV_TX_BUSY;
 
+	/* Make sure there is at least some data for first DMA */
+	if (!pskb_may_pull(skb, ETH_HLEN)) {
+		if (net_ratelimit())
+			pr_info(PFX "%s: packet missing ether header (%d)?",
+				dev->name, skb->len);
+		goto drop;
+	}
+
 	len = skb_headlen(skb);
 	mapping = pci_map_single(hw->pdev, skb->data, len, PCI_DMA_TODEVICE);
 
@@ -1799,6 +1807,7 @@  mapping_unwind:
 mapping_error:
 	if (net_ratelimit())
 		dev_warn(&hw->pdev->dev, "%s: tx mapping error\n", dev->name);
+drop:
 	dev_kfree_skb(skb);
 	return NETDEV_TX_OK;
 }