[PATCH v2] RDMA/rxe: unpublish the per-net tunnel socket before

[PATCH v2] RDMA/rxe: unpublish the per-net tunnel socket before

[Binbin Deng]

KASAN reports a slab-use-after-free in ip6_route_output_flags()
reached from rxe_find_route(), with the free in __sk_destruct() after
rxe_sock_put(), and a user with CAP_NET_ADMIN can remove the device with
"rdma link del rxe0" while RoCE v2 over IPv6 traffic keeps looking the
socket up.

BUG: KASAN: slab-use-after-free in ip6_route_output_flags+0x300/0x360
Read of size 4 at addr ffff888115ddd794 by task kworker/u32:6/309
Call Trace:
 <TASK>
 dump_stack_lvl+0x53/0x70
 print_report+0xd0/0x630
 ? __pfx__raw_spin_lock_irqsave+0x10/0x10
 ? ip6_route_output_flags+0x300/0x360
 kasan_report+0xce/0x100
 ? ip6_route_output_flags+0x300/0x360
 ip6_route_output_flags+0x300/0x360
 ip6_dst_lookup_tail.constprop.0+0x76c/0xcc0
 ? ct_nmi_exit+0xc3/0xf0
 ip6_dst_lookup_flow+0xf5/0x1e0
 ? __pfx_ip6_dst_lookup_flow+0x10/0x10
 rxe_find_route+0x426/0xa30
 ? __kasan_slab_alloc+0x6e/0x70
 ? __pfx_rxe_find_route+0x10/0x10
 ? kmem_cache_alloc_node_noprof+0x141/0x370
 ? kmalloc_reserve+0x103/0x2b0
 ? rxe_icrc_generate+0x229/0x330
 ? __pfx___alloc_skb+0x10/0x10
 rxe_prepare+0x9e8/0x18b0
 ? rxe_init_packet+0x3c7/0x4f0
 rxe_requester+0x1a0f/0x51f0
 ? rxe_completer+0x1de9/0x38c0
 ? __pfx_rxe_completer+0x10/0x10
 ? __queue_work+0x43e/0x11f0
 ? __pfx_rxe_requester+0x10/0x10
 ? irqentry_exit+0xd2/0x640
 ? _raw_spin_lock_irqsave+0x85/0xe0
 ? __pfx__raw_spin_lock_irqsave+0x10/0x10
 ? __pfx_rxe_sender+0x10/0x10
 rxe_sender+0xe/0x30
 do_work+0x144/0x470
 process_one_work+0x633/0x1030
 ? assign_work+0x11d/0x370
 worker_thread+0x45b/0xd10
 ? __pfx_worker_thread+0x10/0x10
 kthread+0x2c6/0x3b0
 ? recalc_sigpending+0x15c/0x1e0
 ? __pfx_kthread+0x10/0x10
 ret_from_fork+0x36e/0x5a0
 ? __pfx_ret_from_fork+0x10/0x10
 ? __switch_to+0x572/0xdd0
 ? __pfx_kthread+0x10/0x10
 ret_from_fork_asm+0x1a/0x30
 </TASK>

Allocated by task 146020:
 kasan_save_stack+0x33/0x60
 kasan_save_track+0x14/0x30
 __kasan_slab_alloc+0x6e/0x70
 kmem_cache_alloc_noprof+0x130/0x360
 sk_prot_alloc+0x56/0x210

Fix by clearing the per-net pointer before the last reference is dropped.
It has been confirmed that this fix has been validated in the test environment.

Fixes: f1327abd6abed ("RDMA/rxe: Support RDMA link creation and destruction per net namespace")
Tested-by: Binbin Deng <18983559317@163.com>
Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 drivers/infiniband/sw/rxe/rxe_net.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
index 53daaf4c1eb2..fc597e02c85c 100644
--- a/drivers/infiniband/sw/rxe/rxe_net.c
+++ b/drivers/infiniband/sw/rxe/rxe_net.c
@@ -638,9 +638,8 @@ static void rxe_sock_put(struct sock *sk,
 	if (refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL) {
 		__sock_put(sk);
 	} else {
+		set_sk(net, NULL);
 		rxe_release_udp_tunnel(sk);
-		sk = NULL;
-		set_sk(net, sk);
 	}
 }
 
