Re: [PATCH 06/16 net-next v3] ipv4: udp: split IPv4 specific logic into udp_ipv4.c

Re: [PATCH 06/16 net-next v3] ipv4: udp: split IPv4 specific logic into udp_ipv4.c

[netdev-bot+sashiko]

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] When CONFIG_IPV4=n, udp4_gro_complete()
  (net/ipv4/udp_offload.c) returns -EAFNOSUPPORT for every merged IPv4…

--

ipv4: udp: split IPv4 specific logic into udp_ipv4.c

This commit moves the IPv4-specific UDP socket code out of udp.c and into
a new file, udp_ipv4.c. That covers lookup, input/output, multicast
delivery and the AF_INET udp_prot. The new file is built only when
CONFIG_IPV4 is enabled. Callers in af_inet.c, udp_bpf.c, udp_offload.c and
ipv6/udp.c get IS_ENABLED(CONFIG_IPV4) guards.

> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index cf07c3c6611a2..bea037d6b4eb7 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -958,6 +958,9 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
>  	const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
>  	struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
>
> +	if (!IS_ENABLED(CONFIG_IPV4))
> +		return -EAFNOSUPPORT;
> +

[Severity: Medium]
With CONFIG_IPV4=n, can this make every merged IPv4 UDP GRO packet get
dropped silently?

The IPv4 GRO offloads are still registered when IPv4 is disabled.
ipv4_offload_init() in af_inet.c still calls udpv4_offload_init() and
dev_add_offload(&net_hotdata.ip_packet_offload) with no guard. So
inet_gro_receive()->udp4_gro_receive()->udp_gro_receive() still runs for
IPv4 UDP packets.

The socket lookup there ends up in the new __udp4_lib_lookup() stub,
which returns NULL. Even so, udp_gro_receive() still aggregates:

	if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
		NAPI_GRO_CB(skb)->is_flist = sk ? !udp_test_bit(GRO_ENABLED, sk) : 1;

	if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
	    (sk && udp_test_bit(GRO_ENABLED, sk)) || NAPI_GRO_CB(skb)->is_flist)
		return call_gro_receive(udp_gro_receive_segment, head, skb);

At flush time, gro_complete()->inet_gro_complete()->udp4_gro_complete()
now returns -EAFNOSUPPORT, and gro_complete() in net/core/gro.c frees
the whole aggregate:

	if (err) {
		WARN_ON(&ptype->list == head);
		kfree_skb(skb);
		return;
	}

The WARN_ON doesn't fire because a ptype was found, so all segments are
lost with no warning. This would affect an IPv6-only kernel that bridges
or forwards IPv4 UDP with NETIF_F_GRO_UDP_FWD or NETIF_F_GRO_FRAGLIST
enabled on the ingress device. Inner IPv4 carried in IPv6 tunnels would
be hit too.

The fraglist branch and the udp_gro_complete() path don't need IPv4
socket state. The only IPv4-only dependency looks like the
udp4_lib_lookup_skb reference. Would it work to stub or skip only the
lookup, for example with a NULL-returning udp4_lib_lookup_skb when
CONFIG_IPV4=n, and keep the rest of udp4_gro_complete()? Or should IPv4
UDP GRO receive also be disabled in that configuration, so packets
aren't aggregated only to be dropped at completion?

The later patch in the series, "ipv4: make CONFIG_IPV4 boolean", makes
this configuration selectable. udp4_gro_complete() and
ipv4_offload_init() are unchanged at the end of the series.

