Message-Id: <20250616025520.92d6c60879210751664f11ad@uniroma2.it>
Date: Mon, 16 Jun 2025 02:55:20 +0200
From: Andrea Mayer <andrea.mayer@...roma2.it>
To: Ido Schimmel <idosch@...dia.com>
Cc: <netdev@...r.kernel.org>, <davem@...emloft.net>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <edumazet@...gle.com>, <dsahern@...nel.org>,
        <horms@...nel.org>, <petrm@...dia.com>,
        Andrea Mayer
 <andrea.mayer@...roma2.it>, stefano.salsano@...roma2.it,
        <paolo.lungaroni@...roma2.it>
Subject: Re: [PATCH net-next 1/4] seg6: Extend seg6_lookup_any_nexthop()
 with an oif argument

On Thu, 12 Jun 2025 15:23:20 +0300
Ido Schimmel <idosch@...dia.com> wrote:

> seg6_lookup_any_nexthop() is called by the different endpoint behaviors
> (e.g., End, End.X) to resolve an IPv6 route. Extend the function with an
> output interface argument so that it could be used to resolve a route
> with a certain output interface. This will be used by subsequent patches
> that will extend the End.X behavior with an output interface as an
> optional argument.
> 
> ip6_route_input_lookup() cannot be used when an output interface is
> specified as it ignores this parameter. Similarly, calling
> ip6_pol_route() when a table ID was not specified (e.g., End.X behavior)
> is wrong.
> 
> Therefore, when an output interface is specified without a table ID,
> resolve the route using ip6_route_output() which will take the output
> interface into account.
> 
> Note that no endpoint behavior currently passes both a table ID and an
> output interface, so the oif argument passed to ip6_pol_route() is
> always zero and there are no functional changes in this regard.
> 
> Signed-off-by: Ido Schimmel <idosch@...dia.com>
> ---
>  net/ipv6/seg6_local.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/net/ipv6/seg6_local.c b/net/ipv6/seg6_local.c
> index a11a02b4ba95..8bce7512df97 100644
> --- a/net/ipv6/seg6_local.c
> +++ b/net/ipv6/seg6_local.c
> @@ -270,7 +270,7 @@ static void advance_nextseg(struct ipv6_sr_hdr *srh, struct in6_addr *daddr)
>  
>  static int
>  seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
> -			u32 tbl_id, bool local_delivery)
> +			u32 tbl_id, bool local_delivery, int oif)
>  {
>  	

I took some time to verify that the proposed updates to
seg6_lookup_any_nexthop() work correctly with End.DT6 when using legacy mode
(as seg6_lookup_any_nexthop() is directly used by End.DT6).
Right now, there is no self-test for End.DT6 in legacy mode because the current
one covers End.DT6 in VRF mode. So, I (locally) adjusted the existing self-test
for testing End.DT6 legacy. After running the test, it looks like the legacy
End.DT6 also works fine with the suggested changes.

struct net *net = dev_net(skb->dev);
>  	struct ipv6hdr *hdr = ipv6_hdr(skb);
> @@ -282,6 +282,7 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
>  
>  	memset(&fl6, 0, sizeof(fl6));
>  	fl6.flowi6_iif = skb->dev->ifindex;
> +	fl6.flowi6_oif = oif;
>  	fl6.daddr = nhaddr ? *nhaddr : hdr->daddr;
>  	fl6.saddr = hdr->saddr;
>  	fl6.flowlabel = ip6_flowinfo(hdr);
> @@ -291,17 +292,19 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
>  	if (nhaddr)
>  		fl6.flowi6_flags = FLOWI_FLAG_KNOWN_NH;
>  
> -	if (!tbl_id) {
> +	if (!tbl_id && !oif) {
>  		dst = ip6_route_input_lookup(net, skb->dev, &fl6, skb, flags);
> -	} else {
> +	} else if (tbl_id) {
>  		struct fib6_table *table;
>  
>  		table = fib6_get_table(net, tbl_id);
>  		if (!table)
>  			goto out;
>  
> -		rt = ip6_pol_route(net, table, 0, &fl6, skb, flags);
> +		rt = ip6_pol_route(net, table, oif, &fl6, skb, flags);
>  		dst = &rt->dst;
> +	} else {
> +		dst = ip6_route_output(net, NULL, &fl6);
>  	}

I checked that regardless of which routing helper function is used -
ip6_route_output, ip6_pol_route, or ip6_route_input_lookup - the End.X behavior
must always invoke dst_input() to continue processing and forwarding the packet
to the next node, i.e., via ip6_forward(). This allows us to adjust the hop
limit, pass the packet through the netfilter forwarding hook, and then send it
out i.e., ip6_output().
The same processing applies to End.X when used without specifying an outgoing
interface (referred to as legacy End.X). The way packets are handled after the
destination is found is consistent for both legacy End.X and End.X with oif.
It's good that we've maintained consistency between the two End.X
implementations.

Overall, the patch looks good to me (greatly appreciated - if any - suggestions
and insights from routing experts).

Reviewed-by: Andrea Mayer <andrea.mayer@...roma2.it>

>  
>  	/* we want to discard traffic destined for local packet processing,
> @@ -330,7 +333,7 @@ seg6_lookup_any_nexthop(struct sk_buff *skb, struct in6_addr *nhaddr,
>  int seg6_lookup_nexthop(struct sk_buff *skb,
>  			struct in6_addr *nhaddr, u32 tbl_id)
>  {
> -	return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false);
> +	return seg6_lookup_any_nexthop(skb, nhaddr, tbl_id, false, 0);
>  }
>  
>  static __u8 seg6_flv_lcblock_octects(const struct seg6_flavors_info *finfo)
> @@ -1277,7 +1280,7 @@ static int input_action_end_dt6(struct sk_buff *skb,
>  	/* note: this time we do not need to specify the table because the VRF
>  	 * takes care of selecting the correct table.
>  	 */
> -	seg6_lookup_any_nexthop(skb, NULL, 0, true);
> +	seg6_lookup_any_nexthop(skb, NULL, 0, true, 0);
>  
>  	return dst_input(skb);
>  
> @@ -1285,7 +1288,7 @@ static int input_action_end_dt6(struct sk_buff *skb,
>  #endif
>  	skb_set_transport_header(skb, sizeof(struct ipv6hdr));
>  
> -	seg6_lookup_any_nexthop(skb, NULL, slwt->table, true);
> +	seg6_lookup_any_nexthop(skb, NULL, slwt->table, true, 0);
>  
>  	return dst_input(skb);
>  
> -- 
> 2.49.0
> 

Powered by blists - more mailing lists