-- 
2.43.0

Re: [PATCH v2] RDMA/rxe: unpublish the per-net tunnel socket before

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] RDMA/rxe: use-after-free in rxe_find_route6() due to unpinned socket
- [High] RDMA/rxe: unsynchronized sk_refcnt check and socket release in rxe_sock_put()
--

commit 860836b4fabbeecebc7b1940a3702993878d2022
Author: Binbin Deng <18983559317@163.com>

RDMA/rxe: unpublish the per-net tunnel socket before

This commit moves the call to set_sk(net, NULL) before
rxe_release_udp_tunnel(sk) in rxe_sock_put() to avoid a slab use-after-free in
ip6_route_output_flags() when a user removes an RXE device while RoCE v2 over
IPv6 traffic is active.

> diff --git a/drivers/infiniband/sw/rxe/rxe_net.c b/drivers/infiniband/sw/rxe/rxe_net.c
> index 53daaf4c1eb2e..fc597e02c85ca 100644
> --- a/drivers/infiniband/sw/rxe/rxe_net.c
> +++ b/drivers/infiniband/sw/rxe/rxe_net.c
> @@ -638,9 +638,8 @@ static void rxe_sock_put(struct sock *sk,
>  	if (refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL) {
>  		__sock_put(sk);
>  	} else {
> +		set_sk(net, NULL);

[Severity: Critical]
Does calling set_sk(net, NULL) before rxe_release_udp_tunnel(sk) actually
prevent the slab use-after-free in ip6_route_output_flags() reached from
rxe_find_route6()?

In drivers/infiniband/sw/rxe/rxe_ns.c, rxe_ns_pernet_sk6() drops
rcu_read_unlock() before returning the raw sk pointer without acquiring a
reference on sk:

struct sock *rxe_ns_pernet_sk6(struct net *net)
{
	struct rxe_ns_sock *ns_sk = net_generic(net, rxe_pernet_id);
	struct sock *sk;

	rcu_read_lock();
	sk = rcu_dereference(ns_sk->rxe_sk6);
	rcu_read_unlock();

	return sk;
}

And in drivers/infiniband/sw/rxe/rxe_net.c, rxe_find_route6() runs in
preemptible workqueue context (rxe_sender() -> rxe_requester() ->
rxe_prepare() -> rxe_find_route() -> rxe_find_route6()) outside any RCU
read-side critical section:

	ndst = ip6_dst_lookup_flow(net, rxe_ns_pernet_sk6(net), &fl6, NULL);

If CPU 0 calls rxe_ns_pernet_sk6(net), obtains the non-NULL sk pointer, and
calls rcu_read_unlock() just before CPU 1 calls set_sk(net, NULL) in
rxe_sock_put(), the synchronize_rcu() inside rxe_ns_pernet_set_sk6() will not
wait for CPU 0 because CPU 0 is no longer in an RCU read-side critical
section.

CPU 1 can then run rxe_release_udp_tunnel(sk) -> udp_tunnel_sock_release(sk) ->
sock_release() -> sock_put(sk) -> sk_free(sk) -> call_rcu(&sk->sk_rcu,
__sk_destruct). Because CPU 0 is outside rcu_read_lock() before
ip6_route_output_flags() enters its internal RCU section (and again after
ip6_route_output_flags() drops rcu_read_unlock() before xfrm_lookup_route() ->
xfrm_lookup_with_ifid()), __sk_destruct() can still free sk while CPU 0 is
dereferencing sk->sk_bound_dev_if, inet6_sk(sk)->srcprefs, or
sk->sk_policy[XFRM_POLICY_OUT].

>  		rxe_release_udp_tunnel(sk);
> -		sk = NULL;
> -		set_sk(net, sk);
>  	}
>  }

[Severity: High]
Could moving set_sk(net, NULL) before rxe_release_udp_tunnel(sk), combined
with the unsynchronized sk->sk_refcnt check in rxe_sock_put(), also introduce
a race window with concurrent link operations or socket lookups?

1. In rxe_sock_put(), set_sk(net, NULL) clears ns_sk->rxe_sk4 or
ns_sk->rxe_sk6 and sleeps in synchronize_rcu() before rxe_release_udp_tunnel()
closes and unhashes the UDP tunnel socket bound to ROCE_V2_UDP_DPORT:

	} else {
		set_sk(net, NULL);
		rxe_release_udp_tunnel(sk);
	}

