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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190716054624.ea6sbbzn62grde2n@kafai-mbp>
Date:   Tue, 16 Jul 2019 05:46:26 +0000
From:   Martin Lau <kafai@...com>
To:     Stanislav Fomichev <sdf@...ichev.me>
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 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.