[Bug Report] fortify false-positive + real overread in skb_metadata_dst_cmp (geneve, gro_cell_poll)

[Bug Report] fortify false-positive + real overread in skb_metadata_dst_cmp (geneve, gro_cell_poll)

[Jean-Paul Sergent]

Hi,

We are hitting a fortify false-positive (followed by a fatal Oops) in
skb_metadata_dst_cmp() on Cilium geneve overlay traffic, via the
gro_cells GRO path. Full trace from the serial console:

  memcmp: detected buffer overflow: 108 byte read of buffer size 96
  WARNING: CPU: 0 PID: 15 at lib/string_helpers.c:1036 __fortify_report+0x45/0x60
  Modules linked in: usbhid virtio_net virtio_scsi virtio_console psmouse uhci_hcd virtio_pci virtio_pci_legacy_dev vmgenid ehci_pci ehci_hcd virtio_pci_modern_dev ata_piix button
  CPU: 0 UID: 0 PID: 15 Comm: ksoftirqd/0 Not tainted 6.18.54-talos #1 PREEMPT(none)
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.17.0-0-gb52ca86e094d-prebuilt.qemu.org 04/01/2014
  RIP: 0010:__fortify_report+0x45/0x60
  Call Trace:
   <TASK>
   __fortify_panic+0x9/0x10
   skb_metadata_dst_cmp+0x11b/0x120
   dev_gro_receive+0x303/0x620
   gro_receive_skb+0xc5/0x230
   gro_cell_poll+0x67/0xa0
   __napi_poll+0x2f/0x190
   net_rx_action+0x2e3/0x500
   handle_softirqs+0xe7/0x310
   run_ksoftirqd+0x26/0x50
   smpboot_thread_fn+0x167/0x250
   kthread+0x201/0x260
   ret_from_fork+0x10c/0x190
   </TASK>
  ---[ end trace 0000000000000000 ]---

The WARNING is immediately followed by a fatal Oops (kernel BUG at
lib/string_helpers.c:1043, "Oops: invalid opcode: 0000 [#1] SMP PTI"),
which takes the node down. We caught the same crash twice: once in
ksoftirqd/0 after ~11.5h of uptime, once in IRQ context (Comm:
containerd-shim, dev_gro_receive reached straight from the IP receive
path) ~14 minutes into a heavy-traffic boot, same signature both
times.

Environment: 12-node Talos v1.14.2 cluster (kernel 6.18.54-talos,
built with clang + CONFIG_FORTIFY_SOURCE) as QEMU/KVM guests
(i440FX, vmxnet3 NICs), Cilium 1.18 with geneve encapsulation and
kube-proxy replacement. Workload at crash time: sustained cross-node
overlay traffic (BitTorrent + NFS + Cilium service traffic).

skb_metadata_dst_cmp() does a single memcmp of struct ip_tunnel_info
plus options:

  memcmp(&a->u.tun_info, &b->u.tun_info,
         sizeof(a->u.tun_info) + a->u.tun_info.options_len);