Because nldev_newlink() -> rxe_newlink() -> rxe_net_init() does not serialize
with nldev_dellink() or rxe_notify(NETDEV_UNREGISTER), if rxe_net_ipv4_init()
or rxe_net_ipv6_init() runs while set_sk(net, NULL) is sleeping in
synchronize_rcu():

	sk = rxe_ns_pernet_sk4(net);
	if (sk) {
		sock_hold(sk);
		return 0;
	}

	sock = rxe_setup_udp_tunnel(net, htons(ROCE_V2_UDP_DPORT), false);

it will see a NULL per-net socket pointer and try to create a new UDP tunnel
socket on ROCE_V2_UDP_DPORT while the old socket is still bound, failing with
-EADDRINUSE.

2. Furthermore, rxe_sock_put() checks refcount_read(&sk->sk_refcnt) >
SK_REF_FOR_TUNNEL without lock serialization across rxe_net_del() callers
(nldev_dellink() and rxe_notify(NETDEV_UNREGISTER)) or rxe_net_init(), and the
UDP tunnel socket is hashed in the global UDP tables where paths like
udp_diag_dump_one() or udp4/6_lib_lookup() transiently increment sk->sk_refcnt
via refcount_inc_not_zero(). If a transient reference is held when the last
RXE device in the netns is removed, refcount_read(&sk->sk_refcnt) >
SK_REF_FOR_TUNNEL evaluates to true, causing rxe_sock_put() to call
__sock_put(sk) instead of releasing the tunnel and leaving a dangling socket
with an unbalanced refcount published in the netns.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019023717.455067-1-18983559317@163.com?part=1

Re:Re: [PATCH v2] RDMA/rxe: unpublish the per-net tunnel socket

[Binbin Deng]

Hi,

Thanks for the review.  I verified both findings against the code and
they are correct.  Details below; a v3 that addresses them follows.

1. [Critical] residual use-after-free via the unpinned tunnel socket

You are right, v2 does not close this one.  Confirmed in code:

- rxe_ns_pernet_sk6() drops rcu_read_unlock() before returning the
  raw sk pointer, without taking a reference;
- rxe_find_route6() passes that pointer to ip6_dst_lookup_flow() in
  the rxe_sender() workqueue context, outside any RCU read-side
  critical section: ip6_route_output_flags() reads sk->sk_bound_dev_if
  and inet6_sk(sk)->srcprefs, and xfrm_lookup_with_ifid() reads
  sk->sk_policy[XFRM_POLICY_OUT];
- therefore neither the synchronize_rcu() in rxe_ns_pernet_set_sk6()
  nor the one inside udp_tunnel_sock_release() can protect a reader
  that already fetched the pointer.  After v2 only new lookups are
  protected (they see NULL, which ip6_dst_lookup_flow() handles).

What v2 does fix is the failure mode KASAN reported: with the
original ordering the per-net pointer stayed published while
udp_tunnel_sock_release() ran and until set_sk(net, NULL) completed
its grace period, so any new lookup in that window fetched an
already-freed socket.  The window for in-flight readers that had
fetched sk before the unpublish remained, as you describe.

The complete fix is to pin the socket.  v3 takes a reference on it
in rxe_ns_pernet_sk4/6() with refcount_inc_not_zero() under
rcu_read_lock() and the callers sock_put() it after use.  Extending
the RCU read-side critical section across the lookup is not an
option because xfrm_lookup_route() may sleep.

2. [High] races with rxe_net_init() / unsynchronized refcount check

