[PATCH net 0/2] net: ioam6: fix bugs in ioam6_iptunnel

[PATCH net 0/2] net: ioam6: fix bugs in ioam6_iptunnel

[Justin Iurman]

When running some measurements on IOAM, we discovered a "bug" that
triggers two reallocations instead of one in some specific cases. Those
specific cases are:
- "inline" mode with pre-allocated data size 236 or 240 bytes
- "encap" mode with pre-allocated data size 196 or 200 bytes

The reason is that we (unluckily) fall on a boundary and, since we use
skb->mac_len by default in skb_cow_head(), the second call to
skb_cow_head() after the insertion may need more than available in the
headroom (because, there, we call LL_RESERVED_SPACE()). Example on a
machine that reallocates by steps of 64 bytes:
- I need to add 256 bytes (+14 skb->mac_len), so 270 bytes
- current headroom is 206 bytes
- call to skb_cow_head, the headroom is now 270 bytes (+64 bytes)
- after the insertion, we want to make sure that the dev has enough
  headroom (LL_RESERVED_SPACE() gives 16 bytes in our case: 14+2)
- current headroom is 14 bytes (yep, remember... see above)
- call to skb_cow_head... oh wait, I need 16 bytes and I only have 14
  available... let's reallocate! The headroom is now 78 bytes (+64)

And so every single time. Patch 2 solves this issue by providing a
mitigation.

Also, while fixing the above, we discovered another bug: after the
insertion, the second call to skb_cow_head() makes sure that the dev has
enough headroom for layer 2 and stuff. In that case, the "old" dst entry
is used, which is not correct. Patch 1 solves this issue.

Justin Iurman (2):
  net: ioam6: use "new" dst entry with skb_cow_head
  net: ioam6: mitigate the two reallocations problem

 net/ipv6/ioam6_iptunnel.c | 85 ++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 41 deletions(-)

-- 
2.34.1

[PATCH net 1/2] net: ioam6: use "new" dst entry with skb_cow_head

[Justin Iurman]

In ioam6_output(), we call skb_cow_head() with LL_RESERVED_SPACE(). The
latter uses "dst", which is potentially not the good one anymore. As a
consequence, there might not be enough headroom, depending on the "new"
dev (e.g., needed_headroom > 0). Therefore, we first need to get the
"new" dst entry (from the cache or by calling ip6_route_output()), just
like seg6 and rpl both do, and only then call skb_cow_head() with
LL_RESERVED_SPACE() on the "new" dst entry.

Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
 net/ipv6/ioam6_iptunnel.c | 65 +++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 33 deletions(-)

diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index bf7120ecea1e..b08c13550144 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -295,7 +295,7 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
 
 static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 {
-	struct dst_entry *dst = skb_dst(skb);
+	struct dst_entry *dst, *orig_dst = skb_dst(skb);
 	struct in6_addr orig_daddr;
 	struct ioam6_lwt *ilwt;
 	int err = -EINVAL;
@@ -304,7 +304,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 	if (skb->protocol != htons(ETH_P_IPV6))
 		goto drop;
 
-	ilwt = ioam6_lwt_state(dst->lwtstate);
+	ilwt = ioam6_lwt_state(orig_dst->lwtstate);
 
 	/* Check for insertion frequency (i.e., "k over n" insertions) */
 	pkt_cnt = atomic_fetch_inc(&ilwt->pkt_cnt);
@@ -346,45 +346,44 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		goto drop;
 	}
 
-	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
-	if (unlikely(err))
-		goto drop;
+	local_bh_disable();
+	dst = dst_cache_get(&ilwt->cache);
+	local_bh_enable();
+
+	if (unlikely(!dst)) {
+		struct ipv6hdr *hdr = ipv6_hdr(skb);
+		struct flowi6 fl6;
+
+		memset(&fl6, 0, sizeof(fl6));
+		fl6.daddr = hdr->daddr;
+		fl6.saddr = hdr->saddr;
+		fl6.flowlabel = ip6_flowinfo(hdr);
+		fl6.flowi6_mark = skb->mark;
+		fl6.flowi6_proto = hdr->nexthdr;
+
+		dst = ip6_route_output(net, NULL, &fl6);
+		if (dst->error) {
+			err = dst->error;
+			dst_release(dst);
+			goto drop;
+		}
 
-	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr)) {
 		local_bh_disable();
-		dst = dst_cache_get(&ilwt->cache);
+		dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
 		local_bh_enable();
+	}
 
