From: Stanislav Fomichev <sdf@fomichev.me>
To: Martin Lau <kafai@fb.com>
Cc: "bpf@vger.kernel.org" <bpf@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Alexei Starovoitov <ast@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
David Miller <davem@davemloft.net>,
Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH bpf] bpf: net: Set sk_bpf_storage back to NULL for cloned sk
Date: Tue, 16 Jul 2019 10:32:44 -0700 [thread overview]
Message-ID: <20190716173244.GA14834@mini-arch> (raw)
In-Reply-To: <20190716054624.ea6sbbzn62grde2n@kafai-mbp>
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.
prev parent reply other threads:[~2019-07-16 17:32 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190716173244.GA14834@mini-arch \
--to=sdf@fomichev.me \
--cc=Kernel-team@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=kafai@fb.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).