netdev - Re: [PATCH net-next v1 4/4] net: lan966x: generate software timestamp just before the doorbell

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: <20250508142157.sk7u37baqsl7yb64@skbuf>
Date: Thu, 8 May 2025 17:21:57 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Horatiu Vultur <horatiu.vultur@...rochip.com>
Cc: Jason Xing <kerneljasonxing@...il.com>, irusskikh@...vell.com,
	andrew+netdev@...n.ch, bharat@...lsio.com, ayush.sawal@...lsio.com,
	UNGLinuxDriver@...rochip.com, mcoquelin.stm32@...il.com,
	alexandre.torgue@...s.st.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	horms@...nel.org, sgoutham@...vell.com, willemb@...gle.com,
	linux-stm32@...md-mailman.stormreply.com,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v1 4/4] net: lan966x: generate software
 timestamp just before the doorbell

Horatiu,

On Thu, May 08, 2025 at 11:41:56AM +0200, Horatiu Vultur wrote:
> > > > -       skb_tx_timestamp(skb);
> > >
> > > Changing this will break the PHY timestamping because the frame gets
> > > modified in the next line, meaning that the classify function will
> > > always return PTP_CLASS_NONE.
> > 
> > Sorry that I'm not that familiar with the details. I will remove it
> > from this series, but still trying to figure out what cases could be.
> > 
> > Do you mean it can break when bpf prog is loaded because
> > 'skb_push(skb, IFH_LEN_BYTES);' expands the skb->data area?
> 
> Well, the bpf program will check if it is a PTP frame that needs to be
> timestamp when it runs ptp_classify_raw, and as we push some data in
> front of the frame, the bpf will run from that point meaning that it
> would failed to detect the PTP frames.
> 
> > May I ask
> > how the modified data of skb breaks the PHY timestamping feature?
> 
> If it fails to detect that it is a PTP frame, then the frame will not be
> passed to the PHY using the callback txtstamp. So the PHY will timestamp the
> frame but it doesn't have the frame to attach the timestamp.

While I was further discussing this in private with Jason, a thought
popped up in my head.

Shouldn't skb_tx_timestamp(skb); be done _before_ this section?

	/* skb processing */
	needed_headroom = max_t(int, IFH_LEN_BYTES - skb_headroom(skb), 0);
	needed_tailroom = max_t(int, ETH_FCS_LEN - skb_tailroom(skb), 0);
	if (needed_headroom || needed_tailroom || skb_header_cloned(skb)) {
		err = pskb_expand_head(skb, needed_headroom, needed_tailroom,
				       GFP_ATOMIC);
		if (unlikely(err)) {
			dev->stats.tx_dropped++;
			err = NETDEV_TX_OK;
			goto release;
		}
	}

The idea is that skb_tx_timestamp() calls skb_clone_tx_timestamp(), and
that should require skb_unshare() before you make any further
modification like insert an IFH here, so that the IFH is not visible to
clones (and thus to user space, on the socket error queue).

I think pskb_expand_head() serves the role of skb_unshare(), because I
see skb_header_cloned() is one of the conditions on which it is called.

But the problem is that skb_header_cloned() may have been false, then
skb_tx_timestamp() makes skb_header_cloned() true, but pskb_expand_head()
has already run. So the IFH is shared with the clone made for TX
timestamping purposes, I guess.

Am I completely off?

Also, I believe you can set dev->needed_headroom = IFH_LEN_BYTES,
dev->needed_tailroom = ETH_FCS_LEN, and then just call
skb_ensure_writable_head_tail().