-		if (unlikely(!dst)) {
-			struct ipv6hdr *hdr = ipv6_hdr(skb);
-			struct flowi6 fl6;
-
-			memset(&fl6, 0, sizeof(fl6));
-			fl6.daddr = hdr->daddr;
-			fl6.saddr = hdr->saddr;
-			fl6.flowlabel = ip6_flowinfo(hdr);
-			fl6.flowi6_mark = skb->mark;
-			fl6.flowi6_proto = hdr->nexthdr;
-
-			dst = ip6_route_output(net, NULL, &fl6);
-			if (dst->error) {
-				err = dst->error;
-				dst_release(dst);
-				goto drop;
-			}
-
-			local_bh_disable();
-			dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
-			local_bh_enable();
-		}
+	skb_dst_drop(skb);
+	skb_dst_set(skb, dst);
 
-		skb_dst_drop(skb);
-		skb_dst_set(skb, dst);
+	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+	if (unlikely(err))
+		goto drop;
 
+	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr))
 		return dst_output(net, sk, skb);
-	}
 out:
-	return dst->lwtstate->orig_output(net, sk, skb);
+	return orig_dst->lwtstate->orig_output(net, sk, skb);
 drop:
 	kfree_skb(skb);
 	return err;
-- 
2.34.1

[PATCH net 2/2] net: ioam6: mitigate the two reallocations problem

[Justin Iurman]

Get the cache _before_ adding bytes. This way, we provide the dst entry
to skb_cow_head(), so that we call LL_RESERVED_SPACE() on it and avoid
two reallocations in some specific cases. We cannot do much when the dst
entry is empty (cache is empty, this is the first time): in that case,
we use skb->mac_len by default and two reallocations will happen in
those specific cases. However, it will only happen once, not every
single time.

Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
---
 net/ipv6/ioam6_iptunnel.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index b08c13550144..e5a7e7472b71 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -220,14 +220,16 @@ static int ioam6_do_fill(struct net *net, struct sk_buff *skb)
 }
 
 static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
-			   struct ioam6_lwt_encap *tuninfo)
+			   struct ioam6_lwt_encap *tuninfo,
+			   struct dst_entry *dst)
 {
 	struct ipv6hdr *oldhdr, *hdr;
 	int hdrlen, err;
 
 	hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
 
-	err = skb_cow_head(skb, hdrlen + skb->mac_len);
+	err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
+					       : LL_RESERVED_SPACE(dst->dev)));
 	if (unlikely(err))
 		return err;
 
@@ -256,16 +258,17 @@ 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,
-			  struct in6_addr *tundst)
+			  struct in6_addr *tundst,
+			  struct dst_entry *dst)
 {
-	struct dst_entry *dst = skb_dst(skb);
 	struct ipv6hdr *hdr, *inner_hdr;
 	int hdrlen, len, err;
 
 	hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
 	len = sizeof(*hdr) + hdrlen;
 
-	err = skb_cow_head(skb, len + skb->mac_len);
+	err = skb_cow_head(skb, len + (!dst ? skb->mac_len
+					    : LL_RESERVED_SPACE(dst->dev)));
 	if (unlikely(err))
 		return err;
 
@@ -285,7 +288,7 @@ 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_dev_get_saddr(net, skb_dst(skb)->dev, &hdr->daddr,
 			   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
 
 	skb_postpush_rcsum(skb, hdr, len);
@@ -313,6 +316,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 	orig_daddr = ipv6_hdr(skb)->daddr;
 
+	local_bh_disable();
+	dst = dst_cache_get(&ilwt->cache);
+	local_bh_enable();
+
 	switch (ilwt->mode) {
 	case IOAM6_IPTUNNEL_MODE_INLINE:
 do_inline:
@@ -320,7 +327,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		if (ipv6_hdr(skb)->nexthdr == NEXTHDR_HOP)
 			goto out;
 
-		err = ioam6_do_inline(net, skb, &ilwt->tuninfo);
+		err = ioam6_do_inline(net, skb, &ilwt->tuninfo, dst);
 		if (unlikely(err))
 			goto drop;
 
@@ -328,7 +335,8 @@ 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->tundst, dst);
 		if (unlikely(err))
 			goto drop;
 
@@ -346,10 +354,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		goto drop;
 	}
 
-	local_bh_disable();
-	dst = dst_cache_get(&ilwt->cache);
-	local_bh_enable();
-
 	if (unlikely(!dst)) {
 		struct ipv6hdr *hdr = ipv6_hdr(skb);
 		struct flowi6 fl6;
@@ -371,15 +375,15 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		local_bh_disable();
 		dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
 		local_bh_enable();
+
+		err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+		if (unlikely(err))
+			goto drop;
 	}
 
 	skb_dst_drop(skb);
 	skb_dst_set(skb, dst);
 
-	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
-	if (unlikely(err))
-		goto drop;
-
 	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr))
 		return dst_output(net, sk, skb);
 out:
