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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240816103558.16502b74@kernel.org>
Date: Fri, 16 Aug 2024 10:35:58 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Justin Iurman <justin.iurman@...ege.be>
Cc: netdev@...r.kernel.org, davem@...emloft.net, dsahern@...nel.org,
 edumazet@...gle.com, pabeni@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: ipv6: ioam6: new feature tunsrc

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);
>