Both points are valid.  They pre-date v2 (they come with
f1327abd6abe "RDMA/rxe: Support RDMA link creation and destruction
per net namespace"), but v3 addresses them as well:

- nldev_newlink() is serialized only by link_ops_rwsem, while
  nldev_dellink() uses nldev_dellink_mutex, and the NETDEV_UNREGISTER
  path in rxe_notify() takes neither: rxe_net_init() could observe
  the just-cleared pointer and fail binding ROCE_V2_UDP_DPORT with
  -EADDRINUSE, or sock_hold() a socket that was concurrently being
  released.  v3 serializes rxe_net_init(), rxe_net_del() and
  rxe_ns_exit() with a new rxe_ns_lock.

- The refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL check was
  indeed TOCTOU, and two concurrent rxe_net_del() callers ("rdma link
  del" racing NETDEV_UNREGISTER of the underlying netdev) could both
  release the tunnel.  v3 replaces the heuristic with explicit
  accounting: the per-net pointer owns a publish reference, every
  device holds its own reference (the creating device previously
  never took one, so removing either of two rxe devices in one netns
  tore the tunnel down underneath the other), and the two
  rxe_net_del() callers of one device are deduplicated.

The KASAN report from v2 is kept unchanged in the v3 commit message.
v3 also fixes the truncated subject line of v2 ("...before") and
drops the Reviewed-by tag since the patch changed.

pw-bot: cr

Re:Re: [PATCH v2] RDMA/rxe: unpublish the per-net tunnel socket

[Binbin Deng]

Hi,

Thanks for the review.  I verified both findings against the code and
they are correct.  Details below; a v3 that fixes the use-after-free
follows.

1. [Critical] residual use-after-free via the unpinned tunnel socket

You are right, v2 does not close this one.  Confirmed in code:

- rxe_ns_pernet_sk6() drops rcu_read_unlock() before returning the
  raw sk pointer, without taking a reference;
- rxe_find_route6() passes that pointer to ip6_dst_lookup_flow() in
  the rxe_sender() workqueue context, outside any RCU read-side
  critical section: ip6_route_output_flags() reads sk->sk_bound_dev_if
  and inet6_sk(sk)->srcprefs, and xfrm_lookup_with_ifid() reads
  sk->sk_policy[XFRM_POLICY_OUT];
- therefore neither the synchronize_rcu() in rxe_ns_pernet_set_sk6()
  nor the one inside udp_tunnel_sock_release() can protect a reader
  that already fetched the pointer.  After v2 only new lookups are
  protected (they see NULL, which ip6_dst_lookup_flow() handles).

What v2 does fix is the failure mode KASAN reported: with the
original ordering the per-net pointer stayed published while
udp_tunnel_sock_release() ran and until set_sk(net, NULL) completed
its grace period, so any new lookup in that window fetched an
already-freed socket.  The window for in-flight readers that had
fetched sk before the unpublish remained, as you describe.

The complete fix is to pin the socket.  v3 takes a reference on it
in rxe_ns_pernet_sk4/6() with refcount_inc_not_zero() under
rcu_read_lock() and the callers sock_put() it after use.  Extending
the RCU read-side critical section across the lookup is not an
option because xfrm_lookup_route() may sleep.

2. [High] races with rxe_net_init() / unsynchronized refcount check

Both points are valid.  They pre-date v2 (they come with
f1327abd6abe "RDMA/rxe: Support RDMA link creation and destruction
per net namespace") and are not introduced by it:

- nldev_newlink() is serialized only by link_ops_rwsem, while
  nldev_dellink() uses nldev_dellink_mutex, and the NETDEV_UNREGISTER
  path in rxe_notify() takes neither: rxe_net_init() could observe
  the just-cleared pointer and fail binding ROCE_V2_UDP_DPORT with
  -EADDRINUSE, or sock_hold() a socket that was concurrently being
  released.  With v3 rxe_net_ipv4/6_init() acquire the socket through
  the pinned getter, so they can no longer resurrect a socket that
  is being released; the -EADDRINUSE window itself remains.

- The refcount_read(&sk->sk_refcnt) > SK_REF_FOR_TUNNEL check was
  indeed TOCTOU, and two concurrent rxe_net_del() callers ("rdma link
  del" racing NETDEV_UNREGISTER of the underlying netdev) could both
  release the tunnel.

Since these races exist in the code that v2 and v3 fix and are
separate from the use-after-free at hand, I kept v3 minimal and
scoped to the socket lifetime fix; I plan to send a follow-up
series with proper locking and per-device reference accounting for
them.

The KASAN report from v2 is kept unchanged in the v3 commit message.
v3 also fixes the truncated subject line of v2 ("...before") and
drops the Reviewed-by tag since the patch changed.

pw-bot: cr