From: Florian Westphal <fw@strlen.de>
To: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Florian Westphal <fw@strlen.de>,
Lorenzo Bianconi <lorenzo@kernel.org>,
bpf@vger.kernel.org, netdev@vger.kernel.org, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, davem@davemloft.net,
kuba@kernel.org, edumazet@google.com, pabeni@redhat.com,
pablo@netfilter.org, netfilter-devel@vger.kernel.org,
lorenzo.bianconi@redhat.com, brouer@redhat.com, toke@redhat.com,
yhs@fb.com
Subject: Re: [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE
Date: Fri, 27 May 2022 14:02:17 +0200 [thread overview]
Message-ID: <20220527120217.GG7680@breakpoint.cc> (raw)
In-Reply-To: <20220527113343.h3q5zmkmqm7fev7r@apollo.legion>
Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> On Fri, May 27, 2022 at 03:15:58AM IST, Florian Westphal wrote:
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> > > From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > >
> > > Since we want to allow user to set some fields in nf_conn after it is
> > > allocated but before it is inserted, we can permit BPF_WRITE for normal
> > > nf_conn, and then mark return value as read only on insert, preventing
> > > further BPF_WRITE. This way, nf_conn can be written to using normal
> > > BPF instructions after allocation, but not after insertion.
> > >
> > > Note that we special nf_conn a bit here, inside the btf_struct_access
> > > callback for XDP and TC programs. Since this is the only struct for
> > > these programs requiring such adjustments, making this mechanism
> > > more generic has been left as an exercise for a future patch adding
> > > custom callbacks for more structs.
> >
> > Are you sure this is safe?
> > As far as I can see this allows nf_conn->status = ~0ul.
> > I'm fairly sure this isn't a good idea, see nf_ct_delete() for example.
>
> This only allows writing to an allocated but not yet inserted nf_conn. The idea
> was that insert checks whether ct->status only has permitted bits set before
> making the entry visible, and then we make nf_conn pointer read only, however
> the runtime check seems to be missing right now in patch 12; something to fix in
> v5. With that sorted, would it be fine?
Its fragile, e.g. what if I set TEMPLATE bit? If refcount goes down to
0, object is released via kfree() instead of kmem_cache_free.
What if I clear SNAT_DONE bit? Would it leave the (freed) entry on the
bysource hash list (see nf_nat_core.c)?
Or is there some magic that prevents this from happening? I have no
idea how processing pipeline looks like...
next prev parent reply other threads:[~2022-05-27 12:22 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-26 21:34 [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 01/14] bpf: Add support for forcing kfunc args to be referenced Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 02/14] bpf: Print multiple type flags in verifier log Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 03/14] bpf: Support rdonly PTR_TO_BTF_ID for pointer to const return value Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 04/14] bpf: Support storing rdonly PTR_TO_BTF_ID in BPF maps Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 05/14] bpf: Support passing rdonly PTR_TO_BTF_ID to kfunc Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 06/14] bpf: Whitelist some fields in nf_conn for BPF_WRITE Lorenzo Bianconi
2022-05-26 21:45 ` Florian Westphal
2022-05-27 11:36 ` Kumar Kartikeya Dwivedi
2022-05-27 12:02 ` Florian Westphal [this message]
2022-05-26 23:55 ` kernel test robot
2022-05-27 7:34 ` kernel test robot
2022-05-27 9:17 ` kernel test robot
2022-05-26 21:34 ` [PATCH v4 bpf-next 07/14] bpf: Define acquire-release pairs for kfuncs Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 08/14] selftests/bpf: Add verifier tests for forced kfunc ref args Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 09/14] selftests/bpf: Add C tests for rdonly PTR_TO_BTF_ID Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 10/14] selftests/bpf: Add verifier " Lorenzo Bianconi
2022-05-26 21:34 ` [PATCH v4 bpf-next 11/14] net: netfilter: add kfunc helper to update ct timeout Lorenzo Bianconi
2022-05-26 21:35 ` [PATCH v4 bpf-next 12/14] net: netfilter: add kfunc helpers to alloc and insert a new ct entry Lorenzo Bianconi
2022-05-26 21:35 ` [PATCH v4 bpf-next 13/14] selftests/bpf: add selftest for bpf_xdp_ct_add and bpf_ct_refresh_timeout kfunc Lorenzo Bianconi
2022-05-26 21:35 ` [PATCH v4 bpf-next 14/14] selftests/bpf: Add negative tests for bpf_nf Lorenzo Bianconi
2022-06-11 20:11 ` [PATCH v4 bpf-next 00/14] net: netfilter: add kfunc helper to update ct timeout Alexei Starovoitov
2022-06-13 16:14 ` Kumar Kartikeya Dwivedi
2022-06-13 22:15 ` Alexei Starovoitov
2022-06-14 2:23 ` Kumar Kartikeya Dwivedi
2022-06-17 20:45 ` Alexei Starovoitov
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=20220527120217.GG7680@breakpoint.cc \
--to=fw@strlen.de \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brouer@redhat.com \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kuba@kernel.org \
--cc=lorenzo.bianconi@redhat.com \
--cc=lorenzo@kernel.org \
--cc=memxor@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=netfilter-devel@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pablo@netfilter.org \
--cc=toke@redhat.com \
--cc=yhs@fb.com \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.