diff mbox series

[net,v2,1/2] gianfar: Replace skb_realloc_headroom with skb_cow_head for PTP

Message ID 20201029081057.8506-1-claudiu.manoil@nxp.com
State New
Headers show
Series [net,v2,1/2] gianfar: Replace skb_realloc_headroom with skb_cow_head for PTP | expand

Commit Message

Claudiu Manoil Oct. 29, 2020, 8:10 a.m. UTC 
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(-)

Comments

Jakub Kicinski Oct. 30, 2020, 4:41 p.m. UTC | #1 
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!
Vladimir Oltean Nov. 3, 2020, 4:13 p.m. UTC | #2 
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.
Jakub Kicinski Nov. 3, 2020, 4:30 p.m. UTC | #3 
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?
Julian Wiedmann Nov. 3, 2020, 4:36 p.m. UTC | #4 
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.
Claudiu Manoil Nov. 3, 2020, 5:08 p.m. UTC | #5 
>-----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.
Jakub Kicinski Nov. 3, 2020, 5:12 p.m. UTC | #6 
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?
Claudiu Manoil Nov. 3, 2020, 5:18 p.m. UTC | #7 
>-----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.
Vladimir Oltean Nov. 3, 2020, 5:18 p.m. UTC | #8 
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>
Vladimir Oltean Nov. 3, 2020, 5:24 p.m. UTC | #9 
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.
Vladimir Oltean Nov. 3, 2020, 5:27 p.m. UTC | #10 
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;
Vladimir Oltean Nov. 3, 2020, 5:30 p.m. UTC | #11 
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.
Jakub Kicinski Nov. 3, 2020, 5:36 p.m. UTC | #12 
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.
Vladimir Oltean Nov. 3, 2020, 5:38 p.m. UTC | #13 
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
Jakub Kicinski Nov. 3, 2020, 5:39 p.m. UTC | #14 
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.
Claudiu Manoil Nov. 3, 2020, 5:41 p.m. UTC | #15 
>-----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?
Vladimir Oltean Nov. 3, 2020, 5:49 p.m. UTC | #16 
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.
Vladimir Oltean Nov. 3, 2020, 5:51 p.m. UTC | #17 
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...
Eric Dumazet Nov. 3, 2020, 6:16 p.m. UTC | #18 
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.
Vladimir Oltean Nov. 3, 2020, 6:16 p.m. UTC | #19 
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.
Claudiu Manoil Nov. 4, 2020, 4:45 p.m. UTC | #20 
>-----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 mbox series

Patch

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 */