-- 
2.34.1

Re: [PATCH net 2/2] net: ioam6: mitigate the two reallocations problem

[Justin Iurman]

On 7/2/24 19:44, Justin Iurman wrote:
> Get the cache _before_ adding bytes. This way, we provide the dst entry
> to skb_cow_head(), so that we call LL_RESERVED_SPACE() on it and avoid
> two reallocations in some specific cases. We cannot do much when the dst
> entry is empty (cache is empty, this is the first time): in that case,
> we use skb->mac_len by default and two reallocations will happen in
> those specific cases. However, it will only happen once, not every
> single time.

This fix could also be applied to seg6 and rpl. Not sure if the problem 
would show up though (I did some quick computations, seems unlikely), 
but still... would probably be interesting to have it there too, just in 
case. Any opinion?

> Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
> Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
> ---
>   net/ipv6/ioam6_iptunnel.c | 36 ++++++++++++++++++++----------------
>   1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index b08c13550144..e5a7e7472b71 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c
> @@ -220,14 +220,16 @@ static int ioam6_do_fill(struct net *net, struct sk_buff *skb)
>   }
>   
>   static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
> -			   struct ioam6_lwt_encap *tuninfo)
> +			   struct ioam6_lwt_encap *tuninfo,
> +			   struct dst_entry *dst)
>   {
>   	struct ipv6hdr *oldhdr, *hdr;
>   	int hdrlen, err;
>   
>   	hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>   
> -	err = skb_cow_head(skb, hdrlen + skb->mac_len);
> +	err = skb_cow_head(skb, hdrlen + (!dst ? skb->mac_len
> +					       : LL_RESERVED_SPACE(dst->dev)));
>   	if (unlikely(err))
>   		return err;
>   
> @@ -256,16 +258,17 @@ 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,
> -			  struct in6_addr *tundst)
> +			  struct in6_addr *tundst,
> +			  struct dst_entry *dst)
>   {
> -	struct dst_entry *dst = skb_dst(skb);
>   	struct ipv6hdr *hdr, *inner_hdr;
>   	int hdrlen, len, err;
>   
>   	hdrlen = (tuninfo->eh.hdrlen + 1) << 3;
>   	len = sizeof(*hdr) + hdrlen;
>   
> -	err = skb_cow_head(skb, len + skb->mac_len);
> +	err = skb_cow_head(skb, len + (!dst ? skb->mac_len
> +					    : LL_RESERVED_SPACE(dst->dev)));
>   	if (unlikely(err))
>   		return err;
>   
> @@ -285,7 +288,7 @@ 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_dev_get_saddr(net, skb_dst(skb)->dev, &hdr->daddr,
>   			   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
>   
>   	skb_postpush_rcsum(skb, hdr, len);
> @@ -313,6 +316,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   
>   	orig_daddr = ipv6_hdr(skb)->daddr;
>   
> +	local_bh_disable();
> +	dst = dst_cache_get(&ilwt->cache);
> +	local_bh_enable();
> +
>   	switch (ilwt->mode) {
>   	case IOAM6_IPTUNNEL_MODE_INLINE:
>   do_inline:
> @@ -320,7 +327,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   		if (ipv6_hdr(skb)->nexthdr == NEXTHDR_HOP)
>   			goto out;
>   
> -		err = ioam6_do_inline(net, skb, &ilwt->tuninfo);
> +		err = ioam6_do_inline(net, skb, &ilwt->tuninfo, dst);
>   		if (unlikely(err))
>   			goto drop;
>   
> @@ -328,7 +335,8 @@ 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->tundst, dst);
>   		if (unlikely(err))
>   			goto drop;
>   
> @@ -346,10 +354,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   		goto drop;
>   	}
>   
> -	local_bh_disable();
> -	dst = dst_cache_get(&ilwt->cache);
> -	local_bh_enable();
> -
>   	if (unlikely(!dst)) {
>   		struct ipv6hdr *hdr = ipv6_hdr(skb);
>   		struct flowi6 fl6;
> @@ -371,15 +375,15 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>   		local_bh_disable();
>   		dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
>   		local_bh_enable();
> +
> +		err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
> +		if (unlikely(err))
> +			goto drop;
>   	}
>   
>   	skb_dst_drop(skb);
>   	skb_dst_set(skb, dst);
>   
> -	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
> -	if (unlikely(err))
> -		goto drop;
> -
>   	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr))
>   		return dst_output(net, sk, skb);
>   out:

