[PATCH net v2 1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels

[PATCH net v2 1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels

[Jakub Kicinski]

dst_cache_get() gives us a reference, we need to release it.

Discovered by the ioam6.sh test, kmemleak was recently fixed
to catch per-cpu memory leaks.

Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue")
Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue")
Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue")
Reviewed-by: Justin Iurman <justin.iurman@uliege.be>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: dsahern@kernel.org
---
 net/ipv6/ioam6_iptunnel.c | 5 +++--
 net/ipv6/rpl_iptunnel.c   | 6 ++++--
 net/ipv6/seg6_iptunnel.c  | 6 ++++--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index 28e5a89dc255..3936c137a572 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -336,7 +336,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), *cache_dst;
+	struct dst_entry *dst = skb_dst(skb), *cache_dst = NULL;
 	struct in6_addr orig_daddr;
 	struct ioam6_lwt *ilwt;
 	int err = -EINVAL;
@@ -407,7 +407,6 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		cache_dst = ip6_route_output(net, NULL, &fl6);
 		if (cache_dst->error) {
 			err = cache_dst->error;
-			dst_release(cache_dst);
 			goto drop;
 		}
 
@@ -426,8 +425,10 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		return dst_output(net, sk, skb);
 	}
 out:
+	dst_release(cache_dst);
 	return dst->lwtstate->orig_output(net, sk, skb);
 drop:
+	dst_release(cache_dst);
 	kfree_skb(skb);
 	return err;
 }
diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c
index 7ba22d2f2bfe..9b7d03563115 100644
--- a/net/ipv6/rpl_iptunnel.c
+++ b/net/ipv6/rpl_iptunnel.c
@@ -232,7 +232,6 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 		dst = ip6_route_output(net, NULL, &fl6);
 		if (dst->error) {
 			err = dst->error;
-			dst_release(dst);
 			goto drop;
 		}
 
@@ -251,6 +250,7 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 	return dst_output(net, sk, skb);
 
 drop:
+	dst_release(dst);
 	kfree_skb(skb);
 	return err;
 }
@@ -269,8 +269,10 @@ static int rpl_input(struct sk_buff *skb)
 	local_bh_enable();
 
 	err = rpl_do_srh(skb, rlwt, dst);