Geneve carries 108 bytes of options here, but kmalloc_flex() in
metadata_dst_alloc() sets __counted_by(options_len) while options_len
is still 0 at comparison time, so the compiler's view of the tail is
96 bytes -> 108-byte read of a 96-byte view. Same disease as the
tun_dst_unclone fix 4c6d43db2a4d ("net: dst_metadata: fix
false-positive memcpy overflow in tun_dst_unclone"), on the RX/GRO
sibling path; that fix carries Fixes: 69050f8d6d07 for the same
reason.

While reading the function I believe there is also a real overread,
independent of fortify: the memcmp length uses a->options_len for
BOTH sides, so when b's metadata carries fewer options than a's, the
comparison reads past b's allocation. In the geneve/GRO case both
sides come from the same tunnel and carry equal option lengths, but
the helper is generic (any METADATA_IP_TUNNEL comparison).

A patch follows: pre-check options_len equality, compare the fixed
part, then compare options via ip_tunnel_info_opts() on both sides so
the counted_by view matches the read length - the same two-stage
shape as 4c6d43db2a4d.

NOTE: I could not build-test the fix (no kernel build environment on
the reporting host). The reproducer is the production workload itself
(sustained cross-node geneve RX under GRO): with GRO left enabled the
node panicked twice; since disabling GRO offload on the geneve device
(ethtool -K cilium_geneve gro off) as a stopgap, the same workload has
run clean - consistent with the crash living in the geneve device's
gro_cells stage.

-- 
Jean-Paul Sergent

[PATCH] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[Jean-Paul Sergent]

memcmp: detected buffer overflow: 108 byte read of buffer size 96
 WARNING: CPU: 0 PID: 15 at lib/string_helpers.c:1036 __fortify_report+0x45/0x60
  __fortify_panic+0x9/0x10
  skb_metadata_dst_cmp+0x11b/0x120
  dev_gro_receive+0x303/0x620
  gro_receive_skb+0xc5/0x230
  gro_cell_poll+0x67/0xa0

Same disease as the tun_dst_unclone fix (4c6d43db2a4d), on the RX
sibling: kmalloc_flex() in metadata_dst_alloc() sets __counted_by for
the structure to options_len, which is then initialized to zero, so
the compiler's view of the metadata_dst tail is 96 bytes at the time
of the access. Geneve carries 108 bytes of options, and the combined
struct+options memcmp trips CONFIG_FORTIFY_SOURCE when built with
clang. Observed live on 6.18.54-talos with Cilium geneve, in
gro_cell_poll (gro_cells GRO on the geneve device) under sustained
cross-node RX; the warning is followed by a fatal Oops
(kernel BUG at lib/string_helpers.c:1043).

While here, fix a related overread: the memcmp length uses
a->u.tun_info.options_len for BOTH sides, so when b carries fewer
options than a the comparison reads past b's allocation. Pre-check
that both sides carry the same options_len and compare the options
through ip_tunnel_info_opts() so the counted_by view matches the read
length (the same two-stage shape the unclone fix uses).

Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
Cc: stable@vger.kernel.org
Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>
Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/
Assisted-by: LLM
---

Reproduced without the fix: live kernel panic on 6.18.54-talos
(Cilium geneve, gro_cell_poll) under sustained cross-node RX; trace
in the report. The fix itself is NOT build-tested - no kernel build
environment on the reporter's host.
---
 include/net/dst_metadata.h | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
index f45d1e3..60878ea 100644
--- a/include/net/dst_metadata.h
+++ b/include/net/dst_metadata.h
@@ -115,10 +115,30 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
 	case METADATA_HW_PORT_MUX:
 		return memcmp(&a->u.port_info, &b->u.port_info,
 			      sizeof(a->u.port_info));
-	case METADATA_IP_TUNNEL:
-		return memcmp(&a->u.tun_info, &b->u.tun_info,
-			      sizeof(a->u.tun_info) +
-					 a->u.tun_info.options_len);
+	case METADATA_IP_TUNNEL: {
+		int ret;
+
+		/* Options lengths must match, or the options memcmp below
+		 * would read past b's allocation when b carries fewer
+		 * options than a.
+		 */
+		if (a->u.tun_info.options_len != b->u.tun_info.options_len)
+			return 1;
+		ret = memcmp(&a->u.tun_info, &b->u.tun_info,
+			     sizeof(a->u.tun_info));
+		if (ret)
+			return ret;
+		/* Compare the options through the flex-array member so the
+		 * compiler's __counted_by(options_len) view stays consistent
+		 * with the read length (same shape as the tun_dst_unclone
+		 * fix); a single memcmp of struct+options trips
+		 * CONFIG_FORTIFY_SOURCE when options_len is still 0 from
+		 * allocation time.
+		 */
+		return memcmp(ip_tunnel_info_opts(&a->u.tun_info),
+			      ip_tunnel_info_opts(&b->u.tun_info),
+			      a->u.tun_info.options_len);
+	}
 	case METADATA_MACSEC:
 		return memcmp(&a->u.macsec_info, &b->u.macsec_info,
 			      sizeof(a->u.macsec_info));
-- 
2.55.0

[PATCH net v2] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[Jean-Paul Sergent]

memcmp: detected buffer overflow: 108 byte read of buffer size 96
 WARNING: CPU: 0 PID: 15 at lib/string_helpers.c:1036 __fortify_report+0x45/0x60
  __fortify_panic+0x9/0x10
  skb_metadata_dst_cmp+0x11b/0x120
  dev_gro_receive+0x303/0x620
  gro_receive_skb+0xc5/0x230
  gro_cell_poll+0x67/0xa0

Same disease as the tun_dst_unclone fix (4c6d43db2a4d), on the RX
sibling: kmalloc_flex() in metadata_dst_alloc() sets __counted_by for
the structure to options_len, which is then initialized to zero, so
the compiler's view of the metadata_dst tail is 96 bytes at the time
of the access. Geneve carries 108 bytes of options, and the combined
struct+options memcmp trips CONFIG_FORTIFY_SOURCE when built with
clang. Observed live on 6.18.54-talos with Cilium geneve, in
gro_cell_poll (gro_cells GRO on the geneve device) under sustained
cross-node RX; the warning is followed by a fatal Oops
(kernel BUG at lib/string_helpers.c:1043).

While here, fix a related overread: the memcmp length uses
a->u.tun_info.options_len for BOTH sides, so when b carries fewer
options than a the comparison reads past b's allocation. Pre-check
that both sides carry the same options_len and compare the options
through ip_tunnel_info_opts() so the counted_by view matches the read
length (the same two-stage shape the unclone fix uses).

Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
Cc: stable@vger.kernel.org
Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>
Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/
Assisted-by: LLM

v2: fix subject prefix to [PATCH net]; no code changes.

---

Reproduced without the fix: live kernel panic on 6.18.54-talos
(Cilium geneve, gro_cell_poll) under sustained cross-node RX; trace
in the report. The fix itself is NOT build-tested - no kernel build
environment on the reporter's host.
---
 include/net/dst_metadata.h | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
index f45d1e3..60878ea 100644
--- a/include/net/dst_metadata.h
+++ b/include/net/dst_metadata.h
@@ -115,10 +115,30 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
 	case METADATA_HW_PORT_MUX:
 		return memcmp(&a->u.port_info, &b->u.port_info,
 			      sizeof(a->u.port_info));
-	case METADATA_IP_TUNNEL:
-		return memcmp(&a->u.tun_info, &b->u.tun_info,
-			      sizeof(a->u.tun_info) +
-					 a->u.tun_info.options_len);
+	case METADATA_IP_TUNNEL: {
+		int ret;
+
+		/* Options lengths must match, or the options memcmp below
+		 * would read past b's allocation when b carries fewer
+		 * options than a.
+		 */
+		if (a->u.tun_info.options_len != b->u.tun_info.options_len)
+			return 1;
+		ret = memcmp(&a->u.tun_info, &b->u.tun_info,
+			     sizeof(a->u.tun_info));
+		if (ret)
+			return ret;
+		/* Compare the options through the flex-array member so the
+		 * compiler's __counted_by(options_len) view stays consistent
+		 * with the read length (same shape as the tun_dst_unclone
+		 * fix); a single memcmp of struct+options trips
+		 * CONFIG_FORTIFY_SOURCE when options_len is still 0 from
+		 * allocation time.
+		 */
+		return memcmp(ip_tunnel_info_opts(&a->u.tun_info),
+			      ip_tunnel_info_opts(&b->u.tun_info),
+			      a->u.tun_info.options_len);
+	}
 	case METADATA_MACSEC:
 		return memcmp(&a->u.macsec_info, &b->u.macsec_info,
 			      sizeof(a->u.macsec_info));
-- 
2.55.0

Re: [PATCH net v2] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[Ilya Maximets]

On 10/3/26 3:24 AM, Jean-Paul Sergent wrote:
> memcmp: detected buffer overflow: 108 byte read of buffer size 96
>  WARNING: CPU: 0 PID: 15 at lib/string_helpers.c:1036 __fortify_report+0x45/0x60
>   __fortify_panic+0x9/0x10
>   skb_metadata_dst_cmp+0x11b/0x120
>   dev_gro_receive+0x303/0x620
>   gro_receive_skb+0xc5/0x230
>   gro_cell_poll+0x67/0xa0
> 
> Same disease as the tun_dst_unclone fix (4c6d43db2a4d), on the RX
> sibling: kmalloc_flex() in metadata_dst_alloc() sets __counted_by for
> the structure to options_len, which is then initialized to zero, so
> the compiler's view of the metadata_dst tail is 96 bytes at the time
> of the access. Geneve carries 108 bytes of options, and the combined
> struct+options memcmp trips CONFIG_FORTIFY_SOURCE when built with
> clang. Observed live on 6.18.54-talos with Cilium geneve, in
> gro_cell_poll (gro_cells GRO on the geneve device) under sustained
> cross-node RX; the warning is followed by a fatal Oops
> (kernel BUG at lib/string_helpers.c:1043).

This doesn't make sense to me.  The problem in tun_dst_unclone was
at the initialization time, where we couldn't write the options_len
together with the options while the currently stored value is zero.

Here the function just compares two blocks and they must be already
fully initialized and have options_len properly set.  If they have
options, but the length is zero, that's a bug somewhere else.

> 
> While here, fix a related overread: the memcmp length uses
> a->u.tun_info.options_len for BOTH sides, so when b carries fewer
> options than a the comparison reads past b's allocation. Pre-check
> that both sides carry the same options_len

This makes sense and may be the real bug here?  If options actually
have different length for some reason, then the memcmp will rightly
trigger the fortification check as it should.

However, someone more familiar with GRO should probably look at this
to see how the comparison should behave when options are different as
it sounds a little weird that they are.

> and compare the options
> through ip_tunnel_info_opts() so the counted_by view matches the read
> length (the same two-stage shape the unclone fix uses).

This makes no sense.  Single memcmp should work just fine as long as the
compared size doesn't exceed the actual size of both memory regions.

Do you still see the fortification issue trigger with just the length
comparison change?

> 
> Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")

This is likely wrong and should point to the commit that added the
tunnel info comparison.

> Cc: stable@vger.kernel.org
> Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>

If you are the author you need a sign-off instead of a reported-by.

Also, you're missing a lot of maintainers in the Cc list.

> Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/

There is no point linking the same thread where you're posting a patch,
it will be linked anyway on commit.

> Assisted-by: LLM
> 
> v2: fix subject prefix to [PATCH net]; no code changes.

This should not be in the commit message.  Also, there should be a link
to the previous version here.

> 
> ---
> 
> Reproduced without the fix: live kernel panic on 6.18.54-talos
> (Cilium geneve, gro_cell_poll) under sustained cross-node RX; trace
> in the report. The fix itself is NOT build-tested - no kernel build
> environment on the reporter's host.
> ---
>  include/net/dst_metadata.h | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index f45d1e3..60878ea 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -115,10 +115,30 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
>  	case METADATA_HW_PORT_MUX:
>  		return memcmp(&a->u.port_info, &b->u.port_info,
>  			      sizeof(a->u.port_info));
> -	case METADATA_IP_TUNNEL:
> -		return memcmp(&a->u.tun_info, &b->u.tun_info,
> -			      sizeof(a->u.tun_info) +
> -					 a->u.tun_info.options_len);
> +	case METADATA_IP_TUNNEL: {
> +		int ret;
> +
> +		/* Options lengths must match, or the options memcmp below
> +		 * would read past b's allocation when b carries fewer
> +		 * options than a.
> +		 */

This is obvious, drop the comment.

> +		if (a->u.tun_info.options_len != b->u.tun_info.options_len)
> +			return 1;
> +		ret = memcmp(&a->u.tun_info, &b->u.tun_info,
> +			     sizeof(a->u.tun_info));
> +		if (ret)
> +			return ret;
> +		/* Compare the options through the flex-array member so the
> +		 * compiler's __counted_by(options_len) view stays consistent
> +		 * with the read length (same shape as the tun_dst_unclone
> +		 * fix); a single memcmp of struct+options trips
> +		 * CONFIG_FORTIFY_SOURCE when options_len is still 0 from
> +		 * allocation time.
> +		 */

This part of the change doesn't make much sense, but anyway, when asking
LLMs to write comments, please ask them to be concise.  There is too much
stuff in there that makes no sense in the context of the code, e.g. the
mentioning of the "tun_dst_unclone fix", and the comment is generally way
too long for what it tries to accomplish.  It should be 2 lines at most
in this particular case.

Same applies to the commit message, there is too much fluff in there that
makes it harder to read.

So, please, do some quality control before sending patches, read what
you're sending.  Don't just shoot out AI slop.  Next person may not be
that kind in their replies.

> +		return memcmp(ip_tunnel_info_opts(&a->u.tun_info),
> +			      ip_tunnel_info_opts(&b->u.tun_info),
> +			      a->u.tun_info.options_len);
> +	}
>  	case METADATA_MACSEC:
>  		return memcmp(&a->u.macsec_info, &b->u.macsec_info,
>  			      sizeof(a->u.macsec_info));

Best regards, Ilya Maximets.

Re: [PATCH net v2] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[Jean-Paul Sergent]

On Sat, Oct 03, 2026 at 03:31:07PM +0200, Ilya Maximets wrote:
> On 10/3/26 3:24 AM, Jean-Paul Sergent wrote:
> > memcmp: detected buffer overflow: 108 byte read of buffer size 96
> > ...
> > Geneve carries 108 bytes of options, and the combined struct+options
> > memcmp trips CONFIG_FORTIFY_SOURCE when built with clang. ...
> 
> This doesn't make sense to me.  The problem in tun_dst_unclone was
> at the initialization time, where we couldn't write the options_len
> together with the options while the currently stored value is zero.
> 
> Here the function just compares two blocks and they must be already
> fully initialized and have options_len properly set.  If they have
> options, but the length is zero, that's a bug somewhere else.

Thank you for the review, and I apologize for the noise. You are
completely right on this. I re-tested the case properly with a userspace
test mimicking the FORTIFY_SOURCE check (clang __builtin_dynamic_object_size
on a __counted_by flexible array matching ip_tunnel_info geometry: 96-byte
struct, options at offset 96):

  - Equal options_len (12/12): compiler bound 108, memcmp length 108 ->
    no trip.
  - Unequal options_len (12/0): compiler bound 96, memcmp length 108 ->
    trips with the exact reported message: "108 byte read of buffer size 96".
  - Same unequal case under ASan: real heap-buffer-overflow, READ of size
    108 past b's 96-byte allocation.

So my initial "false positive at allocation time" theory was completely
wrong. The metadata is fully initialized at comparison time, the trip
happens because of unequal options_len, and the out-of-bounds read past
b's allocation is real.

> > While here, fix a related overread: the memcmp length uses
> > a->u.tun_info.options_len for BOTH sides, so when b carries fewer
> > options than a the comparison reads past b's allocation. Pre-check
> > that both sides carry the same options_len
> 
> This makes sense and may be the real bug here?  If options actually
> have different length for some reason, then the memcmp will rightly
> trigger the fortification check as it should.
> 
> However, someone more familiar with GRO should probably look at this
> to see how the comparison should behave when options are different as
> it sounds a little weird that they are.

Yes, this unequal options_len overread is the real bug.

Regarding GRO behavior: in v3 I kept the original pre-2017 semantics
(unequal options_len -> return 1, no aggregation). Aggregating skbs
with differing tunnel options would drop one packet's options, so
rejecting aggregation seems right. I have CC'd the GRO and networking
maintainers to weigh in.

We are also investigating why the two skbs have differing options_len in
our Cilium geneve setup in the first place, but regardless of the cause,
the comparison helper must not overread past b's buffer.

> > and compare the options
> > through ip_tunnel_info_opts() so the counted_by view matches the read
> > length (the same two-stage shape the unclone fix uses).
> 
> This makes no sense.  Single memcmp should work just fine as long as the
> compared size doesn't exceed the actual size of both memory regions.
> 
> Do you still see the fortification issue trigger with just the length
> comparison change?

No, the fortification issue does not trigger with just the length check.
With equal lengths, the compiler bound tracks the allocation and the
single memcmp is completely fine.

v3 drops the two-stage compare entirely and restores the options_len
check before the memcmp.

> > Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")
> 
> This is likely wrong and should point to the commit that added the
> tunnel info comparison.

Right again. Git history shows that the options_len equality check was
present in the original GRO lightweight tunnel support (commit
ce87fc6ce3f9, "gro: Make GRO aware of lightweight tunnels", 2016), but
was inadvertently dropped when commit 3fcece12bc1b ("net: store
port/representator id in metadata_dst", 2017) switched to a metadata_type
enum.

v3 uses:
Fixes: 3fcece12bc1b ("net: store port/representator id in metadata_dst")

> > Cc: stable@vger.kernel.org
> > Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>
> 
> If you are the author you need a sign-off instead of a reported-by.
> 
> Also, you're missing a lot of maintainers in the Cc list.

Fixed. Added Signed-off-by and dropped Reported-by. Maintainers from
get_maintainer.pl have been added to the Cc list.

> > Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/
> 
> There is no point linking the same thread where you're posting a patch,
> it will be linked anyway on commit.

Dropped the Closes: tag.

> > Assisted-by: LLM
> > 
> > v2: fix subject prefix to [PATCH net]; no code changes.
> 
> This should not be in the commit message.  Also, there should be a link
> to the previous version here.

Moved the changelog below the '---' line with lore links to previous
versions.

> > +		/* Options lengths must match, or the options memcmp below
> > +		 * would read past b's allocation when b carries fewer
> > +		 * options than a.
> > +		 */
> 
> This is obvious, drop the comment.
> 
> > +		/* Compare the options through the flex-array member so the
> ...
> 
> This part of the change doesn't make much sense, but anyway, when asking
> LLMs to write comments, please ask them to be concise.  There is too much
> stuff in there that makes no sense in the context of the code, e.g. the
> mentioning of the "tun_dst_unclone fix", and the comment is generally way
> too long for what it tries to accomplish.  It should be 2 lines at most
> in this particular case.
> 
> Same applies to the commit message, there is too much fluff in there that
> makes it harder to read.
> 
> So, please, do some quality control before sending patches, read what
> you're sending.  Don't just shoot out AI slop.  Next person may not be
> that kind in their replies.

Point taken, and my sincere apologies. The criticism is entirely fair.
I took an unverified explanation at face value and skipped the build
testing I should have done.

For v3:
  - Both comments and the two-stage compare are gone; the fix is a simple
    options_len equality check before the memcmp.
  - The commit message is rewritten and concise.
  - Build-tested locally against net/core/gro.o with clang 22 and
    CONFIG_FORTIFY_SOURCE=y (the affected system's kernel config).
  - In accordance with netdev rules, I will observe the 24-hour waiting
    period from v2 before posting the v3 patch series in a separate thread.

Thanks again for the thorough review.

-- 
Jean-Paul Sergent

Re: [PATCH net v2] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[Jean-Paul Sergent]

On Sat, Oct 03, 2026 at 03:31:07PM +0200, Ilya Maximets wrote:
> Here the function just compares two blocks and they must be already
> fully initialized and have options_len properly set.  If they have
> options, but the length is zero, that's a bug somewhere else.

Following up on my earlier reply: I captured the outer traffic on the
receiving node to answer where the differing options_len come from.
Neither packet is buggy or uninitialized - both lengths are legitimate
traffic on the same geneve device, and the comparison just cannot
handle the mix safely.

This cluster runs Cilium with bpf-lb-mode: dsr and
bpf-lb-dsr-dispatch: geneve. The service LB node forwards a client's
SYN inside geneve with a 12-byte DSR option appended (class 0x014b,
type 0x81, carrying the LB address and service port), while the rest
of the connection and all regular overlay traffic ride the same
tunnel with no options - this Cilium setup adds no identity option
to normal overlay packets.

60 seconds of capture (talosctl pcap, unfiltered; GRO is disabled on
the geneve device here as the standing mitigation, which does not
affect the outer side): 1,423 option-bearing packets from the LB
node, every one a TCP SYN, against 70,935 zero-option packets. 1,370
flows carried both classes on the same inner 5-tuple within that one
window - the SYN with the option, then data without it - all inbound
to one NodePort service.

How that turns into the overread: geneve RX pulls the tunnel headers
and clears the skb hash (iptunnel_pull_header() ->
skb_clear_hash_if_not_l4()), and these virtio netdevs provide no
receive hash, so every decapped packet enters the per-CPU gro cell
with skb->hash == 0 and dev_gro_receive() buckets them all into the
same gro_hash list. In gro_list_prepare() the remaining gates before
the metadata-dst comparison are the inner ethernet header compare and
the slow_gro path. DSR delivers every flow for a service to the same
backend pod, so those flows share the inner destination MAC, and
because the LB node encapsulates every client packet it forwards,
they share the inner source MAC - the LB node's - as well. With a
torrent client running on that pod, hundreds of concurrent
connections hit the same service: the SYN of one connection and
the data segment of another pass every check up to
skb_metadata_dst_cmp(), which then reads 96 + 12 bytes against a
96 + 0 allocation - the "108 byte read of buffer size 96" from the
original report, with the earlier packet on the GRO list (skb_a)
carrying the longer options, matching the reported trip direction.
GRO batches here live exactly one NAPI poll (gro_flush_timeout is
0), so a SYN and a same-service data segment merely have to be
processed by the same poll - routine at ~23 DSR SYNs/s mixed into
~70k packets/min of overlay traffic. This also explains why my
single-stream iperf attempts never reproduced it: a lone
connection's SYN and data are separated by RTT and cannot share a
poll, and it has no second same-MAC-pair flow to collide with.

The v3 patch restores the options_len equality check, so GRO
declines aggregation (clearing same_flow) instead of reading past
the shorter allocation; since the two packets belong to different
TCP flows, declining aggregation is also the correct protocol
behavior. I will post v3 once this discussion settles.

Happy to share the capture or the parsing script if that is useful.

-- 
Jean-Paul Sergent

Re: [PATCH net v2] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The commit message and the new code comment give the wrong
  root cause, while the code change itself works.
- [Medium] The Fixes: tag points at 69050f8d6d07 ("treewide: Replace
  kmalloc with kmalloc_obj for non-scalar types", Feb 2026, first in…
- [Low] The commit has no Signed-off-by:.

--

net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

This patch changes the METADATA_IP_TUNNEL case of skb_metadata_dst_cmp()
to address a FORTIFY_SOURCE memcmp() panic in the GRO path with geneve.
Dsts with different options_len values are now rejected up front.
The struct and the options are then compared with two separate memcmp()
calls.

Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/

> Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")

[Severity: Medium]

Is this the right Fixes: target? 69050f8d6d07 first appears in v7.0, but
the panic was reproduced on 6.18.54. In v6.18, metadata_dst_alloc() still
uses plain kmalloc(struct_size(md_dst, u.tun_info.options, optslen),
flags).

FORTIFY can size both objects here because of the __counted_by
annotation added by bb5e62f2d547 ("net: Add options as a flexible array
to struct ip_tunnel_info"), which is in v6.15:

	u8			options[] __aligned_largest __counted_by(options_len);

The memcmp() itself goes back to 3fcece12bc1b (2017).

With the current tag, would stable tooling skip 6.15 through 6.18,
including the 6.18 kernel where this was observed?

> Cc: stable@vger.kernel.org
> Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>
> Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/
> Assisted-by: LLM
>
> v2: fix subject prefix to [PATCH net]; no code changes.

[Severity: Low]

The commit has no Signed-off-by: line.
Documentation/process/coding-assistants.rst says the human submitter
must add their own Signed-off-by to certify the DCO. Can the patch be
applied without it?

The v2 changelog is also in the commit body, not below the --- line, so
git am would keep it in the permanent history. Could it be moved below
the ---?

> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index f45d1e3163f00..60878ea1eb50a 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -115,10 +115,30 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
>  	case METADATA_HW_PORT_MUX:
>  		return memcmp(&a->u.port_info, &b->u.port_info,
>  			      sizeof(a->u.port_info));
> -	case METADATA_IP_TUNNEL:
> -		return memcmp(&a->u.tun_info, &b->u.tun_info,
> -			      sizeof(a->u.tun_info) +
> -					 a->u.tun_info.options_len);
> +	case METADATA_IP_TUNNEL: {
> +		int ret;
> +
> +		/* Options lengths must match, or the options memcmp below
> +		 * would read past b's allocation when b carries fewer
> +		 * options than a.
> +		 */
> +		if (a->u.tun_info.options_len != b->u.tun_info.options_len)
> +			return 1;
> +		ret = memcmp(&a->u.tun_info, &b->u.tun_info,
> +			     sizeof(a->u.tun_info));
> +		if (ret)
> +			return ret;
> +		/* Compare the options through the flex-array member so the
> +		 * compiler's __counted_by(options_len) view stays consistent
> +		 * with the read length (same shape as the tun_dst_unclone
> +		 * fix); a single memcmp of struct+options trips
> +		 * CONFIG_FORTIFY_SOURCE when options_len is still 0 from
> +		 * allocation time.
> +		 */

[Severity: Medium]

Is this the right explanation for the trap? The commit message says the
same thing:

  Same disease as the tun_dst_unclone fix (4c6d43db2a4d), on the RX
  sibling: kmalloc_flex() in metadata_dst_alloc() sets __counted_by for
  the structure to options_len, which is then initialized to zero, so
  the compiler's view of the metadata_dst tail is 96 bytes at the time
  of the access. Geneve carries 108 bytes of options, ...

The old code computed the read length from the current value:
sizeof(a->u.tun_info) + a->u.tun_info.options_len. sizeof(struct
ip_tunnel_info) is 96. A 108 byte read therefore means
a->u.tun_info.options_len was 12 when the call ran. So geneve carried
12 bytes of options, not 108.

The fortified memcmp() checks the first buffer before the second:

include/linux/fortify-string.h:memcmp() {
    ...
	if (p_size < size)
		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, p_size, size, INT_MIN);
	else if (q_size < size)
		fortify_panic(FORTIFY_FUNC_memcmp, FORTIFY_READ, q_size, size, INT_MIN);
    ...
}

For a, p_size is 96 + 12 = 108, so the reported 96 has to be q_size.
That means b->u.tun_info.options_len was 0.

On RX, neither dst still holds its allocation-time zero when
gro_list_prepare() calls skb_metadata_dst_cmp(). geneve_rx() fills in
the length right after udp_tun_rx_dst():

drivers/net/geneve.c:geneve_rx() {
    ...
		ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
					gnvh->options, gnvh->opt_len * 4,
					flags);
    ...
}

