| 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 | expand |
> 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>
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.
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)")
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>
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;