-	if (unlikely(err))
+	if (unlikely(err)) {
+		dst_release(dst);
 		goto drop;
+	}
 
 	if (!dst) {
 		ip6_route_input(skb);
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index 4bf937bfc263..eacc4e91b48e 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -482,8 +482,10 @@ static int seg6_input_core(struct net *net, struct sock *sk,
 	local_bh_enable();
 
 	err = seg6_do_srh(skb, dst);
-	if (unlikely(err))
+	if (unlikely(err)) {
+		dst_release(dst);
 		goto drop;
+	}
 
 	if (!dst) {
 		ip6_route_input(skb);
@@ -571,7 +573,6 @@ static int seg6_output_core(struct net *net, struct sock *sk,
 		dst = ip6_route_output(net, NULL, &fl6);
 		if (dst->error) {
 			err = dst->error;
-			dst_release(dst);
 			goto drop;
 		}
 
@@ -593,6 +594,7 @@ static int seg6_output_core(struct net *net, struct sock *sk,
 
 	return dst_output(net, sk, skb);
 drop:
+	dst_release(dst);
 	kfree_skb(skb);
 	return err;
 }
-- 
2.48.1

[PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Jakub Kicinski]

Some lwtunnels have a dst cache for post-transformation dst.
If the packet destination did not change we may end up recording
a reference to the lwtunnel in its own cache, and the lwtunnel
state will never be freed.

Discovered by the ioam6.sh test, kmemleak was recently fixed
to catch per-cpu memory leaks. I'm not sure if rpl and seg6
can actually hit this, but in principle I don't see why not.

Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue")
Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue")
Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - fix spello in the comments
v1: https://lore.kernel.org/20250129021346.2333089-2-kuba@kernel.org

CC: dsahern@kernel.org
CC: justin.iurman@uliege.be
---
 net/ipv6/ioam6_iptunnel.c | 9 ++++++---
 net/ipv6/rpl_iptunnel.c   | 9 ++++++---
 net/ipv6/seg6_iptunnel.c  | 9 ++++++---
 3 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index 3936c137a572..2c383c12a431 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -410,9 +410,12 @@ static int ioam6_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 			goto drop;
 		}
 
-		local_bh_disable();
-		dst_cache_set_ip6(&ilwt->cache, cache_dst, &fl6.saddr);
-		local_bh_enable();
+		/* cache only if we don't create a dst reference loop */
+		if (dst->lwtstate != cache_dst->lwtstate) {
+			local_bh_disable();
+			dst_cache_set_ip6(&ilwt->cache, cache_dst, &fl6.saddr);
+			local_bh_enable();
+		}
 
 		err = skb_cow_head(skb, LL_RESERVED_SPACE(cache_dst->dev));
 		if (unlikely(err))
diff --git a/net/ipv6/rpl_iptunnel.c b/net/ipv6/rpl_iptunnel.c
index 9b7d03563115..0ac4283acdf2 100644
--- a/net/ipv6/rpl_iptunnel.c
+++ b/net/ipv6/rpl_iptunnel.c
@@ -235,9 +235,12 @@ static int rpl_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 			goto drop;
 		}
 
-		local_bh_disable();
-		dst_cache_set_ip6(&rlwt->cache, dst, &fl6.saddr);
-		local_bh_enable();
+		/* cache only if we don't create a dst reference loop */
+		if (orig_dst->lwtstate != dst->lwtstate) {
+			local_bh_disable();
+			dst_cache_set_ip6(&rlwt->cache, dst, &fl6.saddr);
+			local_bh_enable();
+		}
 
 		err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
 		if (unlikely(err))
diff --git a/net/ipv6/seg6_iptunnel.c b/net/ipv6/seg6_iptunnel.c
index eacc4e91b48e..33833b2064c0 100644
--- a/net/ipv6/seg6_iptunnel.c
+++ b/net/ipv6/seg6_iptunnel.c
@@ -576,9 +576,12 @@ static int seg6_output_core(struct net *net, struct sock *sk,
 			goto drop;
 		}
 
-		local_bh_disable();
-		dst_cache_set_ip6(&slwt->cache, dst, &fl6.saddr);
-		local_bh_enable();
+		/* cache only if we don't create a dst reference loop */
+		if (orig_dst->lwtstate != dst->lwtstate) {
+			local_bh_disable();
+			dst_cache_set_ip6(&slwt->cache, dst, &fl6.saddr);
+			local_bh_enable();
+		}
 
 		err = skb_cow_head(skb, LL_RESERVED_SPACE(dst->dev));
 		if (unlikely(err))
-- 
2.48.1

Re: [PATCH net v2 1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels

[Simon Horman]

On Wed, Jan 29, 2025 at 07:15:18PM -0800, Jakub Kicinski wrote:
> dst_cache_get() gives us a reference, we need to release it.
> 
> Discovered by the ioam6.sh test, kmemleak was recently fixed
> to catch per-cpu memory leaks.
> 
> Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue")
> Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue")
> Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue")
> Reviewed-by: Justin Iurman <justin.iurman@uliege.be>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Simon Horman]

On Wed, Jan 29, 2025 at 07:15:19PM -0800, Jakub Kicinski wrote:
> Some lwtunnels have a dst cache for post-transformation dst.
> If the packet destination did not change we may end up recording
> a reference to the lwtunnel in its own cache, and the lwtunnel
> state will never be freed.
> 
> Discovered by the ioam6.sh test, kmemleak was recently fixed
> to catch per-cpu memory leaks. I'm not sure if rpl and seg6
> can actually hit this, but in principle I don't see why not.
> 
> Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue")
> Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue")
> Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>  - fix spello in the comments
> v1: https://lore.kernel.org/20250129021346.2333089-2-kuba@kernel.org

Hi Jakub,

This fix looks correct to me. And I believe that the double allocation
issue raised at the cited link for v1 relates to an optimisation
rather than a bug, so this patch seems appropriate for net without
addressing that issue.

I am, however, unsure why the cited patches are used in the Fixes tags
rather than the patches that added use of the cache to the output
routines.

e.g. af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input")

...

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Paolo Abeni]

On 1/30/25 4:15 AM, Jakub Kicinski wrote:
> Some lwtunnels have a dst cache for post-transformation dst.
> If the packet destination did not change we may end up recording
> a reference to the lwtunnel in its own cache, and the lwtunnel
> state will never be freed.

The series LGTM, but I'm wondering if we can't have a similar loop for
input lwt?

Thanks,

Paolo

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Justin Iurman]

