netdev - RE: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX queues with tc-etf enabled

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>] [day] [month] [year] [list]
Message-ID: <DBBPR04MB7818BF360948C482D9088B4292919@DBBPR04MB7818.eurprd04.prod.outlook.com>
Date:   Wed, 10 Mar 2021 06:21:05 +0000
From:   Po Liu <po.liu@....com>
To:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
CC:     Alexandru Marginean <alexandru.marginean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Michael Walle <michael@...le.cc>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>
Subject: RE:  [PATCH net 2/2] net: enetc: allow hardware timestamping on TX
 queues with tc-etf enabled

Ok to me.

> -----Original Message-----
> From: Vladimir Oltean <olteanv@...il.com>
> Sent: 2021年3月7日 21:24
> To: David S . Miller <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>;
> netdev@...r.kernel.org; Po Liu <po.liu@....com>
> Cc: Alexandru Marginean <alexandru.marginean@....com>; Claudiu Manoil
> <claudiu.manoil@....com>; Michael Walle <michael@...le.cc>; Vladimir
> Oltean <vladimir.oltean@....com>; Vinicius Costa Gomes
> <vinicius.gomes@...el.com>
> Subject: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX
> queues with tc-etf enabled
> 
> Caution: EXT Email
> 
> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> The txtime is passed to the driver in skb->skb_mstamp_ns, which is actually in a
> union with skb->tstamp (the place where software timestamps are kept).
> 
> Since commit b50a5c70ffa4 ("net: allow simultaneous SW and HW transmit
> timestamping"), __sock_recv_timestamp has some logic for making sure that
> the two calls to skb_tstamp_tx:
> 
> skb_tx_timestamp(skb) # Software timestamp in the driver
> -> skb_tstamp_tx(skb, NULL)
> 
> and
> 
> skb_tstamp_tx(skb, &shhwtstamps) # Hardware timestamp in the driver
> 
> will both do the right thing and in a race-free manner, meaning that
> skb_tx_timestamp will deliver a cmsg with the software timestamp only, and
> skb_tstamp_tx with a non-NULL hwtstamps argument will deliver a cmsg with
> the hardware timestamp only.
> 
> Why are races even possible? Well, because although the software timestamp
> skb->tstamp is private per skb, the hardware timestamp
> skb->skb_hwtstamps(skb)
> lives in skb_shinfo(skb), an area which is shared between skbs and their clones.
> And skb_tstamp_tx works by cloning the packets when timestamping them,
> therefore attempting to perform hardware timestamping on an skb's clone will
> also change the hardware timestamp of the original skb. And the original skb
> might have been yet again cloned for software timestamping, at an earlier stage.
> 
> So the logic in __sock_recv_timestamp can't be as simple as saying "does this
> skb have a hardware timestamp? if yes I'll send the hardware timestamp to the
> socket, otherwise I'll send the software timestamp", precisely because the
> hardware timestamp is shared.
> Instead, it's quite the other way around: __sock_recv_timestamp says "does this
> skb have a software timestamp? if yes, I'll send the software timestamp,
> otherwise the hardware one". This works because the software timestamp is not
> shared with clones.
> 
> But that means we have a problem when we attempt hardware timestamping
> with skbs that don't have the skb->tstamp == 0. __sock_recv_timestamp will say
> "oh, yeah, this must be some sort of odd clone" and will not deliver the
> hardware timestamp to the socket. And this is exactly what is happening when
> we have txtime enabled on the socket: as mentioned, that is put in a union with
> skb->tstamp, so it is quite easy to mistake it.
> 
> Do what other drivers do (intel igb/igc) and write zero to skb->tstamp before
> taking the hardware timestamp. It's of no use to us now (we're already on the
> TX confirmation path).
> 
> Fixes: 0d08c9ec7d6e ("enetc: add support time specific departure base on the
> qos etf")
> Cc: Vinicius Costa Gomes <vinicius.gomes@...el.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 30d7d4e83900..09471329f3a3 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -344,6 +344,12 @@ static void enetc_tstamp_tx(struct sk_buff *skb, u64
> tstamp)
>         if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
>                 memset(&shhwtstamps, 0, sizeof(shhwtstamps));
>                 shhwtstamps.hwtstamp = ns_to_ktime(tstamp);
> +               /* Ensure skb_mstamp_ns, which might have been populated with
> +                * the txtime, is not mistaken for a software timestamp,
> +                * because this will prevent the dispatch of our hardware
> +                * timestamp to the socket.
> +                */
> +               skb->tstamp = ktime_set(0, 0);
>                 skb_tstamp_tx(skb, &shhwtstamps);
>         }
>  }
> --
> 2.25.1

Reviewed-by: Po Liu <po.liu@....com>

Br,
Po Liu