* [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()
@ 2019-04-23 16:43 Eric Dumazet
2019-04-24 9:58 ` Guillaume Nault
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-04-23 16:43 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, James Chapman
Canonical way to fetch sk_user_data from an encap_rcv() handler called
from UDP stack in rcu protected section is to use rcu_dereference_sk_user_data(),
otherwise compiler might read it multiple times.
Fixes: d00fa9adc528 ("il2tp: fix races with tunnel socket close")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: James Chapman <jchapman@katalix.com>
---
net/l2tp/l2tp_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index fed6becc5daf86afa2ad9188bb28e151244bb5a6..aee33d1320184e411dbedff72b5bf5199481e53f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
{
struct l2tp_tunnel *tunnel;
- tunnel = l2tp_tunnel(sk);
+ tunnel = rcu_dereference_sk_user_data(sk);
if (tunnel == NULL)
goto pass_up;
--
2.21.0.593.g511ec345e18-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()
2019-04-23 16:43 [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv() Eric Dumazet
@ 2019-04-24 9:58 ` Guillaume Nault
2019-04-24 11:33 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Guillaume Nault @ 2019-04-24 9:58 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, James Chapman
On Tue, Apr 23, 2019 at 09:43:26AM -0700, Eric Dumazet wrote:
> Canonical way to fetch sk_user_data from an encap_rcv() handler called
> from UDP stack in rcu protected section is to use rcu_dereference_sk_user_data(),
> otherwise compiler might read it multiple times.
>
That reminds me the more general problem we have with ->sk_user_data:
https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
We're not even guarateed that ->sk_user_data points to a struct l2tp_tunnel
(some external modules can still probably override it).
There were some locking rules defined for setting ->sk_user_data:
https://lore.kernel.org/netdev/20180124203541.3172-3-tom@quantonium.net/
Converting all users to either avoid using ->sk_user_data or using it
under the proper pre-conditions has been on my TODO list for a while...
I'll try to revive this effort.
> Fixes: d00fa9adc528 ("il2tp: fix races with tunnel socket close")
^
Spurious "i". Vi user? :)
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: James Chapman <jchapman@katalix.com>
> ---
> net/l2tp/l2tp_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index fed6becc5daf86afa2ad9188bb28e151244bb5a6..aee33d1320184e411dbedff72b5bf5199481e53f 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> {
> struct l2tp_tunnel *tunnel;
>
> - tunnel = l2tp_tunnel(sk);
> + tunnel = rcu_dereference_sk_user_data(sk);
> if (tunnel == NULL)
> goto pass_up;
>
> --
> 2.21.0.593.g511ec345e18-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()
2019-04-24 9:58 ` Guillaume Nault
@ 2019-04-24 11:33 ` Eric Dumazet
2019-04-24 18:21 ` Guillaume Nault
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-04-24 11:33 UTC (permalink / raw)
To: Guillaume Nault; +Cc: David S . Miller, netdev, Eric Dumazet, James Chapman
On Wed, Apr 24, 2019 at 2:58 AM Guillaume Nault <gnault@redhat.com> wrote:
>
> On Tue, Apr 23, 2019 at 09:43:26AM -0700, Eric Dumazet wrote:
> > Canonical way to fetch sk_user_data from an encap_rcv() handler called
> > from UDP stack in rcu protected section is to use rcu_dereference_sk_user_data(),
> > otherwise compiler might read it multiple times.
> >
> That reminds me the more general problem we have with ->sk_user_data:
> https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
>
> We're not even guarateed that ->sk_user_data points to a struct l2tp_tunnel
> (some external modules can still probably override it).
>
RCU rules should prevail.
An RCU grace period must be enforced if the same UDP socket has its
sk_user_data changed.
Normally, UDP socket observes an RCU grace period at close/destroy
time. (SOCK_RCU_FREE)
If we detect the same UDP socket has not been closed between clearing
sk_user_data
and setting it again to a non NULL value, we must insert a synchronize_rcu()
A generic change could look like the following (but we must check that all
rcu_assign_sk_user_data() callers can sleep)
diff --git a/include/net/sock.h b/include/net/sock.h
index 784cd19d5ff76b53865f79d4580f3fa269fa2408..3cf0dfd9d83a956220d69138339561b62407addd
100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -522,7 +522,6 @@ enum sk_pacing {
#define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))
#define rcu_dereference_sk_user_data(sk)
rcu_dereference(__sk_user_data((sk)))
-#define rcu_assign_sk_user_data(sk, ptr)
rcu_assign_pointer(__sk_user_data((sk)), ptr)
/*
* SK_CAN_REUSE and SK_NO_REUSE on a socket mean that the socket is OK
@@ -811,6 +810,7 @@ enum sock_flags {
SOCK_FILTER_LOCKED, /* Filter cannot be changed anymore */
SOCK_SELECT_ERR_QUEUE, /* Wake select on error queue */
SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */
+ SOCK_USER_DATA_SET,
SOCK_TXTIME,
SOCK_XDP, /* XDP is attached */
SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */
@@ -2582,4 +2582,15 @@ static inline bool sk_dev_equal_l3scope(struct
sock *sk, int dif)
return false;
}
+static inline void rcu_assign_sk_user_data(struct sock *sk, void *ptr)
+{
+ if (ptr) {
+ might_sleep();
+ if (sock_flag(sk, SOCK_USER_DATA_SET))
+ synchronize_net();
+ }
+ rcu_assign_pointer(__sk_user_data((sk)), ptr);
+ if (ptr)
+ sock_set_flag(sk, SOCK_USER_DATA_SET);
+}
#endif /* _SOCK_H */
> There were some locking rules defined for setting ->sk_user_data:
> https://lore.kernel.org/netdev/20180124203541.3172-3-tom@quantonium.net/
> Converting all users to either avoid using ->sk_user_data or using it
> under the proper pre-conditions has been on my TODO list for a while...
> I'll try to revive this effort.
Avoiding using sk_user_data seems not practical.
However making sure it is not blindly overwritten by a buggy module is doable.
>
> > Fixes: d00fa9adc528 ("il2tp: fix races with tunnel socket close")
> ^
> Spurious "i". Vi user? :)
I am not a vi user ;)
>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: James Chapman <jchapman@katalix.com>
> > ---
> > net/l2tp/l2tp_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > index fed6becc5daf86afa2ad9188bb28e151244bb5a6..aee33d1320184e411dbedff72b5bf5199481e53f 100644
> > --- a/net/l2tp/l2tp_core.c
> > +++ b/net/l2tp/l2tp_core.c
> > @@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > {
> > struct l2tp_tunnel *tunnel;
> >
> > - tunnel = l2tp_tunnel(sk);
> > + tunnel = rcu_dereference_sk_user_data(sk);
> > if (tunnel == NULL)
> > goto pass_up;
> >
> > --
> > 2.21.0.593.g511ec345e18-goog
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()
2019-04-24 11:33 ` Eric Dumazet
@ 2019-04-24 18:21 ` Guillaume Nault
2019-04-24 18:29 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Guillaume Nault @ 2019-04-24 18:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, James Chapman
On Wed, Apr 24, 2019 at 04:33:47AM -0700, Eric Dumazet wrote:
> On Wed, Apr 24, 2019 at 2:58 AM Guillaume Nault <gnault@redhat.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 09:43:26AM -0700, Eric Dumazet wrote:
> > > Canonical way to fetch sk_user_data from an encap_rcv() handler called
> > > from UDP stack in rcu protected section is to use rcu_dereference_sk_user_data(),
> > > otherwise compiler might read it multiple times.
> > >
> > That reminds me the more general problem we have with ->sk_user_data:
> > https://lore.kernel.org/netdev/20180117.142538.1972806008716856078.davem@davemloft.net/
> >
> > We're not even guarateed that ->sk_user_data points to a struct l2tp_tunnel
> > (some external modules can still probably override it).
> >
>
> RCU rules should prevail.
>
> An RCU grace period must be enforced if the same UDP socket has its
> sk_user_data changed.
>
> Normally, UDP socket observes an RCU grace period at close/destroy
> time. (SOCK_RCU_FREE)
>
> If we detect the same UDP socket has not been closed between clearing
> sk_user_data
> and setting it again to a non NULL value, we must insert a synchronize_rcu()
>
> A generic change could look like the following (but we must check that all
> rcu_assign_sk_user_data() callers can sleep)
>
It seems that callers of rcu_assign_sk_user_data() can indeed sleep.
But I think we have more problems. Not all code paths treat
->sk_user_data as RCU pointers (IIUC that's why we created the
__sk_user_data() macro, instead of just redefining ->sk_user_data as
"void __rcu *"). But even if RCU rules were respected for all accesses,
we'd need to ensure consistent protection for the update side.
And then, we'd need to make sure that ->sk_user_data is in sync with
the encap_rcv() callback (or whatever actually uses the data pointed
to). Otherwise a module could treat ->sk_user_data as a struct foo
pointer while it actually points to a struct bar.
For example, a quick look at net/sunrpc/svcsock.c seems to indicate
that svc_addsock() would accept any (unconnected) UDP socket and pass
it to svc_addsock(), which in turn would override ->sk_user_data with
a struct svc_sock pointer. If the socket was previously set up by L2TP,
then we'd end up with ->sk_user_data pointing to a svc_sock structure,
but ->encap_rcv still pointing to l2tp_udp_encap_recv(). That's going
to give unexpected results when l2tp_udp_encap_recv() will dereference
->sk_user_data to access (what it believes to be) its tunnel structure.
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 784cd19d5ff76b53865f79d4580f3fa269fa2408..3cf0dfd9d83a956220d69138339561b62407addd
> 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -522,7 +522,6 @@ enum sk_pacing {
> #define __sk_user_data(sk) ((*((void __rcu **)&(sk)->sk_user_data)))
>
> #define rcu_dereference_sk_user_data(sk)
> rcu_dereference(__sk_user_data((sk)))
> -#define rcu_assign_sk_user_data(sk, ptr)
> rcu_assign_pointer(__sk_user_data((sk)), ptr)
>
> /*
> * SK_CAN_REUSE and SK_NO_REUSE on a socket mean that the socket is OK
> @@ -811,6 +810,7 @@ enum sock_flags {
> SOCK_FILTER_LOCKED, /* Filter cannot be changed anymore */
> SOCK_SELECT_ERR_QUEUE, /* Wake select on error queue */
> SOCK_RCU_FREE, /* wait rcu grace period in sk_destruct() */
> + SOCK_USER_DATA_SET,
> SOCK_TXTIME,
> SOCK_XDP, /* XDP is attached */
> SOCK_TSTAMP_NEW, /* Indicates 64 bit timestamps always */
> @@ -2582,4 +2582,15 @@ static inline bool sk_dev_equal_l3scope(struct
> sock *sk, int dif)
> return false;
> }
>
> +static inline void rcu_assign_sk_user_data(struct sock *sk, void *ptr)
> +{
> + if (ptr) {
> + might_sleep();
> + if (sock_flag(sk, SOCK_USER_DATA_SET))
> + synchronize_net();
> + }
> + rcu_assign_pointer(__sk_user_data((sk)), ptr);
> + if (ptr)
> + sock_set_flag(sk, SOCK_USER_DATA_SET);
> +}
> #endif /* _SOCK_H */
>
>
>
> > There were some locking rules defined for setting ->sk_user_data:
> > https://lore.kernel.org/netdev/20180124203541.3172-3-tom@quantonium.net/
> > Converting all users to either avoid using ->sk_user_data or using it
> > under the proper pre-conditions has been on my TODO list for a while...
> > I'll try to revive this effort.
>
> Avoiding using sk_user_data seems not practical.
>
> However making sure it is not blindly overwritten by a buggy module is doable.
>
At least for those that use rcu_assign_sk_user_data(). And even then,
we'd need to ensure callers do properly validate the socket, to avoid
overriding ->sk_user_data with a different pointer type than what other
users expect.
> >
> > > Fixes: d00fa9adc528 ("il2tp: fix races with tunnel socket close")
> > ^
> > Spurious "i". Vi user? :)
>
> I am not a vi user ;)
>
So nice to hear :)
> >
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > Cc: James Chapman <jchapman@katalix.com>
> > > ---
> > > net/l2tp/l2tp_core.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> > > index fed6becc5daf86afa2ad9188bb28e151244bb5a6..aee33d1320184e411dbedff72b5bf5199481e53f 100644
> > > --- a/net/l2tp/l2tp_core.c
> > > +++ b/net/l2tp/l2tp_core.c
> > > @@ -909,7 +909,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
> > > {
> > > struct l2tp_tunnel *tunnel;
> > >
> > > - tunnel = l2tp_tunnel(sk);
> > > + tunnel = rcu_dereference_sk_user_data(sk);
> > > if (tunnel == NULL)
> > > goto pass_up;
> > >
> > > --
> > > 2.21.0.593.g511ec345e18-goog
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()
2019-04-24 18:21 ` Guillaume Nault
@ 2019-04-24 18:29 ` Eric Dumazet
2019-04-24 19:04 ` Guillaume Nault
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2019-04-24 18:29 UTC (permalink / raw)
To: Guillaume Nault; +Cc: David S . Miller, netdev, Eric Dumazet, James Chapman
On Wed, Apr 24, 2019 at 11:21 AM Guillaume Nault <gnault@redhat.com> wrote:
> It seems that callers of rcu_assign_sk_user_data() can indeed sleep.
>
> But I think we have more problems. Not all code paths treat
> ->sk_user_data as RCU pointers (IIUC that's why we created the
> __sk_user_data() macro, instead of just redefining ->sk_user_data as
> "void __rcu *"). But even if RCU rules were respected for all accesses,
> we'd need to ensure consistent protection for the update side.
Sure.
>
> And then, we'd need to make sure that ->sk_user_data is in sync with
> the encap_rcv() callback (or whatever actually uses the data pointed
> to). Otherwise a module could treat ->sk_user_data as a struct foo
> pointer while it actually points to a struct bar.
>
> For example, a quick look at net/sunrpc/svcsock.c seems to indicate
> that svc_addsock() would accept any (unconnected) UDP socket and pass
> it to svc_addsock(), which in turn would override ->sk_user_data with
> a struct svc_sock pointer. If the socket was previously set up by L2TP,
> then we'd end up with ->sk_user_data pointing to a svc_sock structure,
> but ->encap_rcv still pointing to l2tp_udp_encap_recv(). That's going
> to give unexpected results when l2tp_udp_encap_recv() will dereference
> ->sk_user_data to access (what it believes to be) its tunnel structure.
A full audit is needed, and I have started it. If you want to help
just send a patch ;)
I have looked at this l2tp code only after fixing another issue in
RXRPC, and would have
looked later at SUNRPC.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv()
2019-04-24 18:29 ` Eric Dumazet
@ 2019-04-24 19:04 ` Guillaume Nault
0 siblings, 0 replies; 6+ messages in thread
From: Guillaume Nault @ 2019-04-24 19:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, James Chapman
On Wed, Apr 24, 2019 at 11:29:25AM -0700, Eric Dumazet wrote:
> On Wed, Apr 24, 2019 at 11:21 AM Guillaume Nault <gnault@redhat.com> wrote:
>
> >
> > And then, we'd need to make sure that ->sk_user_data is in sync with
> > the encap_rcv() callback (or whatever actually uses the data pointed
> > to). Otherwise a module could treat ->sk_user_data as a struct foo
> > pointer while it actually points to a struct bar.
> >
> > For example, a quick look at net/sunrpc/svcsock.c seems to indicate
> > that svc_addsock() would accept any (unconnected) UDP socket and pass
> > it to svc_addsock(), which in turn would override ->sk_user_data with
> > a struct svc_sock pointer. If the socket was previously set up by L2TP,
> > then we'd end up with ->sk_user_data pointing to a svc_sock structure,
> > but ->encap_rcv still pointing to l2tp_udp_encap_recv(). That's going
> > to give unexpected results when l2tp_udp_encap_recv() will dereference
> > ->sk_user_data to access (what it believes to be) its tunnel structure.
>
> A full audit is needed, and I have started it. If you want to help
> just send a patch ;)
>
> I have looked at this l2tp code only after fixing another issue in
> RXRPC, and would have
> looked later at SUNRPC.
Hum, sorry, I didn't realise that. I'm really interested in the
solutions you can come up with.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-24 19:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-23 16:43 [PATCH net] l2tp: use rcu_dereference_sk_user_data() in l2tp_udp_encap_recv() Eric Dumazet
2019-04-24 9:58 ` Guillaume Nault
2019-04-24 11:33 ` Eric Dumazet
2019-04-24 18:21 ` Guillaume Nault
2019-04-24 18:29 ` Eric Dumazet
2019-04-24 19:04 ` Guillaume Nault
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.