netdev - RE: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to socket error queue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <02874ECE860811409154E81DA85FBB58B26DEDC3@fmsmsx101.amr.corp.intel.com>
Date:   Tue, 17 Dec 2019 19:57:38 +0000
From:   "Keller, Jacob E" <jacob.e.keller@...el.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>
CC:     "richardcochran@...il.com" <richardcochran@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net] net: dsa: sja1105: Fix double delivery of TX
 timestamps to socket error queue

> -----Original Message-----
> From: netdev-owner@...r.kernel.org <netdev-owner@...r.kernel.org> On
> Behalf Of Vladimir Oltean
> Sent: Monday, December 16, 2019 2:34 PM
> To: davem@...emloft.net; jakub.kicinski@...ronome.com
> Cc: richardcochran@...il.com; f.fainelli@...il.com; vivien.didelot@...il.com;
> andrew@...n.ch; netdev@...r.kernel.org; Vladimir Oltean
> <olteanv@...il.com>
> Subject: [PATCH net] net: dsa: sja1105: Fix double delivery of TX timestamps to
> socket error queue
> 
> 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);

I'm not sure I fully understand the problem.

> 
> 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.
>

I thought the point of SKBTX_IN_PROGRESS was to inform the stack that a timestamp was pending. By not setting it, you no longer do this.

Maybe that has changed since the original implementation? Or am I misunderstanding this patch..?

You're removing the sja1105 assignment, not the one from the gianfar. Hmm

Ok, so the issue is that sja1105_ptp.c was incorrectly setting the flag.

Would it make more sense for gianfar to set SKBTX_IN_PROGRESS, but then use some other indicator internally, so that other callers who set it don't cause the gianfar driver to behave incorrectly? I believe we handle it in the Intel drivers that way by storing the skb. Then we don't check the SKBTX_IN_PROGRESS later.

Thanks,
Jake 
 
> Fixes: 47ed985e97f5 ("net: dsa: sja1105: Add logic for TX timestamping")
> Suggested-by: Richard Cochran <richardcochran@...il.com>
> Signed-off-by: Vladimir Oltean <olteanv@...il.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