| Message ID | 20201029081057.8506-1-claudiu.manoil@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2,1/2] gianfar: Replace skb_realloc_headroom with skb_cow_head for PTP | expand |
On Thu, 29 Oct 2020 10:10:56 +0200 Claudiu Manoil wrote: > When PTP timestamping is enabled on Tx, the controller > inserts the Tx timestamp at the beginning of the frame > buffer, between SFD and the L2 frame header. This means > that the skb provided by the stack is required to have > enough headroom otherwise a new skb needs to be created > by the driver to accommodate the timestamp inserted by h/w. > Up until now the driver was relying on skb_realloc_headroom() > to create new skbs to accommodate PTP frames. Turns out that > this method is not reliable in this context at least, as > skb_realloc_headroom() for PTP frames can cause random crashes, > mostly in subsequent skb_*() calls, when multiple concurrent > TCP streams are run at the same time with the PTP flow > on the same device (as seen in James' report). I also noticed > that when the system is loaded by sending multiple TCP streams, > the driver receives cloned skbs in large numbers. > skb_cow_head() instead proves to be stable in this scenario, > and not only handles cloned skbs too but it's also more efficient > and widely used in other drivers. > The commit introducing skb_realloc_headroom in the driver > goes back to 2009, commit 93c1285c5d92 > ("gianfar: reallocate skb when headroom is not enough for fcb"). > For practical purposes I'm referencing a newer commit (from 2012) > that brings the code to its current structure (and fixes the PTP > case). > > Fixes: 9c4886e5e63b ("gianfar: Fix invalid TX frames returned on error queue when time stamping") > Reported-by: James Jurack <james.jurack@ametek.com> > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> Applied both, thank you!
On Thu, Oct 29, 2020 at 10:10:56AM +0200, Claudiu Manoil wrote: > When PTP timestamping is enabled on Tx, the controller > inserts the Tx timestamp at the beginning of the frame > buffer, between SFD and the L2 frame header. This means > that the skb provided by the stack is required to have > enough headroom otherwise a new skb needs to be created > by the driver to accommodate the timestamp inserted by h/w. > Up until now the driver was relying on skb_realloc_headroom() > to create new skbs to accommodate PTP frames. Turns out that > this method is not reliable in this context at least, as > skb_realloc_headroom() for PTP frames can cause random crashes, > mostly in subsequent skb_*() calls, when multiple concurrent > TCP streams are run at the same time with the PTP flow > on the same device (as seen in James' report). I also noticed > that when the system is loaded by sending multiple TCP streams, > the driver receives cloned skbs in large numbers. > skb_cow_head() instead proves to be stable in this scenario, > and not only handles cloned skbs too but it's also more efficient > and widely used in other drivers. > The commit introducing skb_realloc_headroom in the driver > goes back to 2009, commit 93c1285c5d92 > ("gianfar: reallocate skb when headroom is not enough for fcb"). > For practical purposes I'm referencing a newer commit (from 2012) > that brings the code to its current structure (and fixes the PTP > case). > > Fixes: 9c4886e5e63b ("gianfar: Fix invalid TX frames returned on error queue when time stamping") > Reported-by: James Jurack <james.jurack@ametek.com> > Suggested-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> > --- Still crashes for me: [root@LS1021ATSN ~] # ./ptp4l -i eth1 -f /etc/ptp4l_cfg/gPTP.cfg --tx_timestamp_timeout 20 & [1] 887 [root@LS1021ATSN ~] # ./perf record -e cycles iperf3 -c 192.168.1.2 -t 100 Connecting to host 192.168.1.2, port 5201 [ 5] local 192.168.1.1 port 59152 connected to 192.168.1.2 port 5201 [ ID] Interval Transfer Bitrate Retr Cwnd [ 5] 0.00-1.00 sec 111 MBytes 932 Mbits/sec 0 267 KBytes [ 5] 1.00-2.00 sec 111 MBytes 935 Mbits/sec 0 298 KBytes [ 5] 2.00-3.00 sec 112 MBytes 941 Mbits/sec 0 325 KBytes [ 5] 3.00-4.00 sec 112 MBytes 938 Mbits/sec 0 344 KBytes [ 5] 4.00-5.01 sec 108 MBytes 898 Mbits/sec 0 352 KBytes [ 5] 5.01-6.00 sec 93.8 MBytes 789 Mbits/sec 0 354 KBytes [ 5] 6.00-7.00 sec 93.8 MBytes 786 Mbits/sec 0 354 KBytes [ 5] 7.00-8.01 sec 101 MBytes 844 Mbits/sec 0 369 KBytes [ 5] 8.01-9.00 sec 107 MBytes 910 Mbits/sec 0 373 KBytes [ 5] 9.00-10.00 sec 113 MBytes 944 Mbits/sec 0 396 KBytes [ 5] 10.00-11.00 sec 112 MBytes 939 Mbits/sec 0 436 KBytes [ 5] 11.00-12.00 sec 112 MBytes 939 Mbits/sec 0 436 KBytes [ 5] 12.00-13.00 sec 113 MBytes 944 Mbits/sec 0 462 KBytes [ 5] 13.00-14.01 sec 96.0 MBytes 799 Mbits/sec 0 492 KBytes [ 5] 14.01-15.01 sec 93.8 MBytes 788 Mbits/sec 0 570 KBytes [ 5] 15.01-16.00 sec 106 MBytes 894 Mbits/sec 0 708 KBytes [ 5] 16.00-17.00 sec 107 MBytes 898 Mbits/sec 0 844 KBytes [ 5] 17.00-18.00 sec 105 MBytes 882 Mbits/sec 0 997 KBytes [ 5] 18.00-19.00 sec 103 MBytes 863 Mbits/sec 0 1.07 MBytes [14538.007252] 8<--- cut here --- [14538.010310] Unable to handle kernel NULL pointer dereference at virtual address 00000008 [14538.018395] pgd = d79f0b3d [14538.021108] [00000008] *pgd=80000080204003, *pmd=00000000 [14538.026553] Internal error: Oops: 207 [#1] SMP ARM [14538.031324] Modules linked in: [14538.034368] CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.10.0-rc1-00296-g37442a47b604 #707 [14538.042672] Hardware name: Freescale LS1021A [14538.046926] PC is at skb_release_data+0x6c/0x14c [14538.051518] LR is at consume_skb+0x38/0xd8 [14538.055588] pc : [<c10e439c>] lr : [<c10e3fac>] psr: 200f0013 [14538.061817] sp : c28f1da8 ip : 00000000 fp : c265aa40 [14538.067010] r10: 00000000 r9 : 00000000 r8 : c2f98000 [14538.072204] r7 : c511d900 r6 : c3d3d900 r5 : c3d3d900 r4 : 00000000 [14538.078693] r3 : 000000d3 r2 : 00000001 r1 : 00000000 r0 : 00000000 [14538.085184] Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [14538.092279] Control: 30c5387d Table: 848c7d00 DAC: fffffffd [14538.097994] Process ksoftirqd/0 (pid: 9, stack limit = 0x6d78b0e1) [14538.104139] Stack: (0xc28f1da8 to 0xc28f2000) [14538.108472] 1da0: c511d900 c511d900 c5271a80 c2f1f900 c2f98000 c10e3fac [14538.116606] 1dc0: c2f6d600 c0d529bc c28f1ecc c04aa7d4 00000001 337a26ba 00000018 c2f6e36c [14538.124741] 1de0: 00000000 c04aa838 ec05c800 c2406f78 f088c000 c04aee10 c236edac 00000045 [14538.132876] 1e00: c236edac c511d900 c2f98000 00000000 00000000 c2f1f900 00000044 c2403d00 [14538.141011] 1e20: c265aa40 c10fcdf8 c0d51064 600f0013 c265aa60 c236eda4 c24087f0 ffffe000 [14538.149146] 1e40: c240675c c28f1e78 c2f98600 c511d900 c4b95000 c2406708 00000000 c2f1f900 [14538.157280] 1e60: c2f98000 00000000 c2403080 c1158524 c28f1ecc 0065ac80 00000010 337a26ba [14538.165415] 1e80: c4b95000 00000001 c511d900 00000040 c2f1f900 00000000 c265ae20 c1158858 [14538.173549] 1ea0: 00000000 00000000 c2f98608 c4b95040 c265ae20 ffffe000 c240675c 2903d000 [14538.181684] 1ec0: ffffe000 c4b95000 00000000 00000000 00000000 ffffe000 c2654040 00000100 [14538.189818] 1ee0: c2403080 c10f959c c2403088 00000003 0000000c 00000002 ffffe000 c2654040 [14538.197954] 1f00: 00000100 c04013f0 00000001 c1404ab4 c2364390 c236fdc0 c240675c 00000009 [14538.206088] 1f20: c236431c 0015ba25 c2403d00 c1608068 04208040 c28db200 ffffe000 c28ab240 [14538.214222] 1f40: ffffe000 00000000 00000001 c24261a4 00000002 00000000 c28ab2e4 c0450180 [14538.222357] 1f60: c28ab240 c047022c c28ab2c0 c28ab280 00000000 c28f0000 c047011c c28ab240 [14538.230491] 1f80: c28cbe34 c046c5c4 00000001 c28ab280 c046c478 00000000 00000000 00000000 [14538.238625] 1fa0: 00000000 00000000 00000000 c0400278 00000000 00000000 00000000 00000000 [14538.246758] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [14538.254892] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [14538.263039] [<c10e439c>] (skb_release_data) from [<c10e3fac>] (consume_skb+0x38/0xd8) [14538.270834] [<c10e3fac>] (consume_skb) from [<c0d529bc>] (gfar_start_xmit+0x704/0x784) [14538.278714] [<c0d529bc>] (gfar_start_xmit) from [<c10fcdf8>] (dev_hard_start_xmit+0xfc/0x254) [14538.287198] [<c10fcdf8>] (dev_hard_start_xmit) from [<c1158524>] (sch_direct_xmit+0x104/0x2e0) [14538.295767] [<c1158524>] (sch_direct_xmit) from [<c1158858>] (__qdisc_run+0x158/0x5fc) [14538.303644] [<c1158858>] (__qdisc_run) from [<c10f959c>] (net_tx_action+0x144/0x2b8) [14538.311350] [<c10f959c>] (net_tx_action) from [<c04013f0>] (__do_softirq+0x130/0x3b8) [14538.319145] [<c04013f0>] (__do_softirq) from [<c0450180>] (run_ksoftirqd+0x2c/0x38) [14538.326766] [<c0450180>] (run_ksoftirqd) from [<c047022c>] (smpboot_thread_fn+0x110/0x1a8) [14538.334991] [<c047022c>] (smpboot_thread_fn) from [<c046c5c4>] (kthread+0x14c/0x150) [14538.342696] [<c046c5c4>] (kthread) from [<c0400278>] (ret_from_fork+0x14/0x3c) [14538.349877] Exception stack(0xc28f1fb0 to 0xc28f1ff8) [14538.354898] 1fa0: 00000000 00000000 00000000 00000000 [14538.363032] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [14538.371164] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [14538.377744] Code: 11a05006 13a04000 0a000014 e5950028 (e5903008) [14538.383877] ---[ end trace bb4785b99c68319a ]--- [14538.388529] Kernel panic - not syncing: Fatal exception in interrupt [14538.394858] CPU1: stopping [14538.397555] CPU: 1 PID: 907 Comm: iperf3 Tainted: G D 5.10.0-rc1-00296-g37442a47b604 #707 [14538.406981] Hardware name: Freescale LS1021A [14538.411238] [<c04120b8>] (unwind_backtrace) from [<c040c554>] (show_stack+0x10/0x14) [14538.418946] [<c040c554>] (show_stack) from [<c13fabfc>] (dump_stack+0xc8/0xdc) [14538.426134] [<c13fabfc>] (dump_stack) from [<c040f83c>] (do_handle_IPI+0x320/0x33c) [14538.433752] [<c040f83c>] (do_handle_IPI) from [<c040f870>] (ipi_handler+0x18/0x20) [14538.441286] [<c040f870>] (ipi_handler) from [<c04b0028>] (handle_percpu_devid_fasteoi_ipi+0x80/0x150) [14538.450462] [<c04b0028>] (handle_percpu_devid_fasteoi_ipi) from [<c04a981c>] (generic_handle_irq+0x34/0x44) [14538.460156] [<c04a981c>] (generic_handle_irq) from [<c04a9e08>] (__handle_domain_irq+0x5c/0xb4) [14538.468814] [<c04a9e08>] (__handle_domain_irq) from [<c08f89dc>] (gic_handle_irq+0x94/0xb4) [14538.477125] [<c08f89dc>] (gic_handle_irq) from [<c0400c38>] (__irq_svc+0x58/0x74) [14538.484564] Exception stack(0xc4fdbdd0 to 0xc4fdbe18) [14538.489587] bdc0: c26e27e8 a00e0013 0000000f 0000f4f6 [14538.497721] bde0: 00000000 c2afb000 00000000 00000002 00000fff c26e27e8 c3e65e00 c1e55086 [14538.505854] be00: 0000000a c4fdbe20 c0ad6bb0 c1409670 800e0013 ffffffff [14538.512437] [<c0400c38>] (__irq_svc) from [<c1409670>] (_raw_spin_unlock_irqrestore+0x1c/0x20) [14538.521007] [<c1409670>] (_raw_spin_unlock_irqrestore) from [<c0ad6bb0>] (uart_write+0xf4/0x1f0) [14538.529749] [<c0ad6bb0>] (uart_write) from [<c0ab759c>] (do_output_char+0x160/0x1e4) [14538.537453] [<c0ab759c>] (do_output_char) from [<c0ab83ac>] (n_tty_write+0x228/0x480) [14538.545243] [<c0ab83ac>] (n_tty_write) from [<c0ab6168>] (tty_write+0x12c/0x33c) [14538.552604] [<c0ab6168>] (tty_write) from [<c0602d28>] (vfs_write+0xc4/0x334) [14538.559705] [<c0602d28>] (vfs_write) from [<c0603114>] (ksys_write+0xa4/0xd4) [14538.566805] [<c0603114>] (ksys_write) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) [14538.574417] Exception stack(0xc4fdbfa8 to 0xc4fdbff0) [14538.579440] bfa0: 0000004f 00022bd0 00000001 00022bd0 0000004f 00000000 [14538.587574] bfc0: 0000004f 00022bd0 b6f0e6d0 00000004 0000004f b6ed4f7d b6ed2e2c 00000000 [14538.595707] bfe0: 00000004 be9e1d18 b6b810f7 b6b0c856 [14538.600736] Rebooting in 3 seconds.. Takes a while to reproduce though.
On Tue, 3 Nov 2020 18:13:19 +0200 Vladimir Oltean wrote: > [14538.046926] PC is at skb_release_data+0x6c/0x14c > [14538.051518] LR is at consume_skb+0x38/0xd8 > [14538.055588] pc : [<c10e439c>] lr : [<c10e3fac>] psr: 200f0013 > [14538.061817] sp : c28f1da8 ip : 00000000 fp : c265aa40 > [14538.067010] r10: 00000000 r9 : 00000000 r8 : c2f98000 > [14538.072204] r7 : c511d900 r6 : c3d3d900 r5 : c3d3d900 r4 : 00000000 > [14538.078693] r3 : 000000d3 r2 : 00000001 r1 : 00000000 r0 : 00000000 > [14538.263039] [<c10e439c>] (skb_release_data) from [<c10e3fac>] (consume_skb+0x38/0xd8) > [14538.270834] [<c10e3fac>] (consume_skb) from [<c0d529bc>] (gfar_start_xmit+0x704/0x784) > [14538.278714] [<c0d529bc>] (gfar_start_xmit) from [<c10fcdf8>] (dev_hard_start_xmit+0xfc/0x254) > [14538.287198] [<c10fcdf8>] (dev_hard_start_xmit) from [<c1158524>] (sch_direct_xmit+0x104/0x2e0) Looks like one of the error paths freeing a wonky skb. Could you decode these addresses to line numbers?
On 03.11.20 18:13, Vladimir Oltean wrote: > On Thu, Oct 29, 2020 at 10:10:56AM +0200, Claudiu Manoil wrote: >> When PTP timestamping is enabled on Tx, the controller >> inserts the Tx timestamp at the beginning of the frame >> buffer, between SFD and the L2 frame header. This means >> that the skb provided by the stack is required to have >> enough headroom otherwise a new skb needs to be created >> by the driver to accommodate the timestamp inserted by h/w. >> Up until now the driver was relying on skb_realloc_headroom() >> to create new skbs to accommodate PTP frames. Turns out that >> this method is not reliable in this context at least, as >> skb_realloc_headroom() for PTP frames can cause random crashes, >> mostly in subsequent skb_*() calls, when multiple concurrent >> TCP streams are run at the same time with the PTP flow >> on the same device (as seen in James' report). I also noticed >> that when the system is loaded by sending multiple TCP streams, >> the driver receives cloned skbs in large numbers. >> skb_cow_head() instead proves to be stable in this scenario, >> and not only handles cloned skbs too but it's also more efficient >> and widely used in other drivers. >> The commit introducing skb_realloc_headroom in the driver >> goes back to 2009, commit 93c1285c5d92 >> ("gianfar: reallocate skb when headroom is not enough for fcb"). >> For practical purposes I'm referencing a newer commit (from 2012) >> that brings the code to its current structure (and fixes the PTP >> case). >> >> Fixes: 9c4886e5e63b ("gianfar: Fix invalid TX frames returned on error queue when time stamping") >> Reported-by: James Jurack <james.jurack@ametek.com> >> Suggested-by: Jakub Kicinski <kuba@kernel.org> >> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> >> --- > > Still crashes for me: > Given the various skb modifications in its xmit path, I wonder why gianfar doesn't clear IFF_TX_SKB_SHARING.
>-----Original Message----- >From: Julian Wiedmann <jwi@linux.ibm.com> >Sent: Tuesday, November 3, 2020 6:37 PM >To: Vladimir Oltean <olteanv@gmail.com>; Claudiu Manoil ><claudiu.manoil@nxp.com> >Cc: netdev@vger.kernel.org; David S . Miller <davem@davemloft.net>; Jakub >Kicinski <kuba@kernel.org>; james.jurack@ametek.com >Subject: Re: [PATCH net v2 1/2] gianfar: Replace skb_realloc_headroom with >skb_cow_head for PTP > >On 03.11.20 18:13, Vladimir Oltean wrote: [...] >> >> Still crashes for me: >> > >Given the various skb modifications in its xmit path, I wonder why >gianfar doesn't clear IFF_TX_SKB_SHARING. Hi Vladimir, Can you try the above suggestion on your setup? Thanks.
On Tue, 3 Nov 2020 17:08:43 +0000 Claudiu Manoil wrote: > >> Still crashes for me: > >> > > > >Given the various skb modifications in its xmit path, I wonder why > >gianfar doesn't clear IFF_TX_SKB_SHARING. > > Hi Vladimir, > Can you try the above suggestion on your setup? While it is something to be fixed - is anything other than pktgen making use of IFF_TX_SKB_SHARING? Are you running pktgen, Vladimir?
>-----Original Message----- >From: Jakub Kicinski <kuba@kernel.org> >Sent: Tuesday, November 3, 2020 6:31 PM >To: Vladimir Oltean <olteanv@gmail.com> >Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; netdev@vger.kernel.org; >David S . Miller <davem@davemloft.net>; james.jurack@ametek.com >Subject: Re: [PATCH net v2 1/2] gianfar: Replace skb_realloc_headroom with >skb_cow_head for PTP > >On Tue, 3 Nov 2020 18:13:19 +0200 Vladimir Oltean wrote: >> [14538.046926] PC is at skb_release_data+0x6c/0x14c >> [14538.051518] LR is at consume_skb+0x38/0xd8 >> [14538.055588] pc : [<c10e439c>] lr : [<c10e3fac>] psr: 200f0013 >> [14538.061817] sp : c28f1da8 ip : 00000000 fp : c265aa40 >> [14538.067010] r10: 00000000 r9 : 00000000 r8 : c2f98000 >> [14538.072204] r7 : c511d900 r6 : c3d3d900 r5 : c3d3d900 r4 : 00000000 >> [14538.078693] r3 : 000000d3 r2 : 00000001 r1 : 00000000 r0 : 00000000 > >> [14538.263039] [<c10e439c>] (skb_release_data) from [<c10e3fac>] >(consume_skb+0x38/0xd8) >> [14538.270834] [<c10e3fac>] (consume_skb) from [<c0d529bc>] >(gfar_start_xmit+0x704/0x784) >> [14538.278714] [<c0d529bc>] (gfar_start_xmit) from [<c10fcdf8>] >(dev_hard_start_xmit+0xfc/0x254) >> [14538.287198] [<c10fcdf8>] (dev_hard_start_xmit) from [<c1158524>] >(sch_direct_xmit+0x104/0x2e0) > >Looks like one of the error paths freeing a wonky skb. > >Could you decode these addresses to line numbers? It's either the dev_kfree_skb_any from the dma mapping error path or the one from skb_cow_head()'s error path. A confirmation would help indeed.
On Tue, Nov 03, 2020 at 06:36:30PM +0200, Julian Wiedmann wrote: > Given the various skb modifications in its xmit path, I wonder why > gianfar doesn't clear IFF_TX_SKB_SHARING. Thanks for the hint Julian :) I'll try to see if it makes any difference. Just a wild guess, but maybe because the gianfar maintainer (or me) had no idea about the existence of the flag, and because IFF_SKB_TX_SHARED was added _after_ gianfar was already a thing (which it was since the beginning of git), which is insane to begin with? commit d8873315065f1f527c7c380402cf59b1e1d0ae36 Author: Neil Horman <nhorman@tuxdriver.com> Date: Tue Jul 26 06:05:37 2011 +0000 net: add IFF_SKB_TX_SHARED flag to priv_flags Pktgen attempts to transmit shared skbs to net devices, which can't be used by some drivers as they keep state information in skbs. This patch adds a flag marking drivers as being able to handle shared skbs in their tx path. Drivers are defaulted to being unable to do so, but calling ether_setup enables this flag, as 90% of the drivers calling ether_setup touch real hardware and can handle shared skbs. A subsequent patch will audit drivers to ensure that the flag is set properly Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Reported-by: Jiri Pirko <jpirko@redhat.com> CC: Robert Olsson <robert.olsson@its.uu.se> CC: Eric Dumazet <eric.dumazet@gmail.com> CC: Alexey Dobriyan <adobriyan@gmail.com> CC: David S. Miller <davem@davemloft.net> Signed-off-by: David S. Miller <davem@davemloft.net>
On Tue, Nov 03, 2020 at 09:12:26AM -0800, Jakub Kicinski wrote: > While it is something to be fixed - is anything other than pktgen > making use of IFF_TX_SKB_SHARING? Are you running pktgen, Vladimir? Nope, just iperf3 TCP and PTP. The problem is actually with PTP, I've been testing gianfar with just TCP for a long while.
On Tue, Nov 03, 2020 at 07:18:49PM +0200, Vladimir Oltean wrote: > On Tue, Nov 03, 2020 at 06:36:30PM +0200, Julian Wiedmann wrote: > > Given the various skb modifications in its xmit path, I wonder why > > gianfar doesn't clear IFF_TX_SKB_SHARING. > > Thanks for the hint Julian :) I'll try to see if it makes any > difference. And unsurprisingly given what Jakub said, it crashed again, even with this change. diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 41dd3d0f3452..7cc8986910f8 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -3337,6 +3337,7 @@ static int gfar_probe(struct platform_device *ofdev) dev->mtu = 1500; dev->min_mtu = 50; dev->max_mtu = GFAR_JUMBO_FRAME_SIZE - ETH_HLEN; + dev->priv_flags &= ~IFF_TX_SKB_SHARING; dev->netdev_ops = &gfar_netdev_ops; dev->ethtool_ops = &gfar_ethtool_ops;
On Tue, Nov 03, 2020 at 05:18:25PM +0000, Claudiu Manoil wrote: > It's either the dev_kfree_skb_any from the dma mapping error path or the one > from skb_cow_head()'s error path. A confirmation would help indeed. It says "consume", not "kfree", which in my mind would make it point towards the only caller of consume_skb from the gianfar driver, i.e. the dev_consume_skb_any that you just added.
On Tue, 3 Nov 2020 19:30:07 +0200 Vladimir Oltean wrote: > On Tue, Nov 03, 2020 at 05:18:25PM +0000, Claudiu Manoil wrote: > > It's either the dev_kfree_skb_any from the dma mapping error path or the one > > from skb_cow_head()'s error path. A confirmation would help indeed. > > It says "consume", not "kfree", which in my mind would make it point > towards the only caller of consume_skb from the gianfar driver, i.e. the > dev_consume_skb_any that you just added. #define dev_kfree_skb(a) consume_skb(a) IIRC we did this because too many drivers used dev_kfree_skb incorrectly and made the dropwatch output very noisy.
On Tue, Nov 03, 2020 at 07:24:41PM +0200, Vladimir Oltean wrote: > On Tue, Nov 03, 2020 at 09:12:26AM -0800, Jakub Kicinski wrote: > > While it is something to be fixed - is anything other than pktgen > > making use of IFF_TX_SKB_SHARING? Are you running pktgen, Vladimir? > > Nope, just iperf3 TCP and PTP. The problem is actually with PTP, I've > been testing gianfar with just TCP for a long while. Now it fails like this too, under exactly the same traffic load: [ 113.937369] 8<--- cut here --- [ 113.940474] Unable to handle kernel NULL pointer dereference at virtual address 00000004 [ 113.948569] pgd = 4340cf4f [ 113.951282] [00000004] *pgd=80000080204003, *pmd=00000000 [ 113.956692] Internal error: Oops: a07 [#1] SMP ARM [ 113.961459] Modules linked in: [ 113.964500] CPU: 0 PID: 9 Comm: ksoftirqd/0 Not tainted 5.10.0-rc1-00297-g2d388e050508-dirty #709 [ 113.973322] Hardware name: Freescale LS1021A [ 113.977574] PC is at __qdisc_run+0x2c4/0x5fc [ 113.981819] LR is at __qdisc_run+0x408/0x5fc [ 113.986061] pc : [<c1158a44>] lr : [<c1158b88>] psr: 600f0013 [ 113.992291] sp : c28f1988 ip : 200f0013 fp : 0000002c [ 113.997484] r10: c265ae20 r9 : 00000000 r8 : c2f38700 [ 114.002677] r7 : 00000040 r6 : c4d9d900 r5 : c3d3b05c r4 : c3d3b000 [ 114.009167] r3 : 00000000 r2 : c28f1d30 r1 : 00000000 r0 : c3d3b05c [ 114.015657] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 114.022752] Control: 30c5387d Table: 84936540 DAC: fffffffd [ 114.028468] Process ksoftirqd/0 (pid: 9, stack limit = 0xb7de1ef0) [ 114.034613] Stack: (0xc28f1988 to 0xc28f2000) [ 114.038946] 1980: 00000000 00000001 7dfff8a0 c3d3b040 c265ae20 ffffe000 [ 114.047081] 19a0: c240675c 00000000 c4d5d138 c4d5d138 c3d3b000 c2406708 00000000 c2f7a000 [ 114.055216] 19c0: c2f38600 00010a22 0000002c c10fd2c8 c2fc36a0 c4aab50c c4d5d138 c1229434 [ 114.063351] 19e0: 000048af c1f2c684 c2fc3400 fffffff4 00000000 c2406708 00000000 c2feb540 [ 114.071485] 1a00: 00000000 e5243720 00000000 c3d5c600 00000000 c4d5d138 c2406708 00000010 [ 114.079620] 1a20: c3d5c68c 0000feca 00000000 c11ca978 c4d5d138 c2619480 c4909500 c11cba48 [ 114.087755] 1a40: 0201a8c0 e5243720 00000000 c4d5d138 c2406708 c2619480 c2f7a000 c4909500 [ 114.095890] 1a60: 00000000 467d5a22 00000000 c11cd44c c3daa280 00000001 00000004 00000002 [ 114.104024] 1a80: 00000000 c2f7a000 c4909500 c2619480 c11cbb4c e5243720 000010f8 c4d5d138 [ 114.112159] 1aa0: c4909500 c49097a0 00000045 00000000 c2619480 c3daa280 c4d5d080 c11cce10 [ 114.120294] 1ac0: c2406708 c11e4ab8 c28f1ba0 c28f1ba0 00001004 c2406708 00071bdc e5243720 [ 114.128429] 1ae0: 7e069980 c4909500 c4d5d138 ffffb750 0000fe88 c4d5d150 c2406708 0000002d [ 114.136564] 1b00: c4d5d080 c11eae60 00000000 c28f1b34 c49095fc 00034ac8 86afd1b0 0000001a [ 114.144698] 1b20: 7e069980 0000fb00 7e0660f0 00000000 7e06aa78 00000002 00000000 00000000 [ 114.152832] 1b40: df8ce62b b9533703 00000000 e5243720 000005a8 00000000 c4d5d080 001b8e30 [ 114.160967] 1b60: 0000fe88 7e06aa78 000005a8 00000000 c4909500 c11ec5b8 b23f0684 00000000 [ 114.169102] 1b80: 00000000 c4909604 00000000 0000002d fff9aed0 ffffffff 00010001 00000107 [ 114.177237] 1ba0: 000005a8 00034ac8 001b8e30 00034ac8 c2406708 000005a8 00000020 c4909500 [ 114.185372] 1bc0: c4d9d900 c4909500 00000020 c4b07874 00000020 c2619480 c4909500 c11ed500 [ 114.193507] 1be0: 00000a20 b9533703 00000020 c4909500 c4d9d900 c2406708 00000020 c11e732c [ 114.201642] 1c00: c2f9c674 e5243720 00000000 c4909500 c4d9d900 c3e1b280 00000000 c2406708 [ 114.209776] 1c20: c4b07860 c2619480 c4909500 c11f363c 00000000 00000001 c4909500 c11f5004 [ 114.217911] 1c40: 00072680 c1f2c684 c2fc3800 c2fc3840 00000001 c2406708 c236fcd8 c2406708 [ 114.226046] 1c60: c4909500 c2fc3804 00000000 00000000 00000000 c28f1cd0 00000000 e5243720 [ 114.234181] 1c80: 00000000 c2623100 c4d9d900 c240a074 c2619480 00000000 c3e1b280 c28f1d84 [ 114.242315] 1ca0: c4d9d900 c11c6e94 00000001 c11991b0 c4d9d900 c2406708 c2619480 00000001 [ 114.250450] 1cc0: c28f1d84 c11c7188 c4d9d900 c11c7288 00000001 c28f1c02 c2f7a000 00000000 [ 114.258585] 1ce0: 00000000 c2619480 c11c713c e5243720 c2619480 c4d9d840 c4d9d840 c28f1d30 [ 114.266720] 1d00: 00000000 c11c61e4 c28f1d84 c28f1d84 c28f1d84 c28f1d30 c28f1d84 c11c682c [ 114.274855] 1d20: c240a95c c2619480 00000008 c2406708 c4d9d840 c4d9dd80 c2f7a000 00000000 [ 114.282990] 1d40: 00000000 c2619480 c11c6694 e5243720 c28f1dcc c28f1dd4 c4d9dd80 c2f7a000 [ 114.291125] 1d60: c2619480 c28f1dd4 c2f7a000 c2619480 c28f1d84 c11c7458 c28f1dd4 c2406708 [ 114.299260] 1d80: c4d9dd80 c28f1d84 c28f1d84 e5243720 c2f7a608 c2f7a68c c28f1dd4 c240a95c [ 114.307395] 1da0: 00000000 c2f7a000 00000000 c2f7a000 c2f7a68c c10ff4d8 00000000 c2406708 [ 114.315530] 1dc0: 00000000 00000000 c2f7a000 c2f7a68c c240a95c c28f1dd4 c28f1dd4 e5243720 [ 114.323665] 1de0: 00000800 c2f7a68c c2f7a68c c2f7a68c 00000000 c2406708 c2700acc c28f0000 [ 114.331800] 1e00: 00000000 c10ff78c 00000000 c4b0a840 c2f7a580 c28f1e14 c28f1e14 c2f7a000 [ 114.339935] 1e20: c2f7a000 e5243720 00000800 c2f7a608 c2f7a68c c2f7a608 00000000 c2403d00 [ 114.348070] 1e40: 00000001 c28f1ecc c265ac80 c10ff9c4 c2f7a608 00000000 c2f7a608 c11007b4 [ 114.356205] 1e60: 00000004 00000040 c2f7aa18 c2f7aa38 c2403d00 c2f7a608 00000004 f088a000 [ 114.364340] 1e80: 00000040 c2403d00 0000012c c28f1ecc c265ac80 c0d51014 c2f7a608 00000001 [ 114.372475] 1ea0: 00000040 ffffb752 c2403d00 c110091c c2406708 eb3adb80 c2370b80 2903d000 [ 114.380609] 1ec0: ffffe000 c240675c 00000000 c28f1ecc c28f1ecc c28f1ed4 c28f1ed4 e5243720 [ 114.388744] 1ee0: c2403080 c240308c 00000001 00000001 00000003 ffffe000 c2654040 00000100 [ 114.396879] 1f00: c2403080 c04013f0 00000000 c1404b34 c2364390 c236fdc0 c240675c 00000009 [ 114.405014] 1f20: c236431c ffffb751 c2403d00 c1608070 04208040 c28db200 ffffe000 c28ab240 [ 114.413149] 1f40: ffffe000 00000000 00000001 c24261a4 00000002 00000000 c28ab2e4 c0450180 [ 114.421284] 1f60: c28ab240 c047022c c28ab2c0 c28ab280 00000000 c28f0000 c047011c c28ab240 [ 114.429418] 1f80: c28cbe34 c046c5c4 00000001 c28ab280 c046c478 00000000 00000000 00000000 [ 114.437552] 1fa0: 00000000 00000000 00000000 c0400278 00000000 00000000 00000000 00000000 [ 114.445687] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 114.453821] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [ 114.461968] [<c1158a44>] (__qdisc_run) from [<c10fd2c8>] (__dev_queue_xmit+0x238/0x998) [ 114.469935] [<c10fd2c8>] (__dev_queue_xmit) from [<c11ca978>] (ip_finish_output2+0x3c8/0x658) [ 114.478418] [<c11ca978>] (ip_finish_output2) from [<c11cd44c>] (ip_output+0xd8/0x15c) [ 114.486209] [<c11cd44c>] (ip_output) from [<c11cce10>] (__ip_queue_xmit+0x154/0x3dc) [ 114.493914] [<c11cce10>] (__ip_queue_xmit) from [<c11eae60>] (__tcp_transmit_skb+0x558/0xaf8) [ 114.502397] [<c11eae60>] (__tcp_transmit_skb) from [<c11ec5b8>] (tcp_write_xmit+0x230/0x1148) [ 114.510879] [<c11ec5b8>] (tcp_write_xmit) from [<c11ed500>] (__tcp_push_pending_frames+0x30/0x114) [ 114.519793] [<c11ed500>] (__tcp_push_pending_frames) from [<c11e732c>] (tcp_rcv_established+0x524/0x690) [ 114.529226] [<c11e732c>] (tcp_rcv_established) from [<c11f363c>] (tcp_v4_do_rcv+0x84/0x19c) [ 114.537537] [<c11f363c>] (tcp_v4_do_rcv) from [<c11f5004>] (tcp_v4_rcv+0xa78/0xb58) [ 114.545157] [<c11f5004>] (tcp_v4_rcv) from [<c11c6e94>] (ip_protocol_deliver_rcu+0x30/0x2d8) [ 114.553555] [<c11c6e94>] (ip_protocol_deliver_rcu) from [<c11c7188>] (ip_local_deliver_finish+0x4c/0x5c) [ 114.562989] [<c11c7188>] (ip_local_deliver_finish) from [<c11c7288>] (ip_local_deliver+0xf0/0xfc) [ 114.571817] [<c11c7288>] (ip_local_deliver) from [<c11c61e4>] (ip_sublist_rcv_finish+0x44/0x5c) [ 114.580474] [<c11c61e4>] (ip_sublist_rcv_finish) from [<c11c682c>] (ip_sublist_rcv+0x154/0x174) [ 114.589130] [<c11c682c>] (ip_sublist_rcv) from [<c11c7458>] (ip_list_rcv+0x104/0x124) [ 114.596922] [<c11c7458>] (ip_list_rcv) from [<c10ff4d8>] (__netif_receive_skb_list_core+0x150/0x23c) [ 114.606010] [<c10ff4d8>] (__netif_receive_skb_list_core) from [<c10ff78c>] (netif_receive_skb_list_internal+0x1c8/0x2cc) [ 114.616825] [<c10ff78c>] (netif_receive_skb_list_internal) from [<c10ff9c4>] (gro_normal_list.part.45+0x14/0x28) [ 114.626949] [<c10ff9c4>] (gro_normal_list.part.45) from [<c11007b4>] (napi_complete_done+0x1a0/0x1e8) [ 114.636125] [<c11007b4>] (napi_complete_done) from [<c0d51014>] (gfar_poll_rx_sq+0x4c/0xa4) [ 114.644436] [<c0d51014>] (gfar_poll_rx_sq) from [<c110091c>] (net_rx_action+0x120/0x40c) [ 114.652488] [<c110091c>] (net_rx_action) from [<c04013f0>] (__do_softirq+0x130/0x3b8) [ 114.660282] [<c04013f0>] (__do_softirq) from [<c0450180>] (run_ksoftirqd+0x2c/0x38) [ 114.667903] [<c0450180>] (run_ksoftirqd) from [<c047022c>] (smpboot_thread_fn+0x110/0x1a8) [ 114.676127] [<c047022c>] (smpboot_thread_fn) from [<c046c5c4>] (kthread+0x14c/0x150) [ 114.683831] [<c046c5c4>] (kthread) from [<c0400278>] (ret_from_fork+0x14/0x3c) [ 114.691012] Exception stack(0xc28f1fb0 to 0xc28f1ff8) [ 114.696034] 1fa0: 00000000 00000000 00000000 00000000 [ 114.704168] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 114.712301] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 114.718881] Code: e5842048 e8960006 e5863000 e5863004 (e5812004) [ 114.725028] ---[ end trace 489a617e8b1805c4 ]--- [ 114.729636] Kernel panic - not syncing: Fatal exception in interrupt [ 114.735958] CPU1: stopping [ 114.738652] CPU: 1 PID: 223 Comm: iperf3 Tainted: G D 5.10.0-rc1-00297-g2d388e050508-dirty #709 [ 114.748598] Hardware name: Freescale LS1021A [ 114.752854] [<c04120b8>] (unwind_backtrace) from [<c040c554>] (show_stack+0x10/0x14) [ 114.760561] [<c040c554>] (show_stack) from [<c13fac7c>] (dump_stack+0xc8/0xdc) [ 114.767749] [<c13fac7c>] (dump_stack) from [<c040f83c>] (do_handle_IPI+0x320/0x33c) [ 114.775367] [<c040f83c>] (do_handle_IPI) from [<c040f870>] (ipi_handler+0x18/0x20) [ 114.782902] [<c040f870>] (ipi_handler) from [<c04b0028>] (handle_percpu_devid_fasteoi_ipi+0x80/0x150) [ 114.792079] [<c04b0028>] (handle_percpu_devid_fasteoi_ipi) from [<c04a981c>] (generic_handle_irq+0x34/0x44) [ 114.801773] [<c04a981c>] (generic_handle_irq) from [<c04a9e08>] (__handle_domain_irq+0x5c/0xb4) [ 114.810430] [<c04a9e08>] (__handle_domain_irq) from [<c08f89dc>] (gic_handle_irq+0x94/0xb4) [ 114.818741] [<c08f89dc>] (gic_handle_irq) from [<c0400c38>] (__irq_svc+0x58/0x74) [ 114.826181] Exception stack(0xc4d69d78 to 0xc4d69dc0) [ 114.831202] 9d60: c4909570 00000000 [ 114.839337] 9d80: 000075bf 000075be c4d69e20 c4909500 c4909500 bef959bc 00000068 ffffe000 [ 114.847472] 9da0: bef959bc c10d9448 c4d69f08 c4d69dc8 c10da6a4 c14097c8 800e0013 ffffffff [ 114.855609] [<c0400c38>] (__irq_svc) from [<c14097c8>] (_raw_spin_lock_bh+0x44/0x58) [ 114.863314] [<c14097c8>] (_raw_spin_lock_bh) from [<c10da6a4>] (lock_sock_fast+0x10/0x5c) [ 114.871452] [<c10da6a4>] (lock_sock_fast) from [<c11d6980>] (tcp_get_info+0x8c/0x348) [ 114.879244] [<c11d6980>] (tcp_get_info) from [<c11da950>] (do_tcp_getsockopt.constprop.30+0x7ac/0x1150) [ 114.888594] [<c11da950>] (do_tcp_getsockopt.constprop.30) from [<c10d874c>] (__sys_getsockopt+0x94/0x14c) [ 114.898114] [<c10d874c>] (__sys_getsockopt) from [<c04001a0>] (ret_fast_syscall+0x0/0x4c) [ 114.906245] Exception stack(0xc4d69fa8 to 0xc4d69ff0) [ 114.911268] 9fa0: bef9595c 00023bd8 00000005 00000006 0000000b bef959bc [ 114.919403] 9fc0: bef9595c 00023bd8 bef95978 00000127 000241b8 bef95970 bef95980 00000001 [ 114.927535] 9fe0: b6f203c4 bef95948 b6f099b0 b6bc4b8a
On Tue, 3 Nov 2020 09:36:55 -0800 Jakub Kicinski wrote: > On Tue, 3 Nov 2020 19:30:07 +0200 Vladimir Oltean wrote: > > On Tue, Nov 03, 2020 at 05:18:25PM +0000, Claudiu Manoil wrote: > > > It's either the dev_kfree_skb_any from the dma mapping error path or the one > > > from skb_cow_head()'s error path. A confirmation would help indeed. > > > > It says "consume", not "kfree", which in my mind would make it point > > towards the only caller of consume_skb from the gianfar driver, i.e. the > > dev_consume_skb_any that you just added. > > #define dev_kfree_skb(a) consume_skb(a) > > IIRC we did this because too many drivers used dev_kfree_skb > incorrectly and made the dropwatch output very noisy. FWIW ./scripts/decode_stacktrace.sh should point you right to the lines of code if the build has enough debug info.
>-----Original Message----- >From: Vladimir Oltean <olteanv@gmail.com> >Sent: Tuesday, November 3, 2020 7:30 PM >To: Claudiu Manoil <claudiu.manoil@nxp.com> >Cc: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; David S . >Miller <davem@davemloft.net>; james.jurack@ametek.com >Subject: Re: [PATCH net v2 1/2] gianfar: Replace skb_realloc_headroom with >skb_cow_head for PTP > >On Tue, Nov 03, 2020 at 05:18:25PM +0000, Claudiu Manoil wrote: >> It's either the dev_kfree_skb_any from the dma mapping error path or the >one >> from skb_cow_head()'s error path. A confirmation would help indeed. > >It says "consume", not "kfree", which in my mind would make it point >towards the only caller of consume_skb from the gianfar driver, i.e. the >dev_consume_skb_any that you just added. This is the patch: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=d145c9031325fed963a887851d9fa42516efd52b are you sure you have it applied?
On Tue, Nov 03, 2020 at 05:41:36PM +0000, Claudiu Manoil wrote: > This is the patch: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=d145c9031325fed963a887851d9fa42516efd52b > > are you sure you have it applied? Actually? No, I didn't have it applied... I had thought that net had been already merged into net-next, for some reason :-/ Let me run the test for a few more tens of minutes with the patch applied.
On Tue, Nov 03, 2020 at 09:36:55AM -0800, Jakub Kicinski wrote: > IIRC we did this because too many drivers used dev_kfree_skb > incorrectly and made the dropwatch output very noisy. Nice, so that's why the drop monitor never complains with my misplaced dev_kfree_skb_any calls...
On 11/3/20 6:49 PM, Vladimir Oltean wrote: > On Tue, Nov 03, 2020 at 05:41:36PM +0000, Claudiu Manoil wrote: >> This is the patch: >> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=d145c9031325fed963a887851d9fa42516efd52b >> >> are you sure you have it applied? > > Actually? No, I didn't have it applied... I had thought that net had > been already merged into net-next, for some reason :-/ > Let me run the test for a few more tens of minutes with the patch > applied. > I find strange that the local TCP traffic can end up calling skb_realloc_headromm() in the old kernels. Normally TCP reserves a lot of bytes for headers. #define MAX_TCP_HEADER L1_CACHE_ALIGN(128 + MAX_HEADER) It should accommodate the gianfar needs for additional 24 bytes, even if LL_MAX_HEADER is 32 in your kernel build perhaps.
On Tue, Nov 03, 2020 at 07:49:06PM +0200, Vladimir Oltean wrote: > On Tue, Nov 03, 2020 at 05:41:36PM +0000, Claudiu Manoil wrote: > > This is the patch: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=d145c9031325fed963a887851d9fa42516efd52b > > > > are you sure you have it applied? > > Actually? No, I didn't have it applied... I had thought that net had > been already merged into net-next, for some reason :-/ > Let me run the test for a few more tens of minutes with the patch > applied. The test has been running for 30 minutes with the cherry-pick from net. Sorry for the noise.
>-----Original Message----- >From: Eric Dumazet <eric.dumazet@gmail.com> >Sent: Tuesday, November 3, 2020 8:16 PM >To: Vladimir Oltean <olteanv@gmail.com>; Claudiu Manoil ><claudiu.manoil@nxp.com> >Cc: Jakub Kicinski <kuba@kernel.org>; netdev@vger.kernel.org; David S . >Miller <davem@davemloft.net>; james.jurack@ametek.com >Subject: Re: [PATCH net v2 1/2] gianfar: Replace skb_realloc_headroom with >skb_cow_head for PTP > > > >On 11/3/20 6:49 PM, Vladimir Oltean wrote: >> On Tue, Nov 03, 2020 at 05:41:36PM +0000, Claudiu Manoil wrote: >>> This is the patch: >>> [...] >>> >>> are you sure you have it applied? >> >> Actually? No, I didn't have it applied... I had thought that net had >> been already merged into net-next, for some reason :-/ >> Let me run the test for a few more tens of minutes with the patch >> applied. >> > >I find strange that the local TCP traffic can end up calling >skb_realloc_headromm() in the old kernels. > >Normally TCP reserves a lot of bytes for headers. > >#define MAX_TCP_HEADER L1_CACHE_ALIGN(128 + MAX_HEADER) > >It should accommodate the gianfar needs for additional 24 bytes, >even if LL_MAX_HEADER is 32 in your kernel build perhaps. > Hi, The PTP packets need extra headroom and get reallocated, not TCP packets. However if TCP streams are sent concurrently with PTP packets, and skb_realloc_headroom() is used to reallocate PTP packets, we get these crashes. If skb_cow_head() is used instead there's no crash.
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 41dd3d0f3452..7b735fe65334 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -1829,20 +1829,12 @@ static netdev_tx_t gfar_start_xmit(struct sk_buff *skb, struct net_device *dev) fcb_len = GMAC_FCB_LEN + GMAC_TXPAL_LEN; /* make space for additional header when fcb is needed */ - if (fcb_len && unlikely(skb_headroom(skb) < fcb_len)) { - struct sk_buff *skb_new; - - skb_new = skb_realloc_headroom(skb, fcb_len); - if (!skb_new) { + if (fcb_len) { + if (unlikely(skb_cow_head(skb, fcb_len))) { dev->stats.tx_errors++; dev_kfree_skb_any(skb); return NETDEV_TX_OK; } - - if (skb->sk) - skb_set_owner_w(skb_new, skb->sk); - dev_consume_skb_any(skb); - skb = skb_new; } /* total number of fragments in the SKB */
When PTP timestamping is enabled on Tx, the controller inserts the Tx timestamp at the beginning of the frame buffer, between SFD and the L2 frame header. This means that the skb provided by the stack is required to have enough headroom otherwise a new skb needs to be created by the driver to accommodate the timestamp inserted by h/w. Up until now the driver was relying on skb_realloc_headroom() to create new skbs to accommodate PTP frames. Turns out that this method is not reliable in this context at least, as skb_realloc_headroom() for PTP frames can cause random crashes, mostly in subsequent skb_*() calls, when multiple concurrent TCP streams are run at the same time with the PTP flow on the same device (as seen in James' report). I also noticed that when the system is loaded by sending multiple TCP streams, the driver receives cloned skbs in large numbers. skb_cow_head() instead proves to be stable in this scenario, and not only handles cloned skbs too but it's also more efficient and widely used in other drivers. The commit introducing skb_realloc_headroom in the driver goes back to 2009, commit 93c1285c5d92 ("gianfar: reallocate skb when headroom is not enough for fcb"). For practical purposes I'm referencing a newer commit (from 2012) that brings the code to its current structure (and fixes the PTP case). Fixes: 9c4886e5e63b ("gianfar: Fix invalid TX frames returned on error queue when time stamping") Reported-by: James Jurack <james.jurack@ametek.com> Suggested-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com> --- v2: added this patch as the actual fix drivers/net/ethernet/freescale/gianfar.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)