[PATCH net] net: do not delay dst_entries_add() in dst_release()

[PATCH net] net: do not delay dst_entries_add() in dst_release()

[Eric Dumazet]

dst_entries_add() uses per-cpu data that might be freed at netns
dismantle from ip6_route_net_exit() calling dst_entries_destroy()

Before ip6_route_net_exit() can be called, we release all
the dsts associated with this netns, via calls to dst_release(),
which waits an rcu grace period before calling dst_destroy()

dst_entries_add() use in dst_destroy() is racy, because
dst_entries_destroy() could have been called already.

Decrementing the number of dsts must happen sooner.

Notes:

1) in CONFIG_XFRM case, dst_destroy() can call
   dst_release_immediate(child), this might also cause UAF
   if the child does not have DST_NOCOUNT set.
   IPSEC maintainers might take a look and see how to address this.

2) There is also discussion about removing this count of dst,
   which might happen in future kernels.

Fixes: f88649721268 ("ipv4: fix dst race in sk_dst_get()")
Closes: https://lore.kernel.org/lkml/CANn89iLCCGsP7SFn9HKpvnKu96Td4KD08xf7aGtiYgZnkjaL=w@mail.gmail.com/T/
Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Xin Long <lucien.xin@gmail.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/core/dst.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/core/dst.c b/net/core/dst.c
index 95f533844f17f119c09f335ccf9bf09515dd3606..9552a90d4772dce49b5fe94d2f1d8da6979d9908 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -109,9 +109,6 @@ static void dst_destroy(struct dst_entry *dst)
 		child = xdst->child;
 	}
 #endif
-	if (!(dst->flags & DST_NOCOUNT))
-		dst_entries_add(dst->ops, -1);
-
 	if (dst->ops->destroy)
 		dst->ops->destroy(dst);
 	netdev_put(dst->dev, &dst->dev_tracker);
@@ -159,17 +156,27 @@ void dst_dev_put(struct dst_entry *dst)
 }
 EXPORT_SYMBOL(dst_dev_put);
 
+static void dst_count_dec(struct dst_entry *dst)
+{
+	if (!(dst->flags & DST_NOCOUNT))
+		dst_entries_add(dst->ops, -1);
+}
+
 void dst_release(struct dst_entry *dst)
 {
-	if (dst && rcuref_put(&dst->__rcuref))
+	if (dst && rcuref_put(&dst->__rcuref)) {
+		dst_count_dec(dst);
 		call_rcu_hurry(&dst->rcu_head, dst_destroy_rcu);
+	}
 }
 EXPORT_SYMBOL(dst_release);
 
 void dst_release_immediate(struct dst_entry *dst)
 {
-	if (dst && rcuref_put(&dst->__rcuref))
+	if (dst && rcuref_put(&dst->__rcuref)) {
+		dst_count_dec(dst);
 		dst_destroy(dst);
+	}
 }
 EXPORT_SYMBOL(dst_release_immediate);
 
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH net] net: do not delay dst_entries_add() in dst_release()

[Xin Long]

On Tue, Oct 8, 2024 at 10:31 AM Eric Dumazet <edumazet@google.com> wrote:
>
> dst_entries_add() uses per-cpu data that might be freed at netns
> dismantle from ip6_route_net_exit() calling dst_entries_destroy()
>
> Before ip6_route_net_exit() can be called, we release all
> the dsts associated with this netns, via calls to dst_release(),
> which waits an rcu grace period before calling dst_destroy()
>
> dst_entries_add() use in dst_destroy() is racy, because
> dst_entries_destroy() could have been called already.
>
> Decrementing the number of dsts must happen sooner.
>
> Notes:
>
> 1) in CONFIG_XFRM case, dst_destroy() can call
>    dst_release_immediate(child), this might also cause UAF
>    if the child does not have DST_NOCOUNT set.
>    IPSEC maintainers might take a look and see how to address this.
>
> 2) There is also discussion about removing this count of dst,
>    which might happen in future kernels.
>
> Fixes: f88649721268 ("ipv4: fix dst race in sk_dst_get()")
> Closes: https://lore.kernel.org/lkml/CANn89iLCCGsP7SFn9HKpvnKu96Td4KD08xf7aGtiYgZnkjaL=w@mail.gmail.com/T/
> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Tested-by: Naresh Kamboju <naresh.kamboju@linaro.org>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Xin Long <lucien.xin@gmail.com>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>

Reviewed-by: Xin Long <lucien.xin@gmail.com>

Re: [PATCH net] net: do not delay dst_entries_add() in dst_release()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Tue,  8 Oct 2024 14:31:10 +0000 you wrote:
> dst_entries_add() uses per-cpu data that might be freed at netns
> dismantle from ip6_route_net_exit() calling dst_entries_destroy()
> 
> Before ip6_route_net_exit() can be called, we release all
> the dsts associated with this netns, via calls to dst_release(),
> which waits an rcu grace period before calling dst_destroy()
> 
> [...]

Here is the summary with links:
  - [net] net: do not delay dst_entries_add() in dst_release()
    https://git.kernel.org/netdev/net/c/ac888d58869b

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