On 1/30/25 11:28, Simon Horman wrote:
> On Wed, Jan 29, 2025 at 07:15:19PM -0800, Jakub Kicinski wrote:
>> Some lwtunnels have a dst cache for post-transformation dst.
>> If the packet destination did not change we may end up recording
>> a reference to the lwtunnel in its own cache, and the lwtunnel
>> state will never be freed.
>>
>> Discovered by the ioam6.sh test, kmemleak was recently fixed
>> to catch per-cpu memory leaks. I'm not sure if rpl and seg6
>> can actually hit this, but in principle I don't see why not.
>>
>> Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue")
>> Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue")
>> Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue")
>> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
>> ---
>> v2:
>>   - fix spello in the comments
>> v1: https://lore.kernel.org/20250129021346.2333089-2-kuba@kernel.org
> 
> Hi Jakub,
> 
> This fix looks correct to me. And I believe that the double allocation
> issue raised at the cited link for v1 relates to an optimisation
> rather than a bug, so this patch seems appropriate for net without
> addressing that issue.

+1. Just to make sure, do you think I should re-apply a fix for the 
double allocation on top of this one and target net or net-next?

> I am, however, unsure why the cited patches are used in the Fixes tags
> rather than the patches that added use of the cache to the output
> routines.
> 
> e.g. af4a2209b134 ("ipv6: sr: use dst_cache in seg6_input")
> 
> ...

This was my thought as well. While Fixes tags are correct for #1, what 
#2 is trying to fix was already there before 985ec6f5e623, 40475b63761a 
and dce525185bc9 respectively. I think it should be:

Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and 
injection with lwtunnels")
Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel")

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Justin Iurman]

On 1/30/25 12:34, Paolo Abeni wrote:
> 
> 
> On 1/30/25 4:15 AM, Jakub Kicinski wrote:
>> Some lwtunnels have a dst cache for post-transformation dst.
>> If the packet destination did not change we may end up recording
>> a reference to the lwtunnel in its own cache, and the lwtunnel
>> state will never be freed.
> 
> The series LGTM, but I'm wondering if we can't have a similar loop for
> input lwt?

Hmmm, I think Paolo is right. At least, I don't see a reason why it 
wouldn't be correct. We should also take care of input lwt for both 
seg6_iptunnel and rpl_iptunnel (ioam6_iptunnel does not implement input).

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Jakub Kicinski]

On Thu, 30 Jan 2025 14:52:14 +0100 Justin Iurman wrote:
> > On 1/30/25 4:15 AM, Jakub Kicinski wrote:  
> >> Some lwtunnels have a dst cache for post-transformation dst.
> >> If the packet destination did not change we may end up recording
> >> a reference to the lwtunnel in its own cache, and the lwtunnel
> >> state will never be freed.  
> > 
> > The series LGTM, but I'm wondering if we can't have a similar loop for
> > input lwt?  
> 
> Hmmm, I think Paolo is right. At least, I don't see a reason why it 
> wouldn't be correct. We should also take care of input lwt for both 
> seg6_iptunnel and rpl_iptunnel (ioam6_iptunnel does not implement input).

Would you be able to take care of that? And perhaps add a selftest at
least for the looped cases?

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Jakub Kicinski]

On Thu, 30 Jan 2025 14:41:56 +0100 Justin Iurman wrote:
> This was my thought as well. While Fixes tags are correct for #1, what 
> #2 is trying to fix was already there before 985ec6f5e623, 40475b63761a 
> and dce525185bc9 respectively. I think it should be:
> 
> Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
> Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and 
> injection with lwtunnels")
> Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel")

I'll swap the tags when applying, if there are no other comments.

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Justin Iurman]

On 1/30/25 15:55, Jakub Kicinski wrote:
> On Thu, 30 Jan 2025 14:52:14 +0100 Justin Iurman wrote:
>>> On 1/30/25 4:15 AM, Jakub Kicinski wrote:
>>>> Some lwtunnels have a dst cache for post-transformation dst.
>>>> If the packet destination did not change we may end up recording
>>>> a reference to the lwtunnel in its own cache, and the lwtunnel
>>>> state will never be freed.
>>>
>>> The series LGTM, but I'm wondering if we can't have a similar loop for
>>> input lwt?
>>
>> Hmmm, I think Paolo is right. At least, I don't see a reason why it
>> wouldn't be correct. We should also take care of input lwt for both
>> seg6_iptunnel and rpl_iptunnel (ioam6_iptunnel does not implement input).
> 
> Would you be able to take care of that?

Sure, I'll send a patch as soon as this patchset is merged to net.

> And perhaps add a selftest at least for the looped cases?

ioam6.sh already triggers the looped cases in both inline and encap 
tests. Not sure about seg6 though, and there is no selftest for rpl.

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Simon Horman]

