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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190716173244.GA14834@mini-arch>
Date:   Tue, 16 Jul 2019 10:32:44 -0700
From:   Stanislav Fomichev <sdf@...ichev.me>
To:     Martin Lau <kafai@...com>
Cc:     "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        David Miller <davem@...emloft.net>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned
 sk

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.