Re: [PATCH net 7/8] net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

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

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

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] bcmgenet_add_tsb() now unconditionally writes 64 bytes into
  the skb head (memset(status, 0, sizeof(*status)) at…
- [Low] The commit carries no Signed-off-by: line and includes
  tool/Gerrit-specific trailers.

--

net: bcmgenet: zero-initialize Transmit Status Block in bcmgenet_add_tsb

This adds a memset() of the 64-byte Transmit Status Block after the
skb_push() in bcmgenet_add_tsb(), so that fields other than
tx_csum_info, and the whole block for packets without CHECKSUM_PARTIAL,
are no longer left holding whatever was in the skb headroom before.

Two observations below, one on the trailer block and one on the new write.

    Fixes: 9a9ba2a4aaaa ("net: bcmgenet: always enable status blocks")
    Assisted-by: LLM
    Co-authored-by: Cursor [off-list ref]
    Change-Id: I6935e24408ea10f68eb61dce4cced9b0ead5c005

[Severity: Low]
This isn't a code bug, but the trailer block has no Signed-off-by: line,
and it still carries a Gerrit Change-Id:.

Documentation/process/submitting-patches.rst requires the Signed-off-by
(Developer's Certificate of Origin), and Change-Id is normally stripped
before posting, so as-is checkpatch/b4 and the netdev CI will reject this.

Can the Signed-off-by be added and the Change-Id dropped, here and in the
rest of the series?

[Severity: Medium]
Can this memset() scribble over headroom that is shared with a clone?

The only unsharing step in bcmgenet_add_tsb() is keyed off available
headroom, not off ownership of the head:

	if (unlikely(skb_headroom(skb) < sizeof(*status))) {
		new_skb = skb_realloc_headroom(skb, sizeof(*status));
		...
	}

	skb_push(skb, sizeof(*status));
	status = (struct status_64 *)skb->data;
	memset(status, 0, sizeof(*status));

A cloned skb that already has 64 bytes of headroom skips the realloc
entirely, and that is the common case because bcmgenet_probe() does:

	dev->needed_headroom += 64;

bcmgenet_xmit() calls bcmgenet_add_tsb() without any copy-on-write of its
own, so the memset() lands in memory shared with the other clone.

Clones of one head do get transmitted on two devices concurrently, for
example bond_xmit_broadcast():

	skb2 = skb_clone(skb, GFP_ATOMIC);

and tcf_mirred_to_dev():

	skb_to_send = skb_clone(skb, GFP_ATOMIC);

If the second device's transmit path pushes its own header (VLAN tag, DSA
tag, tunnel or USB-ethernet header) into the same headroom bytes, do the
two writes overlap and corrupt one of the frames?

Before this patch the driver wrote 4 bytes (tx_csum_info) and only for
CHECKSUM_PARTIAL packets; the memset() extends that to all 64 bytes for
every packet, which widens the overlap considerably.

Would skb_cow_head(skb, sizeof(*status)) before the skb_push(), with the
allocation failure handled like the existing skb_realloc_headroom() path,
be preferable here? That is what the sibling driver does in
bcmasp_csum_offload():

	ret = skb_cow_head(skb, sizeof(*offload));
	if (ret < 0) {
		intf->mib.tx_realloc_offload_failed++;
		goto help;
	}

The missing copy-on-write predates this patch and the same pattern exists
in bcm_sysport_insert_tsb(), so it is not newly introduced, but the larger
write makes it more visible.

 
 	if (skb->ip_summed  == CHECKSUM_PARTIAL) {
 		ip_ver = skb->protocol;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918000732.276506-1-florian.fainelli%40broadcom.com