Re: [PATCH net-next] net: ip6_tunnel: use tunnel parameters for fill_forward_path route lookup

From: AI Reviewer <[email protected]>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
--- Patch [1]: [PATCH net-next] net: ip6_tunnel: use tunnel parameters for fill_forward_path route lookup ---
commit 40b6b6aa3f042df26b7e3c0a15bf2f7d4b9e9df6
Author: Lorenzo Bianconi <[email protected]>

net: ip6_tunnel: use tunnel parameters for fill_forward_path route lookup

This commit updates ip6_tnl_fill_forward_path() to pass the source address,
output interface, and flowlabel from the tunnel configuration to the flowi6
struct. This change intends to align the hardware offload route lookup with
the slow path implementation in ipxip6_tnl_xmit().

> diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
> index bf8e40af60b08..557d8637ac57e 100644
> --- a/net/ipv6/ip6_tunnel.c
> +++ b/net/ipv6/ip6_tunnel.c
> @@ -1847,6 +1847,10 @@ static int ip6_tnl_fill_forward_path(struct net_device_path_ctx *ctx,
>  	struct ip6_tnl *t = netdev_priv(ctx->dev);
>  	struct flowi6 fl6 = {
>  		.daddr = t->parms.raddr,
> +		.saddr = t->parms.laddr,

[Severity: High]
This is a pre-existing issue, but does reading the 128-bit IPv6 addresses
locklessly here risk torn reads if ip6_tnl_update() modifies the parameters
concurrently?

Since these addresses are read again later in this function to populate the
path tunnel information:

ip6_tnl_fill_forward_path() {
    ...
    path->tun.src_v6 = t->parms.laddr;
    path->tun.dst_v6 = t->parms.raddr;
    ...
}

could a concurrent update cause the hardware offload metadata to point to
new endpoints while the egress route was chosen using the old endpoints?
Would it be safer to reuse the fl6.saddr and fl6.daddr values for the path
metadata?

> +		.flowi6_oif = t->parms.link,

[Severity: High]
This is a pre-existing issue, but should the route lookup also include
t->parms.fwmark?

The commit aligns the route lookup with the slow path, but omits
flowi6_mark. If a tunnel relies on fwmark-based policy routing, leaving the
mark as 0 might cause the hardware offload path to resolve to a different
egress device than the software slow path.

> +		.flowlabel = t->parms.flowinfo &
> +			     (IPV6_TCLASS_MASK | IPV6_FLOWLABEL_MASK),

[Severity: High]
Does unconditionally applying the flowinfo mask here bypass per-packet
dynamic routing constraints?

If a tunnel is configured with IP6_TNL_F_USE_ORIG_TCLASS,
IP6_TNL_F_USE_ORIG_FLOWLABEL, or IP6_TNL_F_USE_ORIG_FWMARK, the egress route
might vary dynamically based on the inner packet. Since
ip6_tnl_fill_forward_path() provides a single static path for hardware
offload, shouldn't it explicitly check for these dynamic flags and return
-EOPNOTSUPP, similar to how it handles unsupported encap limits?

>  	};
-- 
This is an AI-generated review.