* [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
@ 2019-06-11 21:45 Martin KaFai Lau
2019-06-12 4:46 ` Andrii Nakryiko
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Martin KaFai Lau @ 2019-06-11 21:45 UTC (permalink / raw)
To: bpf, netdev
Cc: Alexei Starovoitov, Daniel Borkmann, David Miller, kernel-team
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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
2019-06-11 21:45 [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk Martin KaFai Lau
@ 2019-06-12 4:46 ` Andrii Nakryiko
2019-06-12 14:48 ` Daniel Borkmann
2019-07-09 16:33 ` Stanislav Fomichev
2 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2019-06-12 4:46 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, Networking, Alexei Starovoitov, Daniel Borkmann,
David Miller, Kernel Team
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
2019-06-11 21:45 [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk Martin KaFai Lau
2019-06-12 4:46 ` Andrii Nakryiko
@ 2019-06-12 14:48 ` Daniel Borkmann
2019-07-09 16:33 ` Stanislav Fomichev
2 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2019-06-12 14:48 UTC (permalink / raw)
To: Martin KaFai Lau, bpf, netdev
Cc: Alexei Starovoitov, David Miller, kernel-team
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
2019-06-11 21:45 [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk Martin KaFai Lau
2019-06-12 4:46 ` Andrii Nakryiko
2019-06-12 14:48 ` Daniel Borkmann
@ 2019-07-09 16:33 ` Stanislav Fomichev
2019-07-16 5:46 ` Martin Lau
2 siblings, 1 reply; 6+ messages in thread
From: Stanislav Fomichev @ 2019-07-09 16:33 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann, David Miller,
kernel-team
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
2019-07-09 16:33 ` Stanislav Fomichev
@ 2019-07-16 5:46 ` Martin Lau
2019-07-16 17:32 ` Stanislav Fomichev
0 siblings, 1 reply; 6+ messages in thread
From: Martin Lau @ 2019-07-16 5:46 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
2019-07-16 5:46 ` Martin Lau
@ 2019-07-16 17:32 ` Stanislav Fomichev
0 siblings, 0 replies; 6+ messages in thread
From: Stanislav Fomichev @ 2019-07-16 17:32 UTC (permalink / raw)
To: Martin Lau
Cc: bpf, netdev, Alexei Starovoitov, Daniel Borkmann, David Miller,
Kernel Team
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-16 17:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-11 21:45 [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk Martin KaFai Lau
2019-06-12 4:46 ` Andrii Nakryiko
2019-06-12 14:48 ` Daniel Borkmann
2019-07-09 16:33 ` Stanislav Fomichev
2019-07-16 5:46 ` Martin Lau
2019-07-16 17:32 ` Stanislav Fomichev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).