| Message ID | 20191014130438.163688-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] rxrpc: use rcu protection while reading sk->sk_user_data | expand |
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.
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.
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
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
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;
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(-)