e1000e: allocate ring descriptors with dma_zalloc_coherent

Message ID 20180126102404.GA26578@poseidon.lin.mbt.kalray.eu
State Accepted
Delegated to: Jeff Kirsher
Headers show
Series
  • e1000e: allocate ring descriptors with dma_zalloc_coherent
Related show

Commit Message

Pierre-Yves Kerbrat Jan. 26, 2018, 10:24 a.m.
Descriptor rings were not initialized at zero when allocated
When area contained garbage data, it caused skb_over_panic in
e1000_clean_rx_irq (if data had E1000_RXD_STAT_DD bit set)

This patch makes use of dma_zalloc_coherent to make sure the
ring is memset at 0 to prevent the area from containing garbage.

Following is the signature of the panic:
IODDR0@0.0: skbuff: skb_over_panic: text:80407b20 len:64010 put:64010 head:ab46d800 data:ab46d842 tail:0xab47d24c end:0xab46df40 dev:eth0
IODDR0@0.0: BUG: failure at net/core/skbuff.c:105/skb_panic()!
IODDR0@0.0: Kernel panic - not syncing: BUG!
IODDR0@0.0:
IODDR0@0.0: Process swapper/0 (pid: 0, threadinfo=81728000, task=8173cc00 ,cpu: 0)
IODDR0@0.0: SP = <815a1c0c>
IODDR0@0.0: Stack:      00000001
IODDR0@0.0: b2d89800 815e33ac
IODDR0@0.0: ea73c040 00000001
IODDR0@0.0: 60040003 0000fa0a
IODDR0@0.0: 00000002
IODDR0@0.0:
IODDR0@0.0: 804540c0 815a1c70
IODDR0@0.0: b2744000 602ac070
IODDR0@0.0: 815a1c44 b2d89800
IODDR0@0.0: 8173cc00 815a1c08
IODDR0@0.0:
IODDR0@0.0:     00000006
IODDR0@0.0: 815a1b50 00000000
IODDR0@0.0: 80079434 00000001
IODDR0@0.0: ab46df40 b2744000
IODDR0@0.0: b2d89800
IODDR0@0.0:
IODDR0@0.0: 0000fa0a 8045745c
IODDR0@0.0: 815a1c88 0000fa0a
IODDR0@0.0: 80407b20 b2789f80
IODDR0@0.0: 00000005 80407b20
IODDR0@0.0:
IODDR0@0.0:
IODDR0@0.0: Call Trace:
IODDR0@0.0: [<804540bc>] skb_panic+0xa4/0xa8
IODDR0@0.0: [<80079430>] console_unlock+0x2f8/0x6d0
IODDR0@0.0: [<80457458>] skb_put+0xa0/0xc0
IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
IODDR0@0.0: [<804079c8>] e1000_clean_rx_irq+0x188/0x3e8
IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
IODDR0@0.0: [<80468b48>] __dev_kfree_skb_any+0x88/0xa8
IODDR0@0.0: [<804101ac>] e1000e_poll+0x94/0x288
IODDR0@0.0: [<8046e9d4>] net_rx_action+0x19c/0x4e8
IODDR0@0.0:   ...
IODDR0@0.0: Maximum depth to print reached. Use kstack=<maximum_depth_to_print> To specify a custom value (where 0 means to display the full backtrace)
IODDR0@0.0: ---[ end Kernel panic - not syncing: BUG!

Signed-off-by: Pierre-Yves Kerbrat <pkerbrat@kalray.eu>
Signed-off-by: Marius Gligor <mgligor@kalray.eu>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Brown, Aaron F Feb. 3, 2018, 1:59 a.m. | #1
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Pierre-Yves Kerbrat
> Sent: Friday, January 26, 2018 2:24 AM
> To: Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Marius Gligor <mgligor@kalray.eu>
> Subject: [PATCH] e1000e: allocate ring descriptors with dma_zalloc_coherent
> 
> Descriptor rings were not initialized at zero when allocated
> When area contained garbage data, it caused skb_over_panic in
> e1000_clean_rx_irq (if data had E1000_RXD_STAT_DD bit set)
> 
> This patch makes use of dma_zalloc_coherent to make sure the
> ring is memset at 0 to prevent the area from containing garbage.
> 
> Following is the signature of the panic:
> IODDR0@0.0: skbuff: skb_over_panic: text:80407b20 len:64010 put:64010
> head:ab46d800 data:ab46d842 tail:0xab47d24c end:0xab46df40 dev:eth0
> IODDR0@0.0: BUG: failure at net/core/skbuff.c:105/skb_panic()!
> IODDR0@0.0: Kernel panic - not syncing: BUG!
> IODDR0@0.0:
> IODDR0@0.0: Process swapper/0 (pid: 0, threadinfo=81728000,
> task=8173cc00 ,cpu: 0)
> IODDR0@0.0: SP = <815a1c0c>
> IODDR0@0.0: Stack:      00000001
> IODDR0@0.0: b2d89800 815e33ac
> IODDR0@0.0: ea73c040 00000001
> IODDR0@0.0: 60040003 0000fa0a
> IODDR0@0.0: 00000002
> IODDR0@0.0:
> IODDR0@0.0: 804540c0 815a1c70
> IODDR0@0.0: b2744000 602ac070
> IODDR0@0.0: 815a1c44 b2d89800
> IODDR0@0.0: 8173cc00 815a1c08
> IODDR0@0.0:
> IODDR0@0.0:     00000006
> IODDR0@0.0: 815a1b50 00000000
> IODDR0@0.0: 80079434 00000001
> IODDR0@0.0: ab46df40 b2744000
> IODDR0@0.0: b2d89800
> IODDR0@0.0:
> IODDR0@0.0: 0000fa0a 8045745c
> IODDR0@0.0: 815a1c88 0000fa0a
> IODDR0@0.0: 80407b20 b2789f80
> IODDR0@0.0: 00000005 80407b20
> IODDR0@0.0:
> IODDR0@0.0:
> IODDR0@0.0: Call Trace:
> IODDR0@0.0: [<804540bc>] skb_panic+0xa4/0xa8
> IODDR0@0.0: [<80079430>] console_unlock+0x2f8/0x6d0
> IODDR0@0.0: [<80457458>] skb_put+0xa0/0xc0
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<804079c8>] e1000_clean_rx_irq+0x188/0x3e8
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<80468b48>] __dev_kfree_skb_any+0x88/0xa8
> IODDR0@0.0: [<804101ac>] e1000e_poll+0x94/0x288
> IODDR0@0.0: [<8046e9d4>] net_rx_action+0x19c/0x4e8
> IODDR0@0.0:   ...
> IODDR0@0.0: Maximum depth to print reached. Use
> kstack=<maximum_depth_to_print> To specify a custom value (where 0
> means to display the full backtrace)
> IODDR0@0.0: ---[ end Kernel panic - not syncing: BUG!
> 
> Signed-off-by: Pierre-Yves Kerbrat <pkerbrat@kalray.eu>
> Signed-off-by: Marius Gligor <mgligor@kalray.eu>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Neftin, Sasha Feb. 4, 2018, 8:09 a.m. | #2
On 1/26/2018 12:24, Pierre-Yves Kerbrat wrote:
> Descriptor rings were not initialized at zero when allocated
> When area contained garbage data, it caused skb_over_panic in
> e1000_clean_rx_irq (if data had E1000_RXD_STAT_DD bit set)
> 
> This patch makes use of dma_zalloc_coherent to make sure the
> ring is memset at 0 to prevent the area from containing garbage.
> 
> Following is the signature of the panic:
> IODDR0@0.0: skbuff: skb_over_panic: text:80407b20 len:64010 put:64010 head:ab46d800 data:ab46d842 tail:0xab47d24c end:0xab46df40 dev:eth0
> IODDR0@0.0: BUG: failure at net/core/skbuff.c:105/skb_panic()!
> IODDR0@0.0: Kernel panic - not syncing: BUG!
> IODDR0@0.0:
> IODDR0@0.0: Process swapper/0 (pid: 0, threadinfo=81728000, task=8173cc00 ,cpu: 0)
> IODDR0@0.0: SP = <815a1c0c>
> IODDR0@0.0: Stack:      00000001
> IODDR0@0.0: b2d89800 815e33ac
> IODDR0@0.0: ea73c040 00000001
> IODDR0@0.0: 60040003 0000fa0a
> IODDR0@0.0: 00000002
> IODDR0@0.0:
> IODDR0@0.0: 804540c0 815a1c70
> IODDR0@0.0: b2744000 602ac070
> IODDR0@0.0: 815a1c44 b2d89800
> IODDR0@0.0: 8173cc00 815a1c08
> IODDR0@0.0:
> IODDR0@0.0:     00000006
> IODDR0@0.0: 815a1b50 00000000
> IODDR0@0.0: 80079434 00000001
> IODDR0@0.0: ab46df40 b2744000
> IODDR0@0.0: b2d89800
> IODDR0@0.0:
> IODDR0@0.0: 0000fa0a 8045745c
> IODDR0@0.0: 815a1c88 0000fa0a
> IODDR0@0.0: 80407b20 b2789f80
> IODDR0@0.0: 00000005 80407b20
> IODDR0@0.0:
> IODDR0@0.0:
> IODDR0@0.0: Call Trace:
> IODDR0@0.0: [<804540bc>] skb_panic+0xa4/0xa8
> IODDR0@0.0: [<80079430>] console_unlock+0x2f8/0x6d0
> IODDR0@0.0: [<80457458>] skb_put+0xa0/0xc0
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<804079c8>] e1000_clean_rx_irq+0x188/0x3e8
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<80468b48>] __dev_kfree_skb_any+0x88/0xa8
> IODDR0@0.0: [<804101ac>] e1000e_poll+0x94/0x288
> IODDR0@0.0: [<8046e9d4>] net_rx_action+0x19c/0x4e8
> IODDR0@0.0:   ...
> IODDR0@0.0: Maximum depth to print reached. Use kstack=<maximum_depth_to_print> To specify a custom value (where 0 means to display the full backtrace)
> IODDR0@0.0: ---[ end Kernel panic - not syncing: BUG!
> 
> Signed-off-by: Pierre-Yves Kerbrat <pkerbrat@kalray.eu>
> Signed-off-by: Marius Gligor <mgligor@kalray.eu>
> ---
>   drivers/net/ethernet/intel/e1000e/netdev.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 1298b69..26121ed 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -2333,7 +2333,7 @@ static int e1000_alloc_ring_dma(struct e1000_adapter *adapter,
>   {
>   	struct pci_dev *pdev = adapter->pdev;
>   
> -	ring->desc = dma_alloc_coherent(&pdev->dev, ring->size, &ring->dma,
> +	ring->desc = dma_zalloc_coherent(&pdev->dev, ring->size, &ring->dma,
>   					GFP_KERNEL);
>   	if (!ring->desc)
>   		return -ENOMEM;
> 
Looks good. Prefer get another opinion from somebody else.
Florian Fainelli Feb. 4, 2018, 8:01 p.m. | #3
On 01/26/2018 02:24 AM, Pierre-Yves Kerbrat wrote:
> Descriptor rings were not initialized at zero when allocated
> When area contained garbage data, it caused skb_over_panic in
> e1000_clean_rx_irq (if data had E1000_RXD_STAT_DD bit set)
> 
> This patch makes use of dma_zalloc_coherent to make sure the
> ring is memset at 0 to prevent the area from containing garbage.
> 
> Following is the signature of the panic:
> IODDR0@0.0: skbuff: skb_over_panic: text:80407b20 len:64010 put:64010 head:ab46d800 data:ab46d842 tail:0xab47d24c end:0xab46df40 dev:eth0
> IODDR0@0.0: BUG: failure at net/core/skbuff.c:105/skb_panic()!
> IODDR0@0.0: Kernel panic - not syncing: BUG!
> IODDR0@0.0:
> IODDR0@0.0: Process swapper/0 (pid: 0, threadinfo=81728000, task=8173cc00 ,cpu: 0)
> IODDR0@0.0: SP = <815a1c0c>
> IODDR0@0.0: Stack:      00000001
> IODDR0@0.0: b2d89800 815e33ac
> IODDR0@0.0: ea73c040 00000001
> IODDR0@0.0: 60040003 0000fa0a
> IODDR0@0.0: 00000002
> IODDR0@0.0:
> IODDR0@0.0: 804540c0 815a1c70
> IODDR0@0.0: b2744000 602ac070
> IODDR0@0.0: 815a1c44 b2d89800
> IODDR0@0.0: 8173cc00 815a1c08
> IODDR0@0.0:
> IODDR0@0.0:     00000006
> IODDR0@0.0: 815a1b50 00000000
> IODDR0@0.0: 80079434 00000001
> IODDR0@0.0: ab46df40 b2744000
> IODDR0@0.0: b2d89800
> IODDR0@0.0:
> IODDR0@0.0: 0000fa0a 8045745c
> IODDR0@0.0: 815a1c88 0000fa0a
> IODDR0@0.0: 80407b20 b2789f80
> IODDR0@0.0: 00000005 80407b20
> IODDR0@0.0:
> IODDR0@0.0:
> IODDR0@0.0: Call Trace:
> IODDR0@0.0: [<804540bc>] skb_panic+0xa4/0xa8
> IODDR0@0.0: [<80079430>] console_unlock+0x2f8/0x6d0
> IODDR0@0.0: [<80457458>] skb_put+0xa0/0xc0
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<804079c8>] e1000_clean_rx_irq+0x188/0x3e8
> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
> IODDR0@0.0: [<80468b48>] __dev_kfree_skb_any+0x88/0xa8
> IODDR0@0.0: [<804101ac>] e1000e_poll+0x94/0x288
> IODDR0@0.0: [<8046e9d4>] net_rx_action+0x19c/0x4e8
> IODDR0@0.0:   ...
> IODDR0@0.0: Maximum depth to print reached. Use kstack=<maximum_depth_to_print> To specify a custom value (where 0 means to display the full backtrace)
> IODDR0@0.0: ---[ end Kernel panic - not syncing: BUG!

Interesting, this dates back from the driver's initial commit, I am
surprised that not more people did not have that problem, maybe the RX
ring usually goes through at least one filing cycle?

Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently
for ICH9 devices only)")
Alexander Duyck Feb. 4, 2018, 8:44 p.m. | #4
On Sun, Feb 4, 2018 at 12:01 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
> On 01/26/2018 02:24 AM, Pierre-Yves Kerbrat wrote:
>> Descriptor rings were not initialized at zero when allocated
>> When area contained garbage data, it caused skb_over_panic in
>> e1000_clean_rx_irq (if data had E1000_RXD_STAT_DD bit set)
>>
>> This patch makes use of dma_zalloc_coherent to make sure the
>> ring is memset at 0 to prevent the area from containing garbage.
>>
>> Following is the signature of the panic:
>> IODDR0@0.0: skbuff: skb_over_panic: text:80407b20 len:64010 put:64010 head:ab46d800 data:ab46d842 tail:0xab47d24c end:0xab46df40 dev:eth0
>> IODDR0@0.0: BUG: failure at net/core/skbuff.c:105/skb_panic()!
>> IODDR0@0.0: Kernel panic - not syncing: BUG!
>> IODDR0@0.0:
>> IODDR0@0.0: Process swapper/0 (pid: 0, threadinfo=81728000, task=8173cc00 ,cpu: 0)
>> IODDR0@0.0: SP = <815a1c0c>
>> IODDR0@0.0: Stack:      00000001
>> IODDR0@0.0: b2d89800 815e33ac
>> IODDR0@0.0: ea73c040 00000001
>> IODDR0@0.0: 60040003 0000fa0a
>> IODDR0@0.0: 00000002
>> IODDR0@0.0:
>> IODDR0@0.0: 804540c0 815a1c70
>> IODDR0@0.0: b2744000 602ac070
>> IODDR0@0.0: 815a1c44 b2d89800
>> IODDR0@0.0: 8173cc00 815a1c08
>> IODDR0@0.0:
>> IODDR0@0.0:     00000006
>> IODDR0@0.0: 815a1b50 00000000
>> IODDR0@0.0: 80079434 00000001
>> IODDR0@0.0: ab46df40 b2744000
>> IODDR0@0.0: b2d89800
>> IODDR0@0.0:
>> IODDR0@0.0: 0000fa0a 8045745c
>> IODDR0@0.0: 815a1c88 0000fa0a
>> IODDR0@0.0: 80407b20 b2789f80
>> IODDR0@0.0: 00000005 80407b20
>> IODDR0@0.0:
>> IODDR0@0.0:
>> IODDR0@0.0: Call Trace:
>> IODDR0@0.0: [<804540bc>] skb_panic+0xa4/0xa8
>> IODDR0@0.0: [<80079430>] console_unlock+0x2f8/0x6d0
>> IODDR0@0.0: [<80457458>] skb_put+0xa0/0xc0
>> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
>> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
>> IODDR0@0.0: [<804079c8>] e1000_clean_rx_irq+0x188/0x3e8
>> IODDR0@0.0: [<80407b1c>] e1000_clean_rx_irq+0x2dc/0x3e8
>> IODDR0@0.0: [<80468b48>] __dev_kfree_skb_any+0x88/0xa8
>> IODDR0@0.0: [<804101ac>] e1000e_poll+0x94/0x288
>> IODDR0@0.0: [<8046e9d4>] net_rx_action+0x19c/0x4e8
>> IODDR0@0.0:   ...
>> IODDR0@0.0: Maximum depth to print reached. Use kstack=<maximum_depth_to_print> To specify a custom value (where 0 means to display the full backtrace)
>> IODDR0@0.0: ---[ end Kernel panic - not syncing: BUG!
>
> Interesting, this dates back from the driver's initial commit, I am
> surprised that not more people did not have that problem, maybe the RX
> ring usually goes through at least one filing cycle?
>
> Fixes: bc7f75fa9788 ("[E1000E]: New pci-express e1000 driver (currently
> for ICH9 devices only)")

The Rx rings should have been filled long before we triggered this. I
would really want to see more of the call trace before we say this
fixes the bug. For instance I would be curious to see the link
messages and such from the interface. I'm not entirely convinced since
really this does get overwritten by the alloc_rx_buffers function.
Really in order to get into this state I think we would have to have a
significant number of skb allocations and/or the DMA mappings for the
skbs fail.

I'm okay with the patch since it is harmless and just zeroing out the
length field and DD bit in the descriptor. But I would want to know
more information about the architecture and how we got into this state
since it seems like this is an issue that could happen with numerous
possible causes and this addressing only one.

One concern I would have is that we are running into something that is
really more of a race issue, as we have seen in the past with PowerPC,
where the length was getting read before the DD bit due to the
pipeline optimizing things. In that case we had to introduce a barrer
that later became the dma_rmb().

Anyway that is just my $.02 on it. I am good with the patch itself and
I am okay with it being applied.

Reviewed-by: Alexander Duyck <alexander.h.duyck@intel.com>

Patch

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 1298b69..26121ed 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -2333,7 +2333,7 @@  static int e1000_alloc_ring_dma(struct e1000_adapter *adapter,
 {
 	struct pci_dev *pdev = adapter->pdev;
 
-	ring->desc = dma_alloc_coherent(&pdev->dev, ring->size, &ring->dma,
+	ring->desc = dma_zalloc_coherent(&pdev->dev, ring->size, &ring->dma,
 					GFP_KERNEL);
 	if (!ring->desc)
 		return -ENOMEM;