Re: [PATCH net 1/2] net: ioam6: use "new" dst entry with skb_cow_head

[Paolo Abeni]

On Tue, 2024-07-02 at 19:44 +0200, Justin Iurman wrote:
> In ioam6_output(), we call skb_cow_head() with LL_RESERVED_SPACE(). The
> latter uses "dst", which is potentially not the good one anymore. As a
> consequence, there might not be enough headroom, depending on the "new"
> dev (e.g., needed_headroom > 0). Therefore, we first need to get the
> "new" dst entry (from the cache or by calling ip6_route_output()), just
> like seg6 and rpl both do, and only then call skb_cow_head() with
> LL_RESERVED_SPACE() on the "new" dst entry.
> 
> Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
> Signed-off-by: Justin Iurman <justin.iurman@uliege.be>
> ---
>  net/ipv6/ioam6_iptunnel.c | 65 +++++++++++++++++++--------------------
>  1 file changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
> index bf7120ecea1e..b08c13550144 100644
> --- a/net/ipv6/ioam6_iptunnel.c
> +++ b/net/ipv6/ioam6_iptunnel.c
> @@ -295,7 +295,7 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
>  
>  static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  {
> -	struct dst_entry *dst = skb_dst(skb);
> +	struct dst_entry *dst, *orig_dst = skb_dst(skb);
>  	struct in6_addr orig_daddr;
>  	struct ioam6_lwt *ilwt;
>  	int err = -EINVAL;
> @@ -304,7 +304,7 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  	if (skb->protocol != htons(ETH_P_IPV6))
>  		goto drop;
>  
> -	ilwt = ioam6_lwt_state(dst->lwtstate);
> +	ilwt = ioam6_lwt_state(orig_dst->lwtstate);
>  
>  	/* Check for insertion frequency (i.e., "k over n" insertions) */
>  	pkt_cnt = atomic_fetch_inc(&ilwt->pkt_cnt);
> @@ -346,45 +346,44 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  		goto drop;
>  	}
>  
> -	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
> -	if (unlikely(err))
> -		goto drop;
> +	local_bh_disable();
> +	dst = dst_cache_get(&ilwt->cache);
> +	local_bh_enable();

This makes the dst lookup and cache update unconditional, at best
slowing down ioam6_output(), possibly introducing unintended side
effects.

> +
> +	if (unlikely(!dst)) {
> +		struct ipv6hdr *hdr = ipv6_hdr(skb);
> +		struct flowi6 fl6;
> +
> +		memset(&fl6, 0, sizeof(fl6));
> +		fl6.daddr = hdr->daddr;
> +		fl6.saddr = hdr->saddr;
> +		fl6.flowlabel = ip6_flowinfo(hdr);
> +		fl6.flowi6_mark = skb->mark;
> +		fl6.flowi6_proto = hdr->nexthdr;
> +
> +		dst = ip6_route_output(net, NULL, &fl6);
> +		if (dst->error) {
> +			err = dst->error;
> +			dst_release(dst);
> +			goto drop;
> +		}
>  
> -	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr)) {
>  		local_bh_disable();
> -		dst = dst_cache_get(&ilwt->cache);
> +		dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
>  		local_bh_enable();
> +	}
>  
> -		if (unlikely(!dst)) {
> -			struct ipv6hdr *hdr = ipv6_hdr(skb);
> -			struct flowi6 fl6;
> -
> -			memset(&fl6, 0, sizeof(fl6));
> -			fl6.daddr = hdr->daddr;
> -			fl6.saddr = hdr->saddr;
> -			fl6.flowlabel = ip6_flowinfo(hdr);
> -			fl6.flowi6_mark = skb->mark;
> -			fl6.flowi6_proto = hdr->nexthdr;
> -
> -			dst = ip6_route_output(net, NULL, &fl6);
> -			if (dst->error) {
> -				err = dst->error;
> -				dst_release(dst);
> -				goto drop;
> -			}
> -
> -			local_bh_disable();
> -			dst_cache_set_ip6(&ilwt->cache, dst, &fl6.saddr);
> -			local_bh_enable();
> -		}
> +	skb_dst_drop(skb);
> +	skb_dst_set(skb, dst);
>  
> -		skb_dst_drop(skb);
> -		skb_dst_set(skb, dst);
> +	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
> +	if (unlikely(err))
> +		goto drop;

