netdev - Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA driver

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: <532276c5-0c5f-41dd-add9-487f39ec1b3a@engleder-embedded.com>
Date: Mon, 12 May 2025 22:07:52 +0200
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
 Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA
 driver

On 12.05.25 15:24, Vladimir Oltean wrote:
> This driver seems susceptible to a form of the bug explained in commit
> c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
> and in Documentation/networking/timestamping.rst section "Other caveats
> for MAC drivers", specifically it timestamps any skb which has
> SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.

Is it necessary in general to check adapter->hwtstamp_config.tx_type for
HWTSTAMP_TX_ON or only to fix this bug? In 
Documentation/networking/timestamping.rst
section "Hardware Timestamping Implementation: Device Drivers" only the
check of (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) is mentioned.

> Evaluate the proper TX timestamping condition only once on the TX
> path (in tsnep_netdev_xmit_frame()) and pass it down to
> tsnep_xmit_frame_ring() and tsnep_tx_activate() through a bool variable.

What about also storing the TX timestamping condition in TX entry and
evaluating it in tsnep_tx_poll() instead of checking
adapter->hwtstamp_config.tx_type again? This way the timestamping
decision is only done in tsnep_netdev_xmit_frame() and tsnep_tx_poll()
cannot decide differently e.g. if hardware timestamping was deactivated
in between. Also this means that SKBTX_IN_PROGRESS is only set but
never evaluated by tsnep, which should fix this bug AFAIU.

> Also evaluate it again in the TX confirmation path, in tsnep_tx_poll(),
> since I don't know whether TSNEP_DESC_EXTENDED_WRITEBACK_FLAG is a
> confounding condition and may be set for other reasons than hardware
> timestamping too.

Yes, there is also some DMA statistic included besides timestamping so
it can be set for other reasons too in the future.

I can take over this patch and test it when I understand more clearly
what needs to be done.

Gerhard