>  	/* do fraglist only if there is no outer UDP encap (or we already processed it) */
>  	if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {
>  		udp_set_len(uh, skb->len - nhoff);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930135334.4739-1-fmancera%40suse.de

Re: [PATCH 06/16 net-next v3] ipv4: udp: split IPv4 specific logic into udp_ipv4.c

[Fernando Fernandez Mancera]

On 10/4/26 7:22 PM, netdev-bot+sashiko@kernel.org wrote:
> 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] When CONFIG_IPV4=n, udp4_gro_complete()
>    (net/ipv4/udp_offload.c) returns -EAFNOSUPPORT for every merged IPv4…
> 
> --
> 
> ipv4: udp: split IPv4 specific logic into udp_ipv4.c
> 
> This commit moves the IPv4-specific UDP socket code out of udp.c and into
> a new file, udp_ipv4.c. That covers lookup, input/output, multicast
> delivery and the AF_INET udp_prot. The new file is built only when
> CONFIG_IPV4 is enabled. Callers in af_inet.c, udp_bpf.c, udp_offload.c and
> ipv6/udp.c get IS_ENABLED(CONFIG_IPV4) guards.
> 
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index cf07c3c6611a2..bea037d6b4eb7 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -958,6 +958,9 @@ INDIRECT_CALLABLE_SCOPE int udp4_gro_complete(struct sk_buff *skb, int nhoff)
>>   	const struct iphdr *iph = (struct iphdr *)(skb->data + offset);
>>   	struct udphdr *uh = (struct udphdr *)(skb->data + nhoff);
>>
>> +	if (!IS_ENABLED(CONFIG_IPV4))
>> +		return -EAFNOSUPPORT;
>> +
> 
> [Severity: Medium]
> With CONFIG_IPV4=n, can this make every merged IPv4 UDP GRO packet get
> dropped silently?
> 
> The IPv4 GRO offloads are still registered when IPv4 is disabled.
> ipv4_offload_init() in af_inet.c still calls udpv4_offload_init() and
> dev_add_offload(&net_hotdata.ip_packet_offload) with no guard. So
> inet_gro_receive()->udp4_gro_receive()->udp_gro_receive() still runs for
> IPv4 UDP packets.
> 
> The socket lookup there ends up in the new __udp4_lib_lookup() stub,
> which returns NULL. Even so, udp_gro_receive() still aggregates:
> 
> 	if (skb->dev->features & NETIF_F_GRO_FRAGLIST)
> 		NAPI_GRO_CB(skb)->is_flist = sk ? !udp_test_bit(GRO_ENABLED, sk) : 1;
> 
> 	if ((!sk && (skb->dev->features & NETIF_F_GRO_UDP_FWD)) ||
> 	    (sk && udp_test_bit(GRO_ENABLED, sk)) || NAPI_GRO_CB(skb)->is_flist)
> 		return call_gro_receive(udp_gro_receive_segment, head, skb);
> 
> At flush time, gro_complete()->inet_gro_complete()->udp4_gro_complete()
> now returns -EAFNOSUPPORT, and gro_complete() in net/core/gro.c frees
> the whole aggregate:
> 
> 	if (err) {
> 		WARN_ON(&ptype->list == head);
> 		kfree_skb(skb);
> 		return;
> 	}
> 
> The WARN_ON doesn't fire because a ptype was found, so all segments are
> lost with no warning. This would affect an IPv6-only kernel that bridges
> or forwards IPv4 UDP with NETIF_F_GRO_UDP_FWD or NETIF_F_GRO_FRAGLIST
> enabled on the ingress device. Inner IPv4 carried in IPv6 tunnels would
> be hit too.
> 
> The fraglist branch and the udp_gro_complete() path don't need IPv4
> socket state. The only IPv4-only dependency looks like the
> udp4_lib_lookup_skb reference. Would it work to stub or skip only the
> lookup, for example with a NULL-returning udp4_lib_lookup_skb when
> CONFIG_IPV4=n, and keep the rest of udp4_gro_complete()? Or should IPv4
> UDP GRO receive also be disabled in that configuration, so packets
> aren't aggregated only to be dropped at completion?
> 
> The later patch in the series, "ipv4: make CONFIG_IPV4 boolean", makes
> this configuration selectable. udp4_gro_complete() and
> ipv4_offload_init() are unchanged at the end of the series.
> 

I guess this is a good point. I will look more into it.

pw-bot: cr

>>   	/* do fraglist only if there is no outer UDP encap (or we already processed it) */
>>   	if (NAPI_GRO_CB(skb)->is_flist && !NAPI_GRO_CB(skb)->encap_mark) {
>>   		udp_set_len(uh, skb->len - nhoff);
> 
> [ ... ]
>