| Message ID | 20190611214557.2700117-1-kafai@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk | expand |
On Tue, Jun 11, 2019 at 8:33 PM Martin KaFai Lau <kafai@fb.com> wrote: > > The cloned sk should not carry its parent-listener's sk_bpf_storage. > This patch fixes it by setting it back to NULL. Makes sense. Acked-by: Andrii Nakryiko <andriin@fb.com> > > Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > net/core/sock.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 2b3701958486..d90fd04622e5 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1850,6 +1850,9 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > goto out; > } > RCU_INIT_POINTER(newsk->sk_reuseport_cb, NULL); > +#ifdef CONFIG_BPF_SYSCALL > + RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL); > +#endif > > newsk->sk_err = 0; > newsk->sk_err_soft = 0; > -- > 2.17.1 >
On 06/11/2019 11:45 PM, Martin KaFai Lau wrote: > The cloned sk should not carry its parent-listener's sk_bpf_storage. > This patch fixes it by setting it back to NULL. > > Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") > Signed-off-by: Martin KaFai Lau <kafai@fb.com> Applied, thanks!
On 06/11, Martin KaFai Lau wrote: > The cloned sk should not carry its parent-listener's sk_bpf_storage. > This patch fixes it by setting it back to NULL. Have you thought about some kind of inheritance for listener sockets' storage? Suppose I have a situation where I write something to listener's sk storage (directly or via recently added sockopts hooks) and I want to inherit that state for a freshly established connection. I was looking into adding possibility to call bpf_get_listener_sock form BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB callback to manually copy some data form the listener socket, but I don't think at this point there is any association between newly established socket and the listener. Thoughts/ideas? (Btw, sorry for digging up this old mail, but it feels relevant). > Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > net/core/sock.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 2b3701958486..d90fd04622e5 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1850,6 +1850,9 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) > goto out; > } > RCU_INIT_POINTER(newsk->sk_reuseport_cb, NULL); > +#ifdef CONFIG_BPF_SYSCALL > + RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL); > +#endif > > newsk->sk_err = 0; > newsk->sk_err_soft = 0; > -- > 2.17.1 >
On Tue, Jul 09, 2019 at 09:33:21AM -0700, Stanislav Fomichev wrote: > On 06/11, Martin KaFai Lau wrote: > > The cloned sk should not carry its parent-listener's sk_bpf_storage. > > This patch fixes it by setting it back to NULL. > Have you thought about some kind of inheritance for listener sockets' > storage? Suppose I have a situation where I write something > to listener's sk storage (directly or via recently added sockopts hooks) > and I want to inherit that state for a freshly established connection. > > I was looking into adding possibility to call bpf_get_listener_sock form > BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB callback to manually > copy some data form the listener socket, but I don't think > at this point there is any association between newly established > socket and the listener. Right, at that point, the child sk has no reference back to the listener's sk. After a quick look, the listener sk may not always be available also (e.g. the backlog processing case). Hence, adding the listener sk to the bpf running ctx is not obvious either. > > Thoughts/ideas? I think cloning the listener's bpf sk storage could be added to the existing sk cloning logic. It seems to be a more straight forward approach instead of figuring out the right place to call another bpf prog to clone it. Quick thoughts out of my head: 1. Default should be not-to-clone. Have a way (a map's flag?) to opt-in. 2. The listener's sk storage could be being modified while being cloned. One possibility is to check if the value has bpf_spin_lock. If there is, lock it before cloning.
On 07/16, Martin Lau wrote: > On Tue, Jul 09, 2019 at 09:33:21AM -0700, Stanislav Fomichev wrote: > > On 06/11, Martin KaFai Lau wrote: > > > The cloned sk should not carry its parent-listener's sk_bpf_storage. > > > This patch fixes it by setting it back to NULL. > > Have you thought about some kind of inheritance for listener sockets' > > storage? Suppose I have a situation where I write something > > to listener's sk storage (directly or via recently added sockopts hooks) > > and I want to inherit that state for a freshly established connection. > > > > I was looking into adding possibility to call bpf_get_listener_sock form > > BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB callback to manually > > copy some data form the listener socket, but I don't think > > at this point there is any association between newly established > > socket and the listener. > Right, at that point, the child sk has no reference back > to the listener's sk. > > After a quick look, the listener sk may not always be available > also (e.g. the backlog processing case). Hence, adding > the listener sk to the bpf running ctx is not obvious > either. > > > > > Thoughts/ideas? > I think cloning the listener's bpf sk storage could be added > to the existing sk cloning logic. It seems to be a more straight > forward approach instead of figuring out the right place to call > another bpf prog to clone it. > > Quick thoughts out of my head: > 1. Default should be not-to-clone. Have a way (a map's flag?) to opt-in. > 2. The listener's sk storage could be being modified while being cloned. > One possibility is to check if the value has bpf_spin_lock. > If there is, lock it before cloning. Thanks for suggestion! An optional inherit/clone flag to bpf_sk_storage_get seems like a good option. I'll try to play with it, will probably get back with an rfc at some point.
diff --git a/net/core/sock.c b/net/core/sock.c index 2b3701958486..d90fd04622e5 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1850,6 +1850,9 @@ struct sock *sk_clone_lock(const struct sock *sk, const gfp_t priority) goto out; } RCU_INIT_POINTER(newsk->sk_reuseport_cb, NULL); +#ifdef CONFIG_BPF_SYSCALL + RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL); +#endif newsk->sk_err = 0; newsk->sk_err_soft = 0;
The cloned sk should not carry its parent-listener's sk_bpf_storage. This patch fixes it by setting it back to NULL. Fixes: 6ac99e8f23d4 ("bpf: Introduce bpf sk local storage") Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- net/core/sock.c | 3 +++ 1 file changed, 3 insertions(+)