Re: [PATCH 06/16 net-next v2] 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: 1 · Medium: 0 · 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).

- [High] The patch moves udp_rcv() from net/ipv4/udp.c into the new
  net/ipv4/udp_ipv4.c, but the moved copy is stale and undoes commit…

--

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

This patch moves the IPv4 specific UDP code (lookup, input/output, error
handling, multicast and the AF_INET udp_prot) out of net/ipv4/udp.c and
into a new net/ipv4/udp_ipv4.c. The new file is built only when
CONFIG_IPV4 is enabled, which is a step toward building INET without IPv4.

> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index b534b85bc623c..0f2ff5a9d4ae4 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c

[ ... ]

> -int udp_rcv(struct sk_buff *skb)
> -{
> -	struct rtable *rt = skb_rtable(skb);
> -	struct net *net = dev_net(skb->dev);
> -	struct sock *sk = NULL;
> -	__be32 saddr, daddr;
> -	unsigned int ulen;
> -	struct udphdr *uh;
> -	bool refcounted;
> -	int drop_reason;
> -
> -	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> -
> -	/*
> -	 *  Validate the packet.
> -	 */
> -	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> -		goto drop;		/* No space for header. */
> -
> -	uh   = udp_hdr(skb);
> -	ulen = udp_get_len(skb, uh, 0);

[ ... ]

> diff --git a/net/ipv4/udp_ipv4.c b/net/ipv4/udp_ipv4.c
> new file mode 100644
> index 0000000000000..466008c218df4
> --- /dev/null
> +++ b/net/ipv4/udp_ipv4.c

[ ... ]

> +int udp_rcv(struct sk_buff *skb)
> +{
> +	struct rtable *rt = skb_rtable(skb);
> +	struct net *net = dev_net(skb->dev);
> +	struct sock *sk = NULL;
> +	unsigned short ulen;
> +	__be32 saddr, daddr;
> +	struct udphdr *uh;
> +	bool refcounted;
> +	int drop_reason;
> +
> +	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
> +
> +	/*
> +	 *  Validate the packet.
> +	 */
> +	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
> +		goto drop;		/* No space for header. */
> +
> +	uh   = udp_hdr(skb);
> +	ulen = ntohs(uh->len);
> +	saddr = ip_hdr(skb)->saddr;
> +	daddr = ip_hdr(skb)->daddr;
> +
> +	if (ulen > skb->len)
> +		goto short_packet;
> +
> +	if (ulen < sizeof(*uh))
> +		goto short_packet;

[Severity: High]
The moved copy of udp_rcv() doesn't match the one being removed from
net/ipv4/udp.c. Was this copied from a tree older than commit
efbc1aa8ed54 ("udp: Support gro_ipv4_max_size > 65536")?

The removed version has:

	unsigned int ulen;
	...
	ulen = udp_get_len(skb, uh, 0);

The new version has:

	unsigned short ulen;
	...
	ulen = ntohs(uh->len);

Those are the two lines efbc1aa8ed54 replaced.

The GRO completion side still relies on the newer rule. Both
udp_gro_complete() and the fraglist branch of udp4_gro_complete() call
udp_set_len(), which writes a zero length for large aggregates:

include/linux/udp.h:udp_set_len() {
	uh->len = len < GRO_LEGACY_MAX_SIZE ? htons(len) : 0;
}

udp_get_len() is the helper that reads this marker back:

include/linux/udp.h:udp_get_len() {
	if (uh->len)
		return ntohs(uh->len);
	if (skb_is_gso(skb)) /* BIG TCP */
		return skb->len - dataoff;
	return 0;
}

Take gro_ipv4_max_size or gro_max_size set above 65536, with BIG TCP
running over VXLAN or GENEVE. When a 64KB or larger aggregate reaches
udp_rcv(), won't ulen be 0 here? The "ulen < sizeof(*uh)" check would
then send it to short_packet, and the packet would be dropped with
SKB_DROP_REASON_PKT_TOO_SMALL and counted in UDP_MIB_INERRORS.

__udp6_lib_rcv() in net/ipv6/udp.c still uses udp_get_len(), so IPv4
and IPv6 would now read the same header field differently.

Restoring only the udp_get_len() call would not be enough. With
ulen still declared unsigned short, lengths above 65535 would be
truncated. Could both the unsigned int declaration and the
udp_get_len() call be carried over, so this stays a pure code move as
the commit message describes?

> +
> +	if (ulen < skb->len) {
> +		if (pskb_trim_rcsum(skb, ulen))
> +			goto short_packet;
> +
> +		uh = udp_hdr(skb);
> +	}

[ ... ]

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