Re: [net-next,v6,04/11] net: tunnel: allow noref dsts in udp_tunnel{,6}_dst_lookup

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: tunnel: allow noref dsts in udp_tunnel{,6}_dst_lookup

This commit updates udp_tunnel{,6}_dst_lookup to return noref dsts when
possible. All callers are updated to properly use the new noref argument
and conditionally release the dst based on whether they own a reference.

> diff --git a/drivers/net/bareudp.c b/drivers/net/bareudp.c
> index 92ee4a36f86f..1aa3d5d74a84 100644
> --- a/drivers/net/bareudp.c
> +++ b/drivers/net/bareudp.c
> @@ -315,6 +315,7 @@ static int bareudp_xmit_skb(struct sk_buff *skb, struct net_device *dev,
>  	int min_headroom;
>  	__u8 tos, ttl;
>  	__be32 saddr;
> +	bool noref;
>  	int err;

[ ... ]

> @@ -329,7 +330,8 @@ static int bareudp_xmit_skb(struct sk_buff *skb, struct net_device *dev,
>  	rt = udp_tunnel_dst_lookup(skb, dev, bareudp->net, 0, &saddr, &info->key,
>  				   sport, bareudp->port, key->tos,
>  				   use_cache ?
> -				   (struct dst_cache *)&info->dst_cache : NULL);
> +				   (struct dst_cache *)&info->dst_cache : NULL,
> +				   &noref);

[ ... ]

> @@ -364,7 +366,8 @@ static int bareudp_xmit_skb(struct sk_buff *skb, struct net_device *dev,
>  			    !net_eq(bareudp->net, dev_net(bareudp->dev)),
>  			    !test_bit(IP_TUNNEL_CSUM_BIT, info->key.tun_flags),
>  			    0);
> -	ip_rt_put(rt);
> +	if (!noref)
> +		ip_rt_put(rt);
>  	return 0;
>
>  free_dst:
>  	dst_release(&rt->dst);
            ^^^^^^^^^^^^^^^^^^^^

The success path above correctly checks if (!noref) before calling
ip_rt_put(), but the free_dst error path still unconditionally calls
dst_release(). When the dst comes from the cache (noref == true) and an
error occurs in the operations between the lookup and xmit, this would
decrement a refcount we do not own.

For comparison, bareudp6_xmit_skb() in this same patch correctly wraps
both its success and error paths with if (!noref) checks:

> @@ -434,11 +439,13 @@ static int bareudp6_xmit_skb(struct sk_buff *skb, struct net_device *dev,
>  			     !test_bit(IP_TUNNEL_CSUM_BIT,
>  				       info->key.tun_flags),
>  			     0);
> -	dst_release(dst);
> +	if (!noref)
> +		dst_release(dst);
>  	return 0;
>
>  free_dst:
> -	dst_release(dst);
> +	if (!noref)
> +		dst_release(dst);
>  	return err;

Should the free_dst label in bareudp_xmit_skb() also be wrapped with
if (!noref)?