netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Claudiu Manoil <claudiu.manoil@nxp.com>
To: netdev@vger.kernel.org
Cc: "David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	james.jurack@ametek.com
Subject: [PATCH net v2 1/2] gianfar: Replace skb_realloc_headroom with skb_cow_head for PTP
Date: Thu, 29 Oct 2020 10:10:56 +0200	[thread overview]
Message-ID: <20201029081057.8506-1-claudiu.manoil@nxp.com> (raw)
In-Reply-To: <fa12d66e-de52-3e2e-154c-90c775bb4fe4@ametek.com>

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(-)

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


  parent reply	other threads:[~2020-10-29  8:26 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-14 13:53 PROBLEM: Crash when timestamping outgoing PTP packets under heavy network load (ppc, gianfar) James Jurack
2020-10-20 17:36 ` [PATCH net] gianfar: Account for Tx PTP timestamp in the skb headroom Claudiu Manoil
2020-10-21 17:59   ` Jakub Kicinski
2020-10-22 12:09     ` Claudiu Manoil
2020-10-23  2:54       ` Jakub Kicinski
2020-10-23 11:37         ` Claudiu Manoil
2020-10-29  8:10 ` Claudiu Manoil [this message]
2020-10-29  8:10   ` [PATCH net v2 2/2] " Claudiu Manoil
2020-10-30 16:41   ` [PATCH net v2 1/2] gianfar: Replace skb_realloc_headroom with skb_cow_head for PTP Jakub Kicinski
2020-11-03 16:13   ` Vladimir Oltean
2020-11-03 16:30     ` Jakub Kicinski
2020-11-03 17:18       ` Claudiu Manoil
2020-11-03 17:30         ` Vladimir Oltean
2020-11-03 17:36           ` Jakub Kicinski
2020-11-03 17:39             ` Jakub Kicinski
2020-11-03 17:51             ` Vladimir Oltean
2020-11-03 17:41           ` Claudiu Manoil
2020-11-03 17:49             ` Vladimir Oltean
2020-11-03 18:16               ` Eric Dumazet
2020-11-04 16:45                 ` Claudiu Manoil
2020-11-03 18:16               ` Vladimir Oltean
2020-11-03 16:36     ` Julian Wiedmann
2020-11-03 17:08       ` Claudiu Manoil
2020-11-03 17:12         ` Jakub Kicinski
2020-11-03 17:24           ` Vladimir Oltean
2020-11-03 17:38             ` Vladimir Oltean
2020-11-03 17:18       ` Vladimir Oltean
2020-11-03 17:27         ` Vladimir Oltean

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201029081057.8506-1-claudiu.manoil@nxp.com \
    --to=claudiu.manoil@nxp.com \
    --cc=davem@davemloft.net \
    --cc=james.jurack@ametek.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).