Re: [PATCH nf-next v4 3/6] net: netfilter: Add IPv4 over IPv6 tunnel flowtable acceleration

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7867 bytes --]

commenting on shashiko's report:
https://sashiko.dev/#/patchset/20260703-b4-flowtable-sw-accel-ip6ip-v4-0-00398cd12382%40kernel.org

> Introduce sw flowtable acceleration for the TX/RX paths of
> IPv4 over IPv6 tunnels, relying on the netfilter flowtable
> infrastructure.
> The feature can be tested with a forwarding scenario between two
> NICs (eth0 and eth1), where an IPv4 over IPv6 tunnel is used to
> reach a remote site via eth1 as the underlay device:
> 
>     ETH0 -- TUN0 <==> ETH1 -- [IP network] -- TUN1 (2001:db8:2::2)
> 
> [IP configuration]
> 
> 6: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
>     link/ether 00:00:22:33:11:55 brd ff:ff:ff:ff:ff:ff
>     inet 192.168.0.2/24 scope global eth0
>        valid_lft forever preferred_lft forever
> 7: eth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
>     link/ether 00:11:22:33:11:55 brd ff:ff:ff:ff:ff:ff
>     inet6 2001:db8:2::1/64 scope global nodad
>        valid_lft forever preferred_lft forever
> 8: tun0@NONE: <POINTOPOINT,NOARP,UP,LOWER_UP> mtu 1480 qdisc noqueue state UNKNOWN group default qlen 1000
>     link/tunnel6 2001:db8:2::1 peer 2001:db8:2::2 permaddr ce9c:2940:7dcc::
>     inet 192.168.100.1/24 scope global tun0
>        valid_lft forever preferred_lft forever
> 
> $ ip route show
> default via 192.168.100.2 dev tun0
> 192.168.0.0/24 dev eth0 proto kernel scope link src 192.168.0.2
> 192.168.100.0/24 dev tun0 proto kernel scope link src 192.168.100.1
> 

[...]