On Thu, Jan 30, 2025 at 06:56:10AM -0800, Jakub Kicinski wrote:
> On Thu, 30 Jan 2025 14:41:56 +0100 Justin Iurman wrote:
> > This was my thought as well. While Fixes tags are correct for #1, what 
> > #2 is trying to fix was already there before 985ec6f5e623, 40475b63761a 
> > and dce525185bc9 respectively. I think it should be:
> > 
> > Fixes: 8cb3bf8bff3c ("ipv6: ioam: Add support for the ip6ip6 encapsulation")
> > Fixes: 6c8702c60b88 ("ipv6: sr: add support for SRH encapsulation and 
> > injection with lwtunnels")
> > Fixes: a7a29f9c361f ("net: ipv6: add rpl sr tunnel")
> 
> I'll swap the tags when applying, if there are no other comments.

Thanks, SGTM.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Jakub Kicinski]

On Thu, 30 Jan 2025 16:12:23 +0100 Justin Iurman wrote:
> > And perhaps add a selftest at least for the looped cases?  
> 
> ioam6.sh already triggers the looped cases in both inline and encap 
> tests. Not sure about seg6 though, and there is no selftest for rpl.

Right! To be clear I meant just for seg6 and rpl. The ioam6 test 
is clearly paying off, thanks for putting in the work there! :)

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Justin Iurman]

On 1/30/25 17:53, Jakub Kicinski wrote:
> On Thu, 30 Jan 2025 16:12:23 +0100 Justin Iurman wrote:
>>> And perhaps add a selftest at least for the looped cases?
>>
>> ioam6.sh already triggers the looped cases in both inline and encap
>> tests. Not sure about seg6 though, and there is no selftest for rpl.
> 
> Right! To be clear I meant just for seg6 and rpl. The ioam6 test
> is clearly paying off, thanks for putting in the work there! :)

Got it. Of course, I'll see what I can do. And glad the ioam6 selftest 
helped :-) Not sure what the current SRv6 selftests are doing though, 
but it looks like the "looped cases" were not caught. Probably because 
the first segment makes it so that the new dst_entry is not the same as 
the origin (I'll have to double check). I could just add a test with a 
route matching on a subnet, so that the next segment matches too (i.e., 
old dst == new dst). Overall, both seg6 and rpl selftests to detect the 
looped cases would look like "dummy" tests, i.e., useless without 
kmemleak. Does it sound OK?

Re: [PATCH net v2 2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels

[Jakub Kicinski]

On Sat, 1 Feb 2025 14:52:05 +0100 Justin Iurman wrote:
> >> ioam6.sh already triggers the looped cases in both inline and encap
> >> tests. Not sure about seg6 though, and there is no selftest for rpl.  
> > 
> > Right! To be clear I meant just for seg6 and rpl. The ioam6 test
> > is clearly paying off, thanks for putting in the work there! :)  
> 
> Got it. Of course, I'll see what I can do. And glad the ioam6 selftest 
> helped :-) Not sure what the current SRv6 selftests are doing though, 
> but it looks like the "looped cases" were not caught. Probably because 
> the first segment makes it so that the new dst_entry is not the same as 
> the origin (I'll have to double check). I could just add a test with a 
> route matching on a subnet, so that the next segment matches too (i.e., 
> old dst == new dst). Overall, both seg6 and rpl selftests to detect the 
> looped cases would look like "dummy" tests, i.e., useless without 
> kmemleak. Does it sound OK?

Yes, tests written purely for code coverage are perfectly acceptable.
Let alone when they cover paths which where buggy in the past.

FWIW my series was applied now, in case you were waiting.

Re: [PATCH net v2 1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 29 Jan 2025 19:15:18 -0800 you wrote:
> dst_cache_get() gives us a reference, we need to release it.
> 
> Discovered by the ioam6.sh test, kmemleak was recently fixed
> to catch per-cpu memory leaks.
> 
> Fixes: 985ec6f5e623 ("net: ipv6: rpl_iptunnel: mitigate 2-realloc issue")
> Fixes: 40475b63761a ("net: ipv6: seg6_iptunnel: mitigate 2-realloc issue")
> Fixes: dce525185bc9 ("net: ipv6: ioam6_iptunnel: mitigate 2-realloc issue")
> Reviewed-by: Justin Iurman <justin.iurman@uliege.be>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> 
> [...]

Here is the summary with links:
  - [net,v2,1/2] net: ipv6: fix dst refleaks in rpl, seg6 and ioam6 lwtunnels
    https://git.kernel.org/netdev/net/c/c71a192976de
  - [net,v2,2/2] net: ipv6: fix dst ref loops in rpl, seg6 and ioam6 lwtunnels
    https://git.kernel.org/netdev/net/c/92191dd10730

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html