Re: [PATCH net-next v6 1/2] udp: fix encapsulation packet resubmit in multicast deliver

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Anton Danilov wrote:
> When a UDP encapsulation socket (e.g., FOU) receives a multicast
> packet, __udp4_lib_mcast_deliver() and __udp6_lib_mcast_deliver()
> call consume_skb() when udp_queue_rcv_skb() returns a positive value.
> A positive return value from udp_queue_rcv_skb() indicates that the
> encap_rcv handler (e.g., fou_udp_recv) has consumed the UDP header
> and wants the packet to be resubmitted to the IP protocol handler
> for further processing (e.g., as a GRE packet).
> 
> The unicast paths handle this correctly by propagating the return
> value up to ip_protocol_deliver_rcu() / ip6_protocol_deliver_rcu()
> for resubmission. However, the multicast paths destroy the packet
> via consume_skb() instead of resubmitting it, causing silent packet
> loss.
> 
> This affects any UDP encapsulation (FOU, GUE) combined with multicast
> destination addresses.
> 
> Fix this by returning the value from udp_queue_rcv_skb() when it is
> positive, matching the behavior of the corresponding unicast paths.
> Note the sign difference between IPv4 and IPv6:
> 
>   - IPv4: udp_unicast_rcv_skb() returns -ret, and
>     ip_protocol_deliver_rcu() resubmits when ret < 0
>     (using -ret as the protocol number).
>   - IPv6: udp6_unicast_rcv_skb() returns ret, and
>     ip6_protocol_deliver_rcu() resubmits when ret > 0
>     (using ret as the nexthdr).
> 
> Both mcast paths now follow the same convention as their respective
> unicast paths.
> 
> Suggested-by: Kuniyuki Iwashima <kuniyu@google.com>
> Signed-off-by: Anton Danilov <littlesmilingcloud@gmail.com>
> Assisted-by: Claude:claude-opus-4-6
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> ---
>  net/ipv4/udp.c | 6 ++++--
>  net/ipv6/udp.c | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index 59248a59358c..d3ddcbfc8477 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2476,6 +2476,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	struct udp_hslot *hslot;
>  	struct sk_buff *nskb;
>  	bool use_hash2;
> +	int ret;
>  
>  	hash2_any = 0;
>  	hash2 = 0;
> @@ -2520,8 +2521,9 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
>  	}
>  
>  	if (first) {
> -		if (udp_queue_rcv_skb(first, skb) > 0)
> -			consume_skb(skb);
> +		ret = udp_queue_rcv_skb(first, skb);
> +		if (ret > 0)
> +			return -ret;

This helps the case of one encap_rcv socket in the multicast receiver
group, so is a useful fix on its own.

But is Sashiko correct that this would still leave the same issue for
other sockets in the group? If so, something to address in this series
or leave for later?

Might be worthwhile to extend the test to capture that case too.