Re: [PATCH net-next v2 2/2] net: ipv6: ioam6: new feature tunsrc

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 13 Aug 2024 14:27:23 +0200 Justin Iurman wrote:
> This patch provides a new feature (i.e., "tunsrc") for the tunnel (i.e.,
> "encap") mode of ioam6. Just like seg6 already does, except it is
> attached to a route. The "tunsrc" is optional: when not provided (by
> default), the automatic resolution is applied. Using "tunsrc" when
> possible has a benefit: performance. See the comparison:
>  - before (= "encap" mode): https://ibb.co/bNCzvf7
>  - after (= "encap" mode with "tunsrc"): https://ibb.co/PT8L6yq

No need to extend the selftests ?

> diff --git a/include/uapi/linux/ioam6_iptunnel.h b/include/uapi/linux/ioam6_iptunnel.h
> index 38f6a8fdfd34..6cdbd0da7ad8 100644
> --- a/include/uapi/linux/ioam6_iptunnel.h
> +++ b/include/uapi/linux/ioam6_iptunnel.h
> @@ -50,6 +50,13 @@ enum {
>  	IOAM6_IPTUNNEL_FREQ_K,		/* u32 */
>  	IOAM6_IPTUNNEL_FREQ_N,		/* u32 */
>  
> +	/* Tunnel src address.
> +	 * For encap,auto modes.
> +	 * Optional (automatic if
> +	 * not provided).

The wrapping of this text appears excessive

> +	 */
> +	IOAM6_IPTUNNEL_SRC,		/* struct in6_addr */
> +
>  	__IOAM6_IPTUNNEL_MAX,
>  };

> @@ -178,6 +186,23 @@ static int ioam6_build_state(struct net *net, struct nlattr *nla,
>  	ilwt->freq.n = freq_n;
>  
>  	ilwt->mode = mode;
> +
> +	if (!tb[IOAM6_IPTUNNEL_SRC]) {
> +		ilwt->has_tunsrc = false;
> +	} else {
> +		ilwt->has_tunsrc = true;
> +		ilwt->tunsrc = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_SRC]);
> +
> +		if (ipv6_addr_any(&ilwt->tunsrc)) {
> +			dst_cache_destroy(&ilwt->cache);
> +			kfree(lwt);

Let's put the cleanup at the end of the function, and use a goto / label
to jump there.

> +			NL_SET_ERR_MSG_ATTR(extack, tb[IOAM6_IPTUNNEL_SRC],
> +					    "invalid tunnel source address");
> +			return -EINVAL;
> +		}
> +	}
> +
>  	if (tb[IOAM6_IPTUNNEL_DST])
>  		ilwt->tundst = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_DST]);
>  
> @@ -257,6 +282,8 @@ static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
>  
>  static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
>  			  struct ioam6_lwt_encap *tuninfo,
> +			  bool has_tunsrc,
> +			  struct in6_addr *tunsrc,
>  			  struct in6_addr *tundst)
>  {
>  	struct dst_entry *dst = skb_dst(skb);
> @@ -286,8 +313,13 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
>  	hdr->nexthdr = NEXTHDR_HOP;
>  	hdr->payload_len = cpu_to_be16(skb->len - sizeof(*hdr));
>  	hdr->daddr = *tundst;
> -	ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> -			   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
> +
> +	if (has_tunsrc) {
> +		memcpy(&hdr->saddr, tunsrc, sizeof(*tunsrc));
> +	} else {
> +		ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> +				   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
> +	}

single statement branches, no need for {}

>  	skb_postpush_rcsum(skb, hdr, len);
>