From: Vladimir Oltean <olteanv@gmail.com>
To: davem@davemloft.net, jakub.kicinski@netronome.com
Cc: richardcochran@gmail.com, f.fainelli@gmail.com,
vivien.didelot@gmail.com, andrew@lunn.ch, netdev@vger.kernel.org,
Vladimir Oltean <olteanv@gmail.com>
Subject: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue
Date: Tue, 17 Dec 2019 00:33:44 +0200 [thread overview]
Message-ID: <20191216223344.2261-1-olteanv@gmail.com> (raw)
On the LS1021A-TSN board, it can be seen that under rare conditions,
ptp4l gets unexpected extra data in the event socket error queue.
This is because the DSA master driver (gianfar) has TX timestamping
logic along the lines of:
1. In gfar_start_xmit:
do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
priv->hwts_tx_en;
(...)
if (unlikely(do_tstamp))
skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
2. Later in gfar_clean_tx_ring:
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS))
(...)
skb_tstamp_tx(skb, &shhwtstamps);
That is to say, between the first and second check, it drops
priv->hwts_tx_en (it assumes that it is the only one who can set
SKBTX_IN_PROGRESS, disregarding stacked net devices such as DSA switches
or PTP-capable PHY drivers). Any such driver (like sja1105 in this case)
that would set SKBTX_IN_PROGRESS would trip up this check and gianfar
would deliver a garbage TX timestamp for this skb too, which can in turn
have unpredictable and surprising effects to user space.
In fact gianfar is by far not the only driver which uses
SKBTX_IN_PROGRESS to identify skb's that need special handling. The flag
used to have a historical purpose and is now evaluated in the networking
stack in a single place: in __skb_tstamp_tx, only on the software
timestamping path (hwtstamps == NULL) which is not relevant for us.
So do the wise thing and drop the unneeded assignment. Even though this
patch alone will not protect against all classes of Ethernet driver TX
timestamping bugs, it will circumvent those related to the incorrect
interpretation of this skb tx flag.
Fixes: 47ed985e97f5 ("net: dsa: sja1105: Add logic for TX timestamping")
Suggested-by: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
drivers/net/dsa/sja1105/sja1105_ptp.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_ptp.c b/drivers/net/dsa/sja1105/sja1105_ptp.c
index 54258a25031d..038c83fbd9e8 100644
--- a/drivers/net/dsa/sja1105/sja1105_ptp.c
+++ b/drivers/net/dsa/sja1105/sja1105_ptp.c
@@ -668,8 +668,6 @@ void sja1105_ptp_txtstamp_skb(struct dsa_switch *ds, int slot,
u64 ticks, ts;
int rc;
- skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
-
mutex_lock(&ptp_data->lock);
rc = sja1105_ptpclkval_read(priv, &ticks, NULL);
--
2.17.1
next reply other threads:[~2019-12-16 22:34 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-16 22:33 Vladimir Oltean [this message]
2019-12-17 19:57 ` [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue Keller, Jacob E
2019-12-17 20:07 ` Vladimir Oltean
2019-12-17 22:21 ` Keller, Jacob E
2019-12-24 19:05 ` Richard Cochran
2019-12-26 18:24 ` Vladimir Oltean
2019-12-27 1:52 ` Richard Cochran
2019-12-27 15:19 ` Richard Cochran
2019-12-27 15:30 ` Vladimir Oltean
2019-12-27 17:39 ` Richard Cochran
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=20191216223344.2261-1-olteanv@gmail.com \
--to=olteanv@gmail.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=jakub.kicinski@netronome.com \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=vivien.didelot@gmail.com \
/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).