diff mbox series

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

Message ID 20240813122723.22169-3-justin.iurman@uliege.be (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: ipv6: ioam6: introduce tunsrc | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 29 this patch: 29
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 29 this patch: 29
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 29 this patch: 29
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 133 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline fail Was 0 now: 1
netdev/contest success net-next-2024-08-15--18-00 (tests: 706)

Commit Message

Justin Iurman Aug. 13, 2024, 12:27 p.m. UTC 
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

Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
 include/uapi/linux/ioam6_iptunnel.h |  7 ++++
 net/ipv6/ioam6_iptunnel.c           | 57 +++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 4 deletions(-)

Comments

Jakub Kicinski Aug. 16, 2024, 5:35 p.m. UTC | #1 
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);
>
Justin Iurman Aug. 17, 2024, 12:57 p.m. UTC | #2 
On 8/16/24 19:35, Jakub Kicinski wrote:
> 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 ?

Hi Jakub,

I've been wondering too... Currently, it doesn't check the IPv6 header 
and only focuses on the IOAM header+data. So I came to the conclusion 
that the selftests should not necessarily be updated for that. Although 
I'm not closing the door to a future update to include some extra IPv6 
header checks. I just think it's not required *now* and in this series.

I'll post -v3 to address your comments below (thanks, by the way!).

Cheers,
Justin

>> 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);
>>
diff mbox series

Patch

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).
+	 */
+	IOAM6_IPTUNNEL_SRC,		/* struct in6_addr */
+
 	__IOAM6_IPTUNNEL_MAX,
 };
 
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index cd2522f04edf..15c20e37809f 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -42,6 +42,8 @@  struct ioam6_lwt {
 	struct ioam6_lwt_freq freq;
 	atomic_t pkt_cnt;
 	u8 mode;
+	bool has_tunsrc;
+	struct in6_addr tunsrc;
 	struct in6_addr tundst;
 	struct ioam6_lwt_encap tuninfo;
 };
@@ -72,6 +74,7 @@  static const struct nla_policy ioam6_iptunnel_policy[IOAM6_IPTUNNEL_MAX + 1] = {
 	[IOAM6_IPTUNNEL_MODE]	= NLA_POLICY_RANGE(NLA_U8,
 						   IOAM6_IPTUNNEL_MODE_MIN,
 						   IOAM6_IPTUNNEL_MODE_MAX),
+	[IOAM6_IPTUNNEL_SRC]	= NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
 	[IOAM6_IPTUNNEL_DST]	= NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
 	[IOAM6_IPTUNNEL_TRACE]	= NLA_POLICY_EXACT_LEN(
 					sizeof(struct ioam6_trace_hdr)),
@@ -144,6 +147,11 @@  static int ioam6_build_state(struct net *net, struct nlattr *nla,
 	else
 		mode = nla_get_u8(tb[IOAM6_IPTUNNEL_MODE]);
 
+	if (tb[IOAM6_IPTUNNEL_SRC] && mode == IOAM6_IPTUNNEL_MODE_INLINE) {
+		NL_SET_ERR_MSG(extack, "no tunnel src expected in inline mode");
+		return -EINVAL;
+	}
+
 	if (!tb[IOAM6_IPTUNNEL_DST] && mode != IOAM6_IPTUNNEL_MODE_INLINE) {
 		NL_SET_ERR_MSG(extack, "this mode needs a tunnel destination");
 		return -EINVAL;
@@ -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);
+
+			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);
+	}
 
 	skb_postpush_rcsum(skb, hdr, len);
 
@@ -329,7 +361,9 @@  static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 	case IOAM6_IPTUNNEL_MODE_ENCAP:
 do_encap:
 		/* Encapsulation (ip6ip6) */
-		err = ioam6_do_encap(net, skb, &ilwt->tuninfo, &ilwt->tundst);
+		err = ioam6_do_encap(net, skb, &ilwt->tuninfo,
+				     ilwt->has_tunsrc, &ilwt->tunsrc,
+				     &ilwt->tundst);
 		if (unlikely(err))
 			goto drop;
 
@@ -415,6 +449,13 @@  static int ioam6_fill_encap_info(struct sk_buff *skb,
 		goto ret;
 
 	if (ilwt->mode != IOAM6_IPTUNNEL_MODE_INLINE) {
+		if (ilwt->has_tunsrc) {
+			err = nla_put_in6_addr(skb, IOAM6_IPTUNNEL_SRC,
+					       &ilwt->tunsrc);
+			if (err)
+				goto ret;
+		}
+
 		err = nla_put_in6_addr(skb, IOAM6_IPTUNNEL_DST, &ilwt->tundst);
 		if (err)
 			goto ret;
@@ -436,8 +477,12 @@  static int ioam6_encap_nlsize(struct lwtunnel_state *lwtstate)
 		  nla_total_size(sizeof(ilwt->mode)) +
 		  nla_total_size(sizeof(ilwt->tuninfo.traceh));
 
-	if (ilwt->mode != IOAM6_IPTUNNEL_MODE_INLINE)
+	if (ilwt->mode != IOAM6_IPTUNNEL_MODE_INLINE) {
+		if (ilwt->has_tunsrc)
+			nlsize += nla_total_size(sizeof(ilwt->tunsrc));
+
 		nlsize += nla_total_size(sizeof(ilwt->tundst));
+	}
 
 	return nlsize;
 }
@@ -452,8 +497,12 @@  static int ioam6_encap_cmp(struct lwtunnel_state *a, struct lwtunnel_state *b)
 	return (ilwt_a->freq.k != ilwt_b->freq.k ||
 		ilwt_a->freq.n != ilwt_b->freq.n ||
 		ilwt_a->mode != ilwt_b->mode ||
+		ilwt_a->has_tunsrc != ilwt_b->has_tunsrc ||
 		(ilwt_a->mode != IOAM6_IPTUNNEL_MODE_INLINE &&
 		 !ipv6_addr_equal(&ilwt_a->tundst, &ilwt_b->tundst)) ||
+		(ilwt_a->mode != IOAM6_IPTUNNEL_MODE_INLINE &&
+		 ilwt_a->has_tunsrc &&
+		 !ipv6_addr_equal(&ilwt_a->tunsrc, &ilwt_b->tunsrc)) ||
 		trace_a->namespace_id != trace_b->namespace_id);
 }