On Fri, Jul 24, 2026 at 11:27:57PM +0200, Xin Xie wrote:
> On Fri, 24 Jul 2026 14:12:30 +0100 Simon Horman wrote:
> > [Severity: High]
> > ... does modifying the MAC header directly in hsr_xmit() corrupt
> > shared packet data? [...]
> > Should the skb be unshared before making these direct modifications?
>
> I checked this against the v4 base and the series diff.
>
> The shared-data write is real. hsr_xmit() writes
> eth_hdr(skb)->h_source in place - the slave-address substitution
> for master-originated frames, and the macaddress_redbox
> substitution for the interlink port - on skbs that can be clones.
> Multiple consumers can share the same packet data while needing
> different source addresses: a PRP LAN broadcast or multicast can be
> delivered to the master (source rewritten via
> hsr_addr_subst_source()) and forwarded to the interlink (rewritten
> to macaddress_redbox) from the same underlying buffer.
>
> Two scope facts. This is pre-existing HSR RedBox code, present
> since v6.11: hsr_xmit() itself is untouched by the series (patch
> 1/4's hsr_forward.c changes sit below it and do not modify its
> body), and the series' only h_source-adjacent change moves an
> existing ether_addr_copy() a few lines earlier in
> hsr_dev_finalize(), where no skb is involved. But the series does
> make the existing interlink path reachable for PRP.
>
> I will handle this as an independent fix for the existing HSR/PRP
> code, posted separately for net with its own Fixes: tag - not
> folded into this feature series.
>
> Would you like the feature series to wait for that fix, or may v4
> proceed while it is reviewed separately?
TL;DR: Yes, I think this patchset can proceed and fixes for
pre-existing issues can be addressed separately.
Thanks for the detailed feedback above, and likewise on patch 3/4.
It is much appreciated.
My view is that in the age of AI-generated review there is a risk that
making corrections pre-existing problems a pre-requisite risks perpetual
scope increase. So I think that that unless a pre-existing bug is made
materially worse, or fundamentally prevents a patch from reaching it's
goals, then fixing the pre-existing bug can be considered as follow-up.
Of course, drawing that line is a subjective matter. But it seems to me
that the pre-existing issues flagged both here and in the review I
forwarded for 3/4 can be treated as follow-up.
> (The hsr_create_tagged_frame() side question overlaps the third
> finding in your 3/4 mail; it is answered there.)
