Subject: Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk

From: Stanislav Fomichev <[email-masked]>
Date: Tue, 16 Jul 2019 10:32:44 -0700

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.