If options_len really were 0 at compare time, the read length would be
96 and nothing would trap.

So isn't FORTIFY correctly catching a compare of a (12 bytes of options)
against b (no options), with a's length used for both buffers? If so,
the options_len pre-check, which the commit message calls a "While here"
fix, is the actual fix.

Once the lengths are known to match, a single struct+options memcmp()
fits both counted_by views. Is the split into two memcmp() calls needed?

options_len sits at offset 88 and options[] at offset 96. The old
memcmp() would hit the options_len difference before it reached b's
missing options. So the user-visible failure looks like the FORTIFY
panic, not a real overread past b's allocation.

Could the commit message and this comment describe the mismatched
options_len case instead of an allocation-time zero?

> +		return memcmp(ip_tunnel_info_opts(&a->u.tun_info),
> +			      ip_tunnel_info_opts(&b->u.tun_info),
> +			      a->u.tun_info.options_len);
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003005449.2675.3%40jpsergent.gmail.com

Re: [PATCH] net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · 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] The commit message and the new comment in
  skb_metadata_dst_cmp() (include/net/dst_metadata.h) give the wrong…
- [Medium] The Fixes: tag points at 69050f8d6d07 ("treewide: Replace
  kmalloc with kmalloc_obj for non-scalar types", Feb 2026), but the…
- [Medium] The commit trailers are Reported-by, Closes, Cc: stable and
  'Assisted-by: LLM', with no Signed-off-by.

--

net: dst_metadata: fix fortify trap + overread in skb_metadata_dst_cmp

This commit reworks the METADATA_IP_TUNNEL case in skb_metadata_dst_cmp().
It first checks that both sides have the same options_len. It then compares
the fixed part of struct ip_tunnel_info and the options in two separate
memcmp() calls through ip_tunnel_info_opts(). The goal is to fix a
FORTIFY_SOURCE panic seen in GRO on a geneve device.

Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/

> Fixes: 69050f8d6d07 ("treewide: Replace kmalloc with kmalloc_obj for non-scalar types")

[Severity: Medium]
Is this the right Fixes target? The commit message says the crash was seen
on 6.18.54. In v6.18, metadata_dst_alloc() still allocates with
kmalloc(struct_size(md_dst, u.tun_info.options, optslen), flags), not
kmalloc_flex().

Both pieces needed for the trap are older than 69050f8d6d07:

  - the __counted_by(options_len) annotation on options[] in struct
    ip_tunnel_info, added by bb5e62f2d547 ("net: Add options as a flexible
    array to struct ip_tunnel_info")

  - the memcmp() in skb_metadata_dst_cmp() that uses a's options_len for
    both sides, which dates back to 3fcece12bc1b

This patch is Cc: stable. With the current Fixes target, would the fix miss
6.15 through 6.19, including the 6.18 tree where the crash was seen? Would
bb5e62f2d547 be a better target?

> Cc: stable@vger.kernel.org
> Reported-by: Jean-Paul Sergent <jpsergent@gmail.com>
> Closes: https://lore.kernel.org/netdev/20261003005449.2675.1@jpsergent.gmail.com/
> Assisted-by: LLM

[Severity: Medium]
The trailer block has no Signed-off-by line.
Documentation/process/coding-assistants.rst says the human submitter is
responsible for "Adding their own Signed-off-by tag to certify the DCO".

Could a respin add a Signed-off-by so the patch can be applied?

> diff --git a/include/net/dst_metadata.h b/include/net/dst_metadata.h
> index f45d1e3163f00..60878ea1eb50a 100644
> --- a/include/net/dst_metadata.h
> +++ b/include/net/dst_metadata.h
> @@ -115,10 +115,30 @@ static inline int skb_metadata_dst_cmp(const struct sk_buff *skb_a,
>  	case METADATA_HW_PORT_MUX:
>  		return memcmp(&a->u.port_info, &b->u.port_info,
>  			      sizeof(a->u.port_info));
> -	case METADATA_IP_TUNNEL:
> -		return memcmp(&a->u.tun_info, &b->u.tun_info,
> -			      sizeof(a->u.tun_info) +
> -					 a->u.tun_info.options_len);
> +	case METADATA_IP_TUNNEL: {
> +		int ret;
> +
> +		/* Options lengths must match, or the options memcmp below
> +		 * would read past b's allocation when b carries fewer
> +		 * options than a.
> +		 */
> +		if (a->u.tun_info.options_len != b->u.tun_info.options_len)
> +			return 1;
> +		ret = memcmp(&a->u.tun_info, &b->u.tun_info,
> +			     sizeof(a->u.tun_info));
> +		if (ret)
> +			return ret;
> +		/* Compare the options through the flex-array member so the
> +		 * compiler's __counted_by(options_len) view stays consistent
> +		 * with the read length (same shape as the tun_dst_unclone
> +		 * fix); a single memcmp of struct+options trips
> +		 * CONFIG_FORTIFY_SOURCE when options_len is still 0 from
> +		 * allocation time.
> +		 */

[Severity: Medium]
Does this comment, and the commit message, describe the actual root cause?
The commit message says:

  kmalloc_flex() in metadata_dst_alloc() sets __counted_by for the
  structure to options_len, which is then initialized to zero, so the
  compiler's view of the metadata_dst tail is 96 bytes at the time of
  the access. Geneve carries 108 bytes of options

On the geneve RX path, options_len is overwritten before the skb reaches
gro_cell_poll():

geneve_rx()
  udp_tun_rx_dst() -> ... -> tun_rx_dst()
      tun_dst->u.tun_info.options_len = 0;
  ip_tunnel_info_opts_set(&tun_dst->u.tun_info,
                          gnvh->options, gnvh->opt_len * 4, flags);
      info->options_len = len;

So by the time gro_list_prepare() calls skb_metadata_dst_cmp(p, skb), both
objects hold their real options lengths.

The old code read sizeof(a->u.tun_info) + a->u.tun_info.options_len bytes,
and FORTIFY sizes a's object with the same options_len, so a's side cannot
trip. That means "108 byte read of buffer size 96" points to
a->options_len == 12 and b->options_len == 0. In other words, 12 bytes of
options, not 108.

If so, the real bug is the old memcmp() using a's options_len for both
sides. __counted_by only exposed it. Without FORTIFY, it is a slab
out-of-bounds read past b's allocation.

The options_len pre-check, which the commit message calls a "While here"
fix, is what actually stops the panic. Once the lengths are known to be
equal, a single memcmp() of sizeof(tun_info) + options_len would also pass
FORTIFY on both sides, so the two-stage split does not seem to add
anything.

The commit message also says this was seen "under sustained cross-node RX".
Can any Geneve sender trigger it remotely by mixing packets with and
without options in the same inner flow?

Could the commit message and this comment be updated to describe the
options_len mismatch between a and b as the root cause?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003005449.2675.2%40jpsergent.gmail.com

Re: [Bug Report] fortify false-positive + real overread in skb_metadata_dst_cmp (geneve, gro_cell_poll)

[Sasha Levin]

> Same disease as the
> tun_dst_unclone fix 4c6d43db2a4d ("net: dst_metadata: fix
> false-positive memcpy overflow in tun_dst_unclone"), on the RX/GRO
> sibling path; that fix carries Fixes: 69050f8d6d07 for the same
> reason.

I've queued that one for 6.18: 6.18 has the counted_by annotation it trips on.

For your skb_metadata_dst_cmp() patch, the Fixes: tag should be ce87fc6ce3f9
("gro: Make GRO aware of lightweight tunnels."), not 69050f8d6d07 ("treewide:
Replace kmalloc with kmalloc_obj for non-scalar types"). The overread predates
kmalloc_flex, and 6.18 has no kmalloc_flex at all, so with the current tag the
fix would miss 6.18, which is the tree that panics for you, and the older
trees, where the overread is silent.

-- 
Thanks,
Sasha

Re: [Bug Report] fortify false-positive + real overread in skb_metadata_dst_cmp (geneve, gro_cell_poll)

[Jean-Paul Sergent]

On Sat, Oct 03, 2026 at 06:33:09PM -0400, Sasha Levin wrote:
> I've queued that one for 6.18: 6.18 has the counted_by annotation it trips on.

Thanks for queueing the tun_dst_unclone fix for 6.18.

> For your skb_metadata_dst_cmp() patch, the Fixes: tag should be ce87fc6ce3f9
> ("gro: Make GRO aware of lightweight tunnels."), not 69050f8d6d07 ("treewide:
> Replace kmalloc with kmalloc_obj for non-scalar types"). The overread predates
> kmalloc_flex, and 6.18 has no kmalloc_flex at all, so with the current tag the
> fix would miss 6.18, which is the tree that panics for you, and the older
> trees, where the overread is silent.

Agreed that 69050f8d6d07 is wrong - Ilya pointed that out earlier today as
well (our messages crossed in flight).

Looking at the git history, the options_len equality check that prevents the
overread was actually present in ce87fc6ce3f9 itself. It was dropped later by
commit 3fcece12bc1b ("net: store port/representator id in metadata_dst") when
skb_metadata_dst_cmp() was refactored into a type switch.

So the overread was introduced in 3fcece12bc1b (2017). v3 carries:

  Fixes: 3fcece12bc1b ("net: store port/representator id in metadata_dst")

Since 3fcece12bc1b is present in 6.18 and all active stable trees, this reaches
both 6.18 and the older trees. It also means the patch applies cleanly exactly
on the trees that have the bug - in 4.5-4.12 the check is still present and the
patch would not apply there.

Unless you see any issue with using 3fcece12bc1b, I will post v3 with that tag
once the 24-hour waiting window from v2 closes.

-- 
Jean-Paul Sergent

Re: [Bug Report] fortify false-positive + real overread in skb_metadata_dst_cmp (geneve, gro_cell_poll)

[Sasha Levin]

> Unless you see any issue with using 3fcece12bc1b, I will post v3 with that tag
> once the 24-hour waiting window from v2 closes.

No objection, that is the right tag. 3fcece12bc1b ("net: store
port/representator id in metadata_dst") dropped the options_len equality
check that ce87fc6ce3f9 ("gro: Make GRO aware of lightweight tunnels.")
had.

-- 
Thanks,
Sasha