This is quite a bit of code churn, but you just need to postpone the
cow operation after the dst lookup right?

I suggest to simply move the cow there, something alike (completely
untested): 
---
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index bf7120ecea1e..636480bded0e 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -346,10 +346,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		goto drop;
 	}
 
-	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
-	if (unlikely(err))
-		goto drop;
-
 	if (!ipv6_addr_equal(&orig_daddr, &ipv6_hdr(skb)->daddr)) {
 		local_bh_disable();
 		dst = dst_cache_get(&ilwt->cache);
@@ -381,9 +377,17 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		skb_dst_drop(skb);
 		skb_dst_set(skb, dst);
 
+		err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+		if (unlikely(err))
+			goto drop;
+
 		return dst_output(net, sk, skb);
 	}
 out:
+	err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
+	if (unlikely(err))
+		goto drop;
+
 	return dst->lwtstate->orig_output(net, sk, skb);
 drop:
 	kfree_skb(skb);
---
Thanks,

Paolo

Re: [PATCH net 2/2] net: ioam6: mitigate the two reallocations problem

[Paolo Abeni]

On Tue, 2024-07-02 at 19:44 +0200, Justin Iurman wrote:
> @@ -313,6 +316,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  
>  	orig_daddr = ipv6_hdr(skb)->daddr;
>  
> +	local_bh_disable();
> +	dst = dst_cache_get(&ilwt->cache);
> +	local_bh_enable();
> +
>  	switch (ilwt->mode) {
>  	case IOAM6_IPTUNNEL_MODE_INLINE:

I now see that the way you coded patch 1/2 makes this one easier.

Still I think it's quite doubtful to make the dst cache access
unconditional.

Given the above I suggest to replace the 2 patches with a single one
moving the whole dst_cache logic before the switch statement.

Also this does not address a functional issue, IMHO it's more a
performance improvement, could as well target net-next with no fixes
tag.

WRT seg6 and rpl tunnels, before any patch, I think we first need
confirmation the problem is present there, too.

Thanks,

Paolo

Re: [PATCH net 2/2] net: ioam6: mitigate the two reallocations problem

[Justin Iurman]

On 7/4/24 11:23, Paolo Abeni wrote:
> On Tue, 2024-07-02 at 19:44 +0200, Justin Iurman wrote:
>> @@ -313,6 +316,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>>   
>>   	orig_daddr = ipv6_hdr(skb)->daddr;
>>   
>> +	local_bh_disable();
>> +	dst = dst_cache_get(&ilwt->cache);
>> +	local_bh_enable();
>> +
>>   	switch (ilwt->mode) {
>>   	case IOAM6_IPTUNNEL_MODE_INLINE:
> 
> I now see that the way you coded patch 1/2 makes this one easier.

Hi Paolo,

Indeed. I originally had it as a single two-in-one patch, then I thought 
it would be clearer to split it up (looks like I was wrong, sorry).

> Still I think it's quite doubtful to make the dst cache access
> unconditional.

By unconditional, you mean to get the cache _before_ the switch, right? 
If so, that's indeed the only solution to provide it to the encap/inline 
function for the mitigation. However, I don't see it as a problem. 
Instead of having (a) call encap/fill function, then (b) get cache; 
you'd have (a) get cache, then (b) call encap/fill function. IMHO, it's 
the same. I'll re-run our measurements and compare them to our previous 
results in order to confirm getting the cache early does not impact 
performance. The only exception would be when skb_cow_head returns an 
error in encap/fill functions: in that case, getting the cache early 
would be a waste of time, but this situation suggests there is a problem 
already so it's probably fine.

> Given the above I suggest to replace the 2 patches with a single one
> moving the whole dst_cache logic before the switch statement.

Will do!

> Also this does not address a functional issue, IMHO it's more a
> performance improvement, could as well target net-next with no fixes
> tag.

Hmmm, it's indeed OK to target net-next for patch #2 since it could be 
considered as an improvement (not really a functional issue per se). 
However, I'm not sure for patch #1. Wouldn't the kernel crash if not 
enough headroom was allocated (assuming no check is done before writing 
in the driver)?

> WRT seg6 and rpl tunnels, before any patch, I think we first need
> confirmation the problem is present there, too.

Ack. I'll try to run some tests to check that.

Thanks,
Justin

> Thanks,
> 
> Paolo
>