[PATCH net] rxrpc: use rcu protection while reading sk->sk_user_data

[PATCH net] rxrpc: use rcu protection while reading sk->sk_user_data

[Eric Dumazet]

We need to extend the rcu_read_lock() section in rxrpc_error_report()
and use rcu_dereference_sk_user_data() instead of plain access
to sk->sk_user_data to make sure all rules are respected.

The compiler wont reload sk->sk_user_data at will, and RCU rules
prevent memory beeing freed too soon.

Fixes: f0308fb07080 ("rxrpc: Fix possible NULL pointer access in ICMP handling")
Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Howells <dhowells@redhat.com>
---
 net/rxrpc/peer_event.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/rxrpc/peer_event.c b/net/rxrpc/peer_event.c
index 61451281d74a3247ed99b160c4983bbc4a76e429..48f67a9b1037ceb7b7a749a241bbb127a12a1f67 100644
--- a/net/rxrpc/peer_event.c
+++ b/net/rxrpc/peer_event.c
@@ -147,13 +147,16 @@ void rxrpc_error_report(struct sock *sk)
 {
 	struct sock_exterr_skb *serr;
 	struct sockaddr_rxrpc srx;
-	struct rxrpc_local *local = sk->sk_user_data;
+	struct rxrpc_local *local;
 	struct rxrpc_peer *peer;
 	struct sk_buff *skb;
 
-	if (unlikely(!local))
+	rcu_read_lock();
+	local = rcu_dereference_sk_user_data(sk);
+	if (unlikely(!local)) {
+		rcu_read_unlock();
 		return;
-
+	}
 	_enter("%p{%d}", sk, local->debug_id);
 
 	/* Clear the outstanding error value on the socket so that it doesn't
@@ -163,6 +166,7 @@ void rxrpc_error_report(struct sock *sk)
 
 	skb = sock_dequeue_err_skb(sk);
 	if (!skb) {
+		rcu_read_unlock();
 		_leave("UDP socket errqueue empty");
 		return;
 	}
@@ -170,11 +174,11 @@ void rxrpc_error_report(struct sock *sk)
 	serr = SKB_EXT_ERR(skb);
 	if (!skb->len && serr->ee.ee_origin == SO_EE_ORIGIN_TIMESTAMPING) {
 		_leave("UDP empty message");
+		rcu_read_unlock();
 		rxrpc_free_skb(skb, rxrpc_skb_freed);
 		return;
 	}
 
-	rcu_read_lock();
 	peer = rxrpc_lookup_peer_icmp_rcu(local, skb, &srx);
 	if (peer && !rxrpc_get_peer_maybe(peer))
 		peer = NULL;
-- 
2.23.0.700.g56cf767bdb-goog

Re: [PATCH net] rxrpc: use rcu protection while reading sk->sk_user_data

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 14 Oct 2019 06:04:38 -0700

> We need to extend the rcu_read_lock() section in rxrpc_error_report()
> and use rcu_dereference_sk_user_data() instead of plain access
> to sk->sk_user_data to make sure all rules are respected.
> 
> The compiler wont reload sk->sk_user_data at will, and RCU rules
> prevent memory beeing freed too soon.
> 
> Fixes: f0308fb07080 ("rxrpc: Fix possible NULL pointer access in ICMP handling")
> Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Howells <dhowells@redhat.com>

David, please review.

Re: [PATCH net] rxrpc: use rcu protection while reading sk->sk_user_data

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Mon, 14 Oct 2019 06:04:38 -0700

> We need to extend the rcu_read_lock() section in rxrpc_error_report()
> and use rcu_dereference_sk_user_data() instead of plain access
> to sk->sk_user_data to make sure all rules are respected.
> 
> The compiler wont reload sk->sk_user_data at will, and RCU rules
> prevent memory beeing freed too soon.
> 
> Fixes: f0308fb07080 ("rxrpc: Fix possible NULL pointer access in ICMP handling")
> Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: David Howells <dhowells@redhat.com>

Applied, thanks Eric.

Re: [PATCH net] rxrpc: use rcu protection while reading sk->sk_user_data

[David Howells]

Eric Dumazet <edumazet@google.com> wrote:

> We need to extend the rcu_read_lock() section in rxrpc_error_report()
> and use rcu_dereference_sk_user_data() instead of plain access
> to sk->sk_user_data to make sure all rules are respected.

Should I take it that the caller won't be guaranteed to be holding the RCU
read lock?

Looking at __udp4_lib_err(), that calls __udp4_lib_err_encap(), which calls
__udp4_lib_err_encap_no_sk(), which should throw a warning if the RCU read
lock is not held.

Similarly, icmp_socket_deliver() and icmpv6_notify() should also throw a
warning before calling ->err_handler().

Does that mean something further up the CPU stack is going to be holding the
RCU read lock?

David

Re: [PATCH net] rxrpc: use rcu protection while reading sk->sk_user_data

[Eric Dumazet]

On Wed, Oct 16, 2019 at 4:24 PM David Howells <dhowells@redhat.com> wrote:
>
> Eric Dumazet <edumazet@google.com> wrote:
>
> > We need to extend the rcu_read_lock() section in rxrpc_error_report()
> > and use rcu_dereference_sk_user_data() instead of plain access
> > to sk->sk_user_data to make sure all rules are respected.
>
> Should I take it that the caller won't be guaranteed to be holding the RCU
> read lock?
>
> Looking at __udp4_lib_err(), that calls __udp4_lib_err_encap(), which calls
> __udp4_lib_err_encap_no_sk(), which should throw a warning if the RCU read
> lock is not held.
>
> Similarly, icmp_socket_deliver() and icmpv6_notify() should also throw a
> warning before calling ->err_handler().
>
> Does that mean something further up the CPU stack is going to be holding the
> RCU read lock?

Note  that before my patch, the code had a rcu_read_lock()/rcu_read_unlock(),
so I only extended it.

I am not sure that all callers already have rcu_read_lock()  held, I
prefer leaving this matter for net-next

>
> David