Re: [PATCH net-next v8 2/8] hsr: Use skb_clone() while adding the HSR header

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-10-06 14:31:13 [+0000], netdev-bot+sashiko@kernel.org wrote:
> [Severity: Low]
> Each per-slave skb now keeps SKBTX_HW_TSTAMP, SKBTX_SW_TSTAMP,
> SKBTX_SCHED_TSTAMP and tskey in its shinfo. However, __skb_clone() leaves
> it without an owning socket:
…
> Will a slave driver with hardware timestamping enabled now take a TX
> timestamp for this clone, only for __skb_tstamp_tx() to discard it?

Yes. There is a patch later in this series where a socket is assigned.

…
> Before this patch, __pskb_copy() did not carry these flags over on the
> software path, so the slaves never saw these requests. Some NICs have only
> a single in-flight TX timestamp slot. On those, a concurrent PTP request on
> the same slave might be skipped.
> 
> The later patch "hsr: Assign a socket for cloned skbs" calls
> skb_set_owner_w() only when frame->req_tx_port != HSR_PT_NONE. Ordinary
> traffic sent over hsr0 by a socket with SOF_TIMESTAMPING_TX_HARDWARE would
> still reach this through:
> 
> hsr_dev_xmit()->hsr_forward_do()->hsr_create_tagged_frame()->
>     hsr_xmit()->dev_queue_xmit()
> 
> The NETIF_F_HW_HSR_TAG_INS path already sent sk-less clones like this
> before the patch. Should the timestamp request flags be cleared on clones
> that have no socket attached?

No. If there is no socket, it gets dropped. If there the HW is capable
of concurrent PTP request then the user shouldn't ask for them. We
didn't have PTP timestamps before.

> >  	if (!skb)
> >  		return NULL;
> > -	skb_reset_mac_header(skb);
> >  
> > -	if (skb->ip_summed == CHECKSUM_PARTIAL)
> > -		skb->csum_start += HSR_HLEN;
> > +	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
> > +		return skb;
> > +
> > +	/* Ensure the cloned skb has enough headroom to fit the HSR tag */
> > +	if (skb_cow_head(skb, HSR_HLEN))
> > +		goto err;
> >  
> >  	movelen = ETH_HLEN;
> >  	if (frame->is_vlan)
> >  		movelen += VLAN_HLEN;
> >  
> > +	/* The entire header area needs to be linear */
> > +	if (!pskb_may_pull(skb, movelen))
> > +		goto err;
> > +
> >  	src = skb_mac_header(skb);
> >  	dst = skb_push(skb, HSR_HLEN);
> >  	memmove(dst, src, movelen);
> 
> [Severity: Medium]
> Can skb_shared_info.meta_len end up describing the wrong bytes here?
> 
> skb_clone() shares the shinfo of frame->skb_std. pskb_expand_head() then
> copies both the shinfo and the head contents, so the XDP metadata length is
> kept. __pskb_copy() used a fresh shinfo, so meta_len used to be 0 on this
> path.
> 
> skb_metadata_end() returns skb_mac_header(). This code pushes HSR_HLEN,
> moves only the ETH(+VLAN) header 6 bytes earlier and resets mac_header. As
> a result, the range [mac - meta_len, mac) begins with 6 bytes that are not
> metadata, followed by only the first meta_len - 6 bytes of the original
> metadata. The memmove() also overwrites the last 6 bytes of the original
> metadata with the start of the Ethernet header.
> 
…
> Should this use skb_postpush_data_move(), or clear the metadata with
> skb_metadata_clear() after the clone?

puh. This is the only thing I found that could be improved. It looks
like the memmove() could be replace with
	skb_postpush_data_move(skb, HSR_HLEN, movelen);

should I re-spin the whole series or send a follow-up for this change?

> > +	skb_set_network_header(skb, HSR_HLEN + movelen);
> >  	skb_reset_mac_header(skb);
> > +	skb_reset_mac_len(skb);
> 
> [Severity: Low]
> The commit message only covers preserving skb_shared_info for timestamps:
> 
>     hsr_create_tagged_frame() creates a new skb where the HSR header can be
>     prepended via __pskb_copy(). This does not copy the skb_shared_info part
>     of the original skb so any timestamp request is lost.
> 
> Could the changelog also mention the header layout changes made here?
> 
> With the old code, __pskb_copy() only ran skb_copy_header(), so the
> head-relative offsets were copied unchanged into a buffer with HSR_HLEN
> more headroom. Only mac_header and csum_start were fixed up afterwards.
> That left network_header pointing at the HSR tag, transport_header
> HSR_HLEN bytes too early, and mac_len at 14 (18 with VLAN).
> 
> With this patch:
> 
> - skb_cow_head()->pskb_expand_head()->skb_headers_offset_update() moves
>   all offsets, which replaces the manual csum_start adjustment.
> - network_header now points past the HSR tag.
> - mac_len becomes 20 (24 with VLAN).
> - A new pskb_may_pull() drop path is added.
> 
> So the patch fixes the transport_header offset and changes what
> network_header and mac_len mean for these frames. Neither change is
> described in the changelog.

This sounds like a fix? I just followed the API and it is a side effect.

Sebastian