Re: [PATCH net v3 1/3] net: hsr: privatize interlink-bound skbs before address mutation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: hsr: privatize interlink-bound skbs before address mutation

This adds an skb_cow() call in hsr_xmit() for interlink egress when the
frame either came in on the master port or is locally destined without
being local-exclusive, dropping the egress if the copy fails. The stated
goal is to stop hsr_deliver_master() and hsr_xmit() from writing
different source addresses into shared data.

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 0774981a65c1..67aaf5a8622b 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -420,6 +420,22 @@ static void hsr_deliver_master(struct sk_buff *skb, struct net_device *dev,
>  static int hsr_xmit(struct sk_buff *skb, struct hsr_port *port,
>  		    struct hsr_frame_info *frame)
>  {
> +	/* An interlink-bound skb from get_untagged_frame() can still alias
> +	 * another live consumer: for master-originated frames the clone
> +	 * shares the original TX skb (which taps or the TX path may still
> +	 * hold); for ring frames the master also consumes them when they
> +	 * are destined to the local node without being exclusive to it.
> +	 * Privatize before any address mutation.
> +	 */
> +	if (port->type == HSR_PT_INTERLINK &&
> +	    (frame->port_rcv->type == HSR_PT_MASTER ||
> +	     (frame->is_local_dest && !frame->is_local_exclusive)) &&
> +	    skb_cow(skb, 0)) {
> +		frame->port_rcv->dev->stats.rx_dropped++;
> +		kfree_skb(skb);
> +		return NET_XMIT_DROP;
> +	}
> +

[Severity: Medium]
Is the enumeration in this comment complete?  The unconditional interlink
source-address rewrite is pre-existing and was not introduced by this
patch, but the new guard and the commit message claim ("Other interlink
traffic keeps its zero-copy behavior") assert that the uncovered cases are
safe, and there looks to be one that isn't.

For an untagged ring frame, handle_std_frame() keeps the received skb:

net/hsr/hsr_forward.c:handle_std_frame() {
	frame->skb_hsr = NULL;
	frame->skb_prp = NULL;
	frame->skb_std = skb;
	...
}

and hsr_get_untagged_frame() hands the interlink a plain clone of that
skb.  So the interlink egress skb shares the receive head with any clone
already taken by a ptype_all tap, since __netif_receive_skb_core() runs
the deliver_skb() loop over skb->dev->ptype_all before it dereferences
skb->dev->rx_handler, and packet_rcv() then queues its skb_share_check()
clone on sk_receive_queue where the bytes are copied to userspace only
later.

For a remote unicast frame addressed to a SAN behind the RedBox,
check_local_dest() leaves frame->is_local_dest false (pkt_type is
PACKET_OTHERHOST) and frame->port_rcv->type is SLAVE_A or SLAVE_B, so the
new predicate is false, no copy happens, and the rewrite lower down in
hsr_xmit() still touches the shared data:

	if (port->type == HSR_PT_INTERLINK)
		ether_addr_copy(eth_hdr(skb)->h_source,
				port->hsr->macaddress_redbox);

Would the tap consumer on the ring slave then read the RedBox MAC instead
of the real source MAC?  Reaching this needs an HSR RedBox whose ring
slave advertises NETIF_F_HW_HSR_TAG_RM, so that untagged ring ingress is
forwarded rather than passed up:

net/hsr/hsr_slave.c:hsr_handle_frame() {
	if (!(port->dev->features & NETIF_F_HW_HSR_TAG_RM) &&
	    port->type != HSR_PT_INTERLINK &&
	    hsr->proto_ops->invalid_dan_ingress_frame &&
	    hsr->proto_ops->invalid_dan_ingress_frame(protocol))
		goto finish_pass;
	...
}

At this patch alone the same case also mutates a real on-wire frame,
because hsr_create_tagged_frame() and prp_create_tagged_frame() return a
bare clone of frame->skb_std for NETIF_F_HW_HSR_TAG_INS devices:

	} else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
		return skb_clone(frame->skb_std, GFP_ATOMIC);
	}

That second aliasing consumer is removed later in the series by "net: hsr:
return private clones from the tagged-frame helpers", which switches those
helpers to hsr_clone_private(), so only the tap aliasing remains at the
end of the series.

Since skb_cow(skb, 0) is a no-op for a non-cloned skb (__skb_cow() only
calls pskb_expand_head() when skb_cloned() or extra headroom is needed),
could the call be keyed on the actual sharing state, or simply made
unconditional for HSR_PT_INTERLINK, instead of on the topology predicate?
-- 
pw-bot: cr