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] [day] [month] [year] [list]

Message-ID: <20250512072227.wseiy7kfxyxbnj2l@DEN-DL-M31836.microchip.com>
Date: Mon, 12 May 2025 09:22:27 +0200
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Vladimir Oltean <olteanv@...il.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

The 05/08/2025 17:21, Vladimir Oltean wrote:
> 
> Horatiu,

Hi Vladimir,

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

Sorry for late reply.
I think you are right!. I just want to double check by actually trying
it.

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

Thanks for the advice. I will look also into this.

-- 
/Horatiu