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:

quoted

quoted

quoted

quoted

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

quoted

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