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

[Fernando Fernandez Mancera <fmancera@suse.de>]

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);
> 
> [ ... ]
>