* [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array @ 2020-07-09 6:10 Martin KaFai Lau 2020-07-09 6:11 ` [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone Martin KaFai Lau ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Martin KaFai Lau @ 2020-07-09 6:10 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev This set fixes two issues on sk_user_data when a sk is added to a reuseport_array. The first patch is to avoid the sk_user_data being copied to a cloned sk. The second patch avoids doing bpf_sk_reuseport_detach() on sk_user_data that is not managed by reuseport_array. Since the changes are mostly related to bpf reuseport_array, so it is currently tagged as bpf fixes. v2: - Avoid ~3UL (Andrii) Martin KaFai Lau (2): bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone bpf: net: Avoid incorrect bpf_sk_reuseport_detach call include/net/sock.h | 3 ++- kernel/bpf/reuseport_array.c | 14 ++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) -- 2.24.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone 2020-07-09 6:10 [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Martin KaFai Lau @ 2020-07-09 6:11 ` Martin KaFai Lau 2020-07-09 16:46 ` Jakub Sitnicki 2020-07-09 20:07 ` Daniel Borkmann 2020-07-09 6:11 ` [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call Martin KaFai Lau 2020-07-09 20:07 ` [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Daniel Borkmann 2 siblings, 2 replies; 9+ messages in thread From: Martin KaFai Lau @ 2020-07-09 6:11 UTC (permalink / raw) To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev It makes little sense for copying sk_user_data of reuseport_array during sk_clone_lock(). This patch reuses the SK_USER_DATA_NOCOPY bit introduced in commit f1ff5ce2cd5e ("net, sk_msg: Clear sk_user_data pointer on clone if tagged"). It is used to mark the sk_user_data is not supposed to be copied to its clone. Although the cloned sk's sk_user_data will not be used/freed in bpf_sk_reuseport_detach(), this change can still allow the cloned sk's sk_user_data to be used by some other means. Freeing the reuseport_array's sk_user_data does not require a rcu grace period. Thus, the existing rcu_assign_sk_user_data_nocopy() is not used. Fixes: 5dc4c4b7d4e8 ("bpf: Introduce BPF_MAP_TYPE_REUSEPORT_SOCKARRAY") Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- kernel/bpf/reuseport_array.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c index 21cde24386db..a95bc8d7e812 100644 --- a/kernel/bpf/reuseport_array.c +++ b/kernel/bpf/reuseport_array.c @@ -20,11 +20,14 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map) /* The caller must hold the reuseport_lock */ void bpf_sk_reuseport_detach(struct sock *sk) { - struct sock __rcu **socks; + uintptr_t sk_user_data; write_lock_bh(&sk->sk_callback_lock); - socks = sk->sk_user_data; - if (socks) { + sk_user_data = (uintptr_t)sk->sk_user_data; + if (sk_user_data) { + struct sock __rcu **socks; + + socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK); WRITE_ONCE(sk->sk_user_data, NULL); /* * Do not move this NULL assignment outside of @@ -252,6 +255,7 @@ int bpf_fd_reuseport_array_update_elem(struct bpf_map *map, void *key, struct sock *free_osk = NULL, *osk, *nsk; struct sock_reuseport *reuse; u32 index = *(u32 *)key; + uintptr_t sk_user_data; struct socket *socket; int err, fd; @@ -305,7 +309,8 @@ int bpf_fd_reuseport_array_update_elem(struct bpf_map *map, void *key, if (err) goto put_file_unlock; - WRITE_ONCE(nsk->sk_user_data, &array->ptrs[index]); + sk_user_data = (uintptr_t)&array->ptrs[index] | SK_USER_DATA_NOCOPY; + WRITE_ONCE(nsk->sk_user_data, (void *)sk_user_data); rcu_assign_pointer(array->ptrs[index], nsk); free_osk = osk; err = 0; -- 2.24.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone 2020-07-09 6:11 ` [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone Martin KaFai Lau @ 2020-07-09 16:46 ` Jakub Sitnicki 2020-07-09 20:07 ` Daniel Borkmann 1 sibling, 0 replies; 9+ messages in thread From: Jakub Sitnicki @ 2020-07-09 16:46 UTC (permalink / raw) To: Martin KaFai Lau Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev, bpf On Thu, 09 Jul 2020 06:11:04 +0000 Martin KaFai Lau <kafai@fb.com> wrote: > It makes little sense for copying sk_user_data of reuseport_array during > sk_clone_lock(). This patch reuses the SK_USER_DATA_NOCOPY bit introduced in > commit f1ff5ce2cd5e ("net, sk_msg: Clear sk_user_data pointer on clone if tagged"). > It is used to mark the sk_user_data is not supposed to be copied to its clone. > > Although the cloned sk's sk_user_data will not be used/freed in > bpf_sk_reuseport_detach(), this change can still allow the cloned > sk's sk_user_data to be used by some other means. > > Freeing the reuseport_array's sk_user_data does not require a rcu grace > period. Thus, the existing rcu_assign_sk_user_data_nocopy() is not > used. > > Fixes: 5dc4c4b7d4e8 ("bpf: Introduce BPF_MAP_TYPE_REUSEPORT_SOCKARRAY") > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > kernel/bpf/reuseport_array.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c > index 21cde24386db..a95bc8d7e812 100644 > --- a/kernel/bpf/reuseport_array.c > +++ b/kernel/bpf/reuseport_array.c > @@ -20,11 +20,14 @@ static struct reuseport_array *reuseport_array(struct bpf_map *map) > /* The caller must hold the reuseport_lock */ > void bpf_sk_reuseport_detach(struct sock *sk) > { > - struct sock __rcu **socks; > + uintptr_t sk_user_data; > > write_lock_bh(&sk->sk_callback_lock); > - socks = sk->sk_user_data; > - if (socks) { > + sk_user_data = (uintptr_t)sk->sk_user_data; > + if (sk_user_data) { > + struct sock __rcu **socks; > + > + socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK); > WRITE_ONCE(sk->sk_user_data, NULL); > /* > * Do not move this NULL assignment outside of > @@ -252,6 +255,7 @@ int bpf_fd_reuseport_array_update_elem(struct bpf_map *map, void *key, > struct sock *free_osk = NULL, *osk, *nsk; > struct sock_reuseport *reuse; > u32 index = *(u32 *)key; > + uintptr_t sk_user_data; > struct socket *socket; > int err, fd; > > @@ -305,7 +309,8 @@ int bpf_fd_reuseport_array_update_elem(struct bpf_map *map, void *key, > if (err) > goto put_file_unlock; > > - WRITE_ONCE(nsk->sk_user_data, &array->ptrs[index]); > + sk_user_data = (uintptr_t)&array->ptrs[index] | SK_USER_DATA_NOCOPY; > + WRITE_ONCE(nsk->sk_user_data, (void *)sk_user_data); > rcu_assign_pointer(array->ptrs[index], nsk); > free_osk = osk; > err = 0; Thanks for fixing this before I got around to it. Now we can use reuseport with sockmap splicing :-) Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone 2020-07-09 6:11 ` [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone Martin KaFai Lau 2020-07-09 16:46 ` Jakub Sitnicki @ 2020-07-09 20:07 ` Daniel Borkmann 2020-07-09 21:27 ` Martin KaFai Lau 1 sibling, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2020-07-09 20:07 UTC (permalink / raw) To: Martin KaFai Lau, bpf; +Cc: Alexei Starovoitov, kernel-team, netdev On 7/9/20 8:11 AM, Martin KaFai Lau wrote: > It makes little sense for copying sk_user_data of reuseport_array during > sk_clone_lock(). This patch reuses the SK_USER_DATA_NOCOPY bit introduced in > commit f1ff5ce2cd5e ("net, sk_msg: Clear sk_user_data pointer on clone if tagged"). > It is used to mark the sk_user_data is not supposed to be copied to its clone. > > Although the cloned sk's sk_user_data will not be used/freed in > bpf_sk_reuseport_detach(), this change can still allow the cloned > sk's sk_user_data to be used by some other means. > > Freeing the reuseport_array's sk_user_data does not require a rcu grace > period. Thus, the existing rcu_assign_sk_user_data_nocopy() is not > used. nit: Would have been nice though to add a nonrcu API for this nevertheless instead of open coding. > Fixes: 5dc4c4b7d4e8 ("bpf: Introduce BPF_MAP_TYPE_REUSEPORT_SOCKARRAY") > Signed-off-by: Martin KaFai Lau <kafai@fb.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone 2020-07-09 20:07 ` Daniel Borkmann @ 2020-07-09 21:27 ` Martin KaFai Lau 0 siblings, 0 replies; 9+ messages in thread From: Martin KaFai Lau @ 2020-07-09 21:27 UTC (permalink / raw) To: Daniel Borkmann; +Cc: bpf, Alexei Starovoitov, kernel-team, netdev On Thu, Jul 09, 2020 at 10:07:59PM +0200, Daniel Borkmann wrote: > On 7/9/20 8:11 AM, Martin KaFai Lau wrote: > > It makes little sense for copying sk_user_data of reuseport_array during > > sk_clone_lock(). This patch reuses the SK_USER_DATA_NOCOPY bit introduced in > > commit f1ff5ce2cd5e ("net, sk_msg: Clear sk_user_data pointer on clone if tagged"). > > It is used to mark the sk_user_data is not supposed to be copied to its clone. > > > > Although the cloned sk's sk_user_data will not be used/freed in > > bpf_sk_reuseport_detach(), this change can still allow the cloned > > sk's sk_user_data to be used by some other means. > > > > Freeing the reuseport_array's sk_user_data does not require a rcu grace > > period. Thus, the existing rcu_assign_sk_user_data_nocopy() is not > > used. > > nit: Would have been nice though to add a nonrcu API for this nevertheless > instead of open coding. Agreed. I will create a follow-up patch to bpf-next later. I had created (READ|WRITE)_ONCE_SK_USER_DATA() earlier but then noticed there is no use on the READ_ONCE in that particular call, so ditched the idea. I think it does not matter much and should just use READ_ONCE also. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call 2020-07-09 6:10 [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Martin KaFai Lau 2020-07-09 6:11 ` [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone Martin KaFai Lau @ 2020-07-09 6:11 ` Martin KaFai Lau 2020-07-09 10:58 ` James Chapman 2020-07-09 20:07 ` [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Daniel Borkmann 2 siblings, 1 reply; 9+ messages in thread From: Martin KaFai Lau @ 2020-07-09 6:11 UTC (permalink / raw) To: bpf Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev, James Chapman bpf_sk_reuseport_detach is currently called when sk->sk_user_data is not NULL. It is incorrect because sk->sk_user_data may not be managed by the bpf's reuseport_array. It has been reported in [1] that, the bpf_sk_reuseport_detach() which is called from udp_lib_unhash() has corrupted the sk_user_data managed by l2tp. This patch solves it by using another bit (defined as SK_USER_DATA_BPF) of the sk_user_data pointer value. It marks that a sk_user_data is managed/owned by BPF. The patch depends on a PTRMASK introduced in commit f1ff5ce2cd5e ("net, sk_msg: Clear sk_user_data pointer on clone if tagged"). [ Note: sk->sk_user_data is used by bpf's reuseport_array only when a sk is added to the bpf's reuseport_array. i.e. doing setsockopt(SO_REUSEPORT) and having "sk->sk_reuseport == 1" alone will not stop sk->sk_user_data being used by other means. ] [1]: https://lore.kernel.org/netdev/20200706121259.GA20199@katalix.com/ Reported-by: James Chapman <jchapman@katalix.com> Cc: James Chapman <jchapman@katalix.com> Fixes: 5dc4c4b7d4e8 ("bpf: Introduce BPF_MAP_TYPE_REUSEPORT_SOCKARRAY") Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- include/net/sock.h | 3 ++- kernel/bpf/reuseport_array.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/net/sock.h b/include/net/sock.h index 3428619faae4..1183507df95b 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -533,7 +533,8 @@ enum sk_pacing { * be copied. */ #define SK_USER_DATA_NOCOPY 1UL -#define SK_USER_DATA_PTRMASK ~(SK_USER_DATA_NOCOPY) +#define SK_USER_DATA_BPF 2UL /* Managed by BPF */ +#define SK_USER_DATA_PTRMASK ~(SK_USER_DATA_NOCOPY | SK_USER_DATA_BPF) /** * sk_user_data_is_nocopy - Test if sk_user_data pointer must not be copied diff --git a/kernel/bpf/reuseport_array.c b/kernel/bpf/reuseport_array.c index a95bc8d7e812..cae9d505e04a 100644 --- a/kernel/bpf/reuseport_array.c +++ b/kernel/bpf/reuseport_array.c @@ -24,7 +24,7 @@ void bpf_sk_reuseport_detach(struct sock *sk) write_lock_bh(&sk->sk_callback_lock); sk_user_data = (uintptr_t)sk->sk_user_data; - if (sk_user_data) { + if (sk_user_data & SK_USER_DATA_BPF) { struct sock __rcu **socks; socks = (void *)(sk_user_data & SK_USER_DATA_PTRMASK); @@ -309,7 +309,8 @@ int bpf_fd_reuseport_array_update_elem(struct bpf_map *map, void *key, if (err) goto put_file_unlock; - sk_user_data = (uintptr_t)&array->ptrs[index] | SK_USER_DATA_NOCOPY; + sk_user_data = (uintptr_t)&array->ptrs[index] | SK_USER_DATA_NOCOPY | + SK_USER_DATA_BPF; WRITE_ONCE(nsk->sk_user_data, (void *)sk_user_data); rcu_assign_pointer(array->ptrs[index], nsk); free_osk = osk; -- 2.24.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call 2020-07-09 6:11 ` [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call Martin KaFai Lau @ 2020-07-09 10:58 ` James Chapman 2020-07-09 18:47 ` Martin KaFai Lau 0 siblings, 1 reply; 9+ messages in thread From: James Chapman @ 2020-07-09 10:58 UTC (permalink / raw) To: Martin KaFai Lau Cc: bpf, Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev On Wed, Jul 08, 2020 at 23:11:10 -0700, Martin KaFai Lau wrote: > bpf_sk_reuseport_detach is currently called when sk->sk_user_data > is not NULL. It is incorrect because sk->sk_user_data may not be > managed by the bpf's reuseport_array. It has been reported in [1] that, > the bpf_sk_reuseport_detach() which is called from udp_lib_unhash() has > corrupted the sk_user_data managed by l2tp. > > This patch solves it by using another bit (defined as SK_USER_DATA_BPF) > of the sk_user_data pointer value. It marks that a sk_user_data is > managed/owned by BPF. I have reservations about using a bit in sk_user_data to indicate ownership of that pointer. But putting that aside, I confirm that the patch fixes the problem. Acked-by: James Chapman <jchapman@katalix.com> Tested-by: James Chapman <jchapman@katalix.com> Reported-by: syzbot+9f092552ba9a5efca5df@syzkaller.appspotmail.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call 2020-07-09 10:58 ` James Chapman @ 2020-07-09 18:47 ` Martin KaFai Lau 0 siblings, 0 replies; 9+ messages in thread From: Martin KaFai Lau @ 2020-07-09 18:47 UTC (permalink / raw) To: James Chapman Cc: bpf, Alexei Starovoitov, Daniel Borkmann, kernel-team, netdev On Thu, Jul 09, 2020 at 11:58:33AM +0100, James Chapman wrote: > On Wed, Jul 08, 2020 at 23:11:10 -0700, Martin KaFai Lau wrote: > > bpf_sk_reuseport_detach is currently called when sk->sk_user_data > > is not NULL. It is incorrect because sk->sk_user_data may not be > > managed by the bpf's reuseport_array. It has been reported in [1] that, > > the bpf_sk_reuseport_detach() which is called from udp_lib_unhash() has > > corrupted the sk_user_data managed by l2tp. > > > > This patch solves it by using another bit (defined as SK_USER_DATA_BPF) > > of the sk_user_data pointer value. It marks that a sk_user_data is > > managed/owned by BPF. > > I have reservations about using a bit in sk_user_data to indicate > ownership of that pointer. But putting that aside, I confirm that the > patch fixes the problem. > > Acked-by: James Chapman <jchapman@katalix.com> > Tested-by: James Chapman <jchapman@katalix.com> > Reported-by: syzbot+9f092552ba9a5efca5df@syzkaller.appspotmail.com Thanks for the test! One bit of sk_user_data has already been used to indicate SK_USER_DATA_NOCOPY. I think using another bit is the cleanest fix for the bpf/net branch instead of tracking this somewhere else. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array 2020-07-09 6:10 [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Martin KaFai Lau 2020-07-09 6:11 ` [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone Martin KaFai Lau 2020-07-09 6:11 ` [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call Martin KaFai Lau @ 2020-07-09 20:07 ` Daniel Borkmann 2 siblings, 0 replies; 9+ messages in thread From: Daniel Borkmann @ 2020-07-09 20:07 UTC (permalink / raw) To: Martin KaFai Lau, bpf; +Cc: Alexei Starovoitov, kernel-team, netdev On 7/9/20 8:10 AM, Martin KaFai Lau wrote: > This set fixes two issues on sk_user_data when a sk is added to > a reuseport_array. > > The first patch is to avoid the sk_user_data being copied > to a cloned sk. The second patch avoids doing bpf_sk_reuseport_detach() > on sk_user_data that is not managed by reuseport_array. > > Since the changes are mostly related to bpf reuseport_array, so it is > currently tagged as bpf fixes. > > v2: > - Avoid ~3UL (Andrii) > > Martin KaFai Lau (2): > bpf: net: Avoid copying sk_user_data of reuseport_array during > sk_clone > bpf: net: Avoid incorrect bpf_sk_reuseport_detach call > > include/net/sock.h | 3 ++- > kernel/bpf/reuseport_array.c | 14 ++++++++++---- > 2 files changed, 12 insertions(+), 5 deletions(-) > Applied, thanks! ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-09 21:27 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-09 6:10 [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Martin KaFai Lau 2020-07-09 6:11 ` [PATCH v2 bpf 1/2] bpf: net: Avoid copying sk_user_data of reuseport_array during sk_clone Martin KaFai Lau 2020-07-09 16:46 ` Jakub Sitnicki 2020-07-09 20:07 ` Daniel Borkmann 2020-07-09 21:27 ` Martin KaFai Lau 2020-07-09 6:11 ` [PATCH v2 bpf 2/2] bpf: net: Avoid incorrect bpf_sk_reuseport_detach call Martin KaFai Lau 2020-07-09 10:58 ` James Chapman 2020-07-09 18:47 ` Martin KaFai Lau 2020-07-09 20:07 ` [PATCH v2 bpf 0/2] bpf: net: Fixes in sk_user_data of reuseport_array Daniel Borkmann
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.