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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] hsr_create_tagged_frame() (net/hsr/hsr_forward.c) calls
  skb_cow_head(skb, HSR_HLEN) before skb_push(skb, HSR_HLEN) and…

--

hsr: Use skb_clone() while adding the HSR header

This commit changes hsr_create_tagged_frame() to use skb_clone() and
skb_cow_head() instead of __pskb_copy(). The goal is to keep the
skb_shared_info part, which holds timestamp requests. It also replaces
the open coded memmove() with skb_postpush_data_move() so that skb
metadata is preserved.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 44f6d9c83180a..6cc1812be15ad 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c

[ ... ]

> -	/* Create the new skb with enough headroom to fit the HSR tag */
> -	skb = __pskb_copy(frame->skb_std,
> -			  skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);
> +	skb = skb_clone(frame->skb_std, GFP_ATOMIC);
>  	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;
>
> -	src = skb_mac_header(skb);
> -	dst = skb_push(skb, HSR_HLEN);
> -	memmove(dst, src, movelen);
> +	/* The entire header area needs to be linear */
> +	if (!pskb_may_pull(skb, movelen))
> +		goto err;
> +
> +	skb_push(skb, HSR_HLEN);
> +	skb_postpush_data_move(skb, HSR_HLEN, movelen);

[Severity: Medium]
Should the skb_cow_head() call above also reserve room for the skb
metadata? It currently reserves only HSR_HLEN.

skb_postpush_data_move() moves the metadata HSR_HLEN bytes toward
skb->head. skb_data_move() requires the metadata to fit in the headroom
that remains after the push:

include/linux/skbuff.h:skb_data_move() {
    ...
	if (WARN_ON_ONCE(meta_end + len != skb->data ||
			 meta_len > skb_headroom(skb))) {
		skb_metadata_clear(skb);
		goto no_metadata;
	}
    ...
}

__skb_cow() only expands the head when the requested headroom is larger
than the current headroom. Consider a clone with at least HSR_HLEN bytes
of headroom but fewer than HSR_HLEN spare bytes in front of the metadata.
No expansion happens, so after the 6 byte push meta_len is larger than
skb_headroom(skb). Would that fire the WARN_ON_ONCE() and clear the
metadata?

Such an skb can come from hsr_handle_frame() on the interlink port, or
from hsr_dev_xmit() after a redirect. Both paths reset mac_header to
skb->data, so the first half of the WARN condition holds and only the
headroom check decides the outcome.

Other users of skb_postpush_data_move() include meta_len in the headroom
request. For example, __vlan_insert_inner_tag() does:

	if (skb_cow_head(skb, meta_len + VLAN_HLEN) < 0)

and __bpf_skb_change_head() does:

	ret = skb_cow(skb, meta_len + head_room);

Would something like this be needed here as well:

	skb_cow_head(skb, skb_metadata_len(skb) + HSR_HLEN)

Without it, the metadata preservation described in the commit message
("The clone also preserves metadata so skb_postpush_data_move() is used
instead of memmove()") would not hold in this case.

> +	skb_set_network_header(skb, HSR_HLEN + movelen);
>  	skb_reset_mac_header(skb);
> +	skb_reset_mac_len(skb);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014-hsr_ptp-v9-0-2d20a2a0f816%40linutronix.de