>  	struct nf_flow_xmit xmit = {};
> +	struct in6_addr *ip6_daddr;
>  	struct flow_offload *flow;
>  	struct neighbour *neigh;
>  	struct rtable *rt;
> @@ -844,25 +872,41 @@ nf_flow_offload_ip_hook(void *priv, struct sk_buff *skb,
>  	other_tuple = &flow->tuplehash[!dir].tuple;
>  	ip_daddr = other_tuple->src_v4.s_addr;
>  
> -	if (nf_flow_tunnel_v4_push(state->net, skb, other_tuple, &ip_daddr) < 0)
> +	if (nf_flow_tunnel_push(state->net, skb, other_tuple, &ip_daddr,
> +				&ip6_daddr) < 0)
>  		return NF_DROP;
>  
>  	switch (tuplehash->tuple.xmit_type) {
> -	case FLOW_OFFLOAD_XMIT_NEIGH:
> -		rt = dst_rtable(tuplehash->tuple.dst_cache);
> +	case FLOW_OFFLOAD_XMIT_NEIGH: {
> +		struct dst_entry *dst;
> +
>  		xmit.outdev = dev_get_by_index_rcu(state->net, tuplehash->tuple.ifidx);
>  		if (!xmit.outdev) {
>  			flow_offload_teardown(flow);
>  			return NF_DROP;
>  		}
> -		neigh = ip_neigh_gw4(rt->dst.dev, rt_nexthop(rt, ip_daddr));
> +		if (other_tuple->tun.encap_proto == AF_INET6 ||
> +		    ctx.tun.proto == IPPROTO_IPV6) {
> +			struct rt6_info *rt6;
> +
> +			rt6 = dst_rt6_info(tuplehash->tuple.dst_cache);
> +			neigh = ip_neigh_gw6(rt6->dst.dev,
> +					     rt6_nexthop(rt6, ip6_daddr));
> +			dst = &rt6->dst;
> +		} else {
> +			rt = dst_rtable(tuplehash->tuple.dst_cache);
> +			neigh = ip_neigh_gw4(rt->dst.dev,
> +					     rt_nexthop(rt, ip_daddr));
> +			dst = &rt->dst;
> +		}

- Does this logic correctly handle cross-family tunneling like an IPv6-in-IPv4
  packet (SIT) being forwarded out via another IPv4 tunnel?
  - SIT support is added by a following patch in the series.

>  		if (IS_ERR(neigh)) {
>  			flow_offload_teardown(flow);
>  			return NF_DROP;
>  		}
>  		xmit.dest = neigh->ha;
> -		skb_dst_set_noref(skb, &rt->dst);
> +		skb_dst_set_noref(skb, dst);
>  		break;
> +	}
>  	case FLOW_OFFLOAD_XMIT_DIRECT:
>  		xmit.outdev = dev_get_by_index_rcu(state->net, tuplehash->tuple.out.ifidx);
>  		if (!xmit.outdev) {
> @@ -1112,8 +1156,12 @@ nf_flow_offload_ipv6_lookup(struct nf_flowtable_ctx *ctx,
>  	if (!nf_flow_skb_encap_protocol(ctx, skb, htons(ETH_P_IPV6)))
>  		return NULL;
>  
> -	if (nf_flow_tuple_ipv6(ctx, skb, &tuple) < 0)
> +	if (ctx->tun.proto == IPPROTO_IPIP) {
> +		if (nf_flow_tuple_ip(ctx, skb, &tuple) < 0)
> +			return NULL;
> +	} else if (nf_flow_tuple_ipv6(ctx, skb, &tuple) < 0) {
>  		return NULL;
> +	}
>  
>  	return flow_offload_lookup(flow_table, &tuple);
>  }
> @@ -1140,7 +1188,11 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
>  	if (tuplehash == NULL)
>  		return NF_ACCEPT;
>  
> -	ret = nf_flow_offload_ipv6_forward(&ctx, flow_table, tuplehash, skb);
> +	if (ctx.tun.proto == IPPROTO_IPIP)
> +		ret = nf_flow_offload_forward(&ctx, flow_table, tuplehash, skb);
> +	else
> +		ret = nf_flow_offload_ipv6_forward(&ctx, flow_table, tuplehash,
> +						   skb);
>  	if (ret < 0)
>  		return NF_DROP;
>  	else if (ret == 0)
> @@ -1164,21 +1216,38 @@ nf_flow_offload_ipv6_hook(void *priv, struct sk_buff *skb,
>  		return NF_DROP;
>  
>  	switch (tuplehash->tuple.xmit_type) {
> -	case FLOW_OFFLOAD_XMIT_NEIGH:
> -		rt = dst_rt6_info(tuplehash->tuple.dst_cache);
> +	case FLOW_OFFLOAD_XMIT_NEIGH: {
> +		struct dst_entry *dst;
> +
>  		xmit.outdev = dev_get_by_index_rcu(state->net, tuplehash->tuple.ifidx);
>  		if (!xmit.outdev) {
>  			flow_offload_teardown(flow);
>  			return NF_DROP;
>  		}
> -		neigh = ip_neigh_gw6(rt->dst.dev, rt6_nexthop(rt, ip6_daddr));
> +		if (other_tuple->tun.encap_proto == AF_INET ||
> +		    ctx.tun.proto == IPPROTO_IPIP) {
> +			__be32 ip_daddr = other_tuple->src_v4.s_addr;
> +			struct rtable *rt4;
> +
> +			skb->protocol = htons(ETH_P_IP);
> +			rt4 = dst_rtable(tuplehash->tuple.dst_cache);
> +			neigh = ip_neigh_gw4(rt4->dst.dev,
> +					     rt_nexthop(rt4, ip_daddr));
> +			dst = &rt4->dst;

- Can this branch incorrectly cast a struct rt6_info to a struct rtable
  if an IPv4-in-IPv6 packet is forwarded to another IPv6 tunnel?
  - Double tunnel encapsulation is not currently supported.

Regards,
Lorenzo

> +		} else {
> +			rt = dst_rt6_info(tuplehash->tuple.dst_cache);
> +			neigh = ip_neigh_gw6(rt->dst.dev,
> +					     rt6_nexthop(rt, ip6_daddr));
> +			dst = &rt->dst;
> +		}
>  		if (IS_ERR(neigh)) {
>  			flow_offload_teardown(flow);
>  			return NF_DROP;
>  		}
>  		xmit.dest = neigh->ha;
> -		skb_dst_set_noref(skb, &rt->dst);
> +		skb_dst_set_noref(skb, dst);
>  		break;
> +	}
>  	case FLOW_OFFLOAD_XMIT_DIRECT:
>  		xmit.outdev = dev_get_by_index_rcu(state->net, tuplehash->tuple.out.ifidx);
>  		if (!xmit.outdev) {
> diff --git a/net/netfilter/nf_flow_table_path.c b/net/netfilter/nf_flow_table_path.c
> index caaf48c5fd2a..5e84b7f18a26 100644
> --- a/net/netfilter/nf_flow_table_path.c
> +++ b/net/netfilter/nf_flow_table_path.c
> @@ -216,12 +216,13 @@ static int nft_flow_tunnel_update_route(const struct nft_pktinfo *pkt,
>  	struct dst_entry *tun_dst = NULL;
>  	struct flowi fl = {};
>  
> -	switch (nft_pf(pkt)) {
> +	switch (tun->encap_proto) {
>  	case NFPROTO_IPV4:
>  		fl.u.ip4.daddr = tun->dst_v4.s_addr;
>  		fl.u.ip4.saddr = tun->src_v4.s_addr;
>  		fl.u.ip4.flowi4_iif = nft_in(pkt)->ifindex;
> -		fl.u.ip4.flowi4_dscp = ip4h_dscp(ip_hdr(pkt->skb));
> +		if (nft_pf(pkt) == NFPROTO_IPV4)
> +			fl.u.ip4.flowi4_dscp = ip4h_dscp(ip_hdr(pkt->skb));
>  		fl.u.ip4.flowi4_mark = pkt->skb->mark;
>  		fl.u.ip4.flowi4_flags = FLOWI_FLAG_ANYSRC;
>  		break;
> @@ -229,13 +230,14 @@ static int nft_flow_tunnel_update_route(const struct nft_pktinfo *pkt,
>  		fl.u.ip6.daddr = tun->dst_v6;
>  		fl.u.ip6.saddr = tun->src_v6;
>  		fl.u.ip6.flowi6_iif = nft_in(pkt)->ifindex;
> -		fl.u.ip6.flowlabel = ip6_flowinfo(ipv6_hdr(pkt->skb));
> +		if (nft_pf(pkt) == NFPROTO_IPV6)
> +			fl.u.ip6.flowlabel = ip6_flowinfo(ipv6_hdr(pkt->skb));
>  		fl.u.ip6.flowi6_mark = pkt->skb->mark;
>  		fl.u.ip6.flowi6_flags = FLOWI_FLAG_ANYSRC;
>  		break;
>  	}
>  
> -	nf_route(nft_net(pkt), &tun_dst, &fl, false, nft_pf(pkt));
> +	nf_route(nft_net(pkt), &tun_dst, &fl, false, tun->encap_proto);
>  	if (!tun_dst)
>  		return -ENOENT;
>  
> 
> -- 
> 2.55.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]