Message ID | 20200709061104.4018798-1-kafai@fb.com |
---|---|
State | Accepted |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: net: Fixes in sk_user_data of reuseport_array | expand |
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>
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>
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.
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;
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(-)