From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: John Fastabend <john.fastabend@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
Yonghong Song <yhs@fb.com>, Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>, bpf <bpf@vger.kernel.org>
Subject: Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null
Date: Sat, 1 Aug 2020 20:10:58 -0700 [thread overview]
Message-ID: <20200802031058.rk4rlygvbbmny3bl@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <5f262751ab97c_11b82ae318aac5b44d@john-XPS-13-9370.notmuch>
On Sat, Aug 01, 2020 at 07:39:13PM -0700, John Fastabend wrote:
> Andrii Nakryiko wrote:
> > On Fri, Jul 31, 2020 at 3:36 PM John Fastabend <john.fastabend@gmail.com> wrote:
> > >
> > > The verifier contains both types PTR_TO_BTF_ID and PTR_TO_BTF_ID_OR_NULL.
> > > For all other type pairs PTR_TO_foo and PTR_TO_foo_OR_NULL we follow the
> > > convention to use PTR_TO_foo_OR_NULL for pointers that may be null and
> > > PTR_TO_foo when the ptr value has been checked to ensure it is _not_ NULL.
> > >
> > > For PTR_TO_BTF_ID this is not the case though. It may be still be NULL
> > > even though we have the PTR_TO_BTF_ID type.
> >
> > _OR_NULL means that the verifier enforces an explicit NULL check,
> > before allowing the BPF program to dereference corresponding
> > registers. That's not the case for PTR_TO_BTF_ID, though. The BPF
> > program is allowed to assume valid pointer and proceed without checks.
> >
> > You are right that NULLs are still possible (as well as just invalid
> > pointers), but BPF JITs handle that by installing exception handlers
> > and zeroing out destination registers if it happens to be a NULL or
> > invalid pointer. This mimics bpf_probe_read() behavior, btw.
>
> Yes aware of all this.
>
> >
> > So I think the way it's described and named in the verifier makes
> > sense, at least from the verifier's implementation point of view.
>
> The other other problem with the proposed patch is it makes BTF_ID
> and BTF_ID_OR_NULL the same from the reg_type_str side which might
> make things a bit confusing.
>
> I'm fine to drop this, but from the branch analysis side it still
> feels odd to me. I would need to look at it more to see what the
> side effects might be, but perhaps we should consider adding it
> to the list in reg_type_may_be_null(). OTOH this is not causing
> me any real problems at the moment just idle speculation so we
> can leave it alone for now.
>
> >
> > >
> > > Improve the comment here to reflect the current state and change the reg
> > > type string to indicate it may be null. We should try to avoid this in
> > > future types, but its too much code churn to unwind at this point.
> > >
> > > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > > ---
> > > include/linux/bpf.h | 2 +-
> > > kernel/bpf/verifier.c | 2 +-
> > > 2 files changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > > index 40c5e206ecf2..b9c192fe0d0f 100644
> > > --- a/include/linux/bpf.h
> > > +++ b/include/linux/bpf.h
> > > @@ -352,7 +352,7 @@ enum bpf_reg_type {
> > > PTR_TO_TCP_SOCK_OR_NULL, /* reg points to struct tcp_sock or NULL */
> > > PTR_TO_TP_BUFFER, /* reg points to a writable raw tp's buffer */
> > > PTR_TO_XDP_SOCK, /* reg points to struct xdp_sock */
> > > - PTR_TO_BTF_ID, /* reg points to kernel struct */
> > > + PTR_TO_BTF_ID, /* reg points to kernel struct or NULL */
John,
could you please add the summary of this discussion here as a comment?
I think it's too important to lose and it's better to stay as comment.
> > > PTR_TO_BTF_ID_OR_NULL, /* reg points to kernel struct or NULL */
> > > PTR_TO_MEM, /* reg points to valid memory region */
> > > PTR_TO_MEM_OR_NULL, /* reg points to valid memory region or NULL */
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index b6ccfce3bf4c..d657efcad47b 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -501,7 +501,7 @@ static const char * const reg_type_str[] = {
> > > [PTR_TO_TCP_SOCK_OR_NULL] = "tcp_sock_or_null",
> > > [PTR_TO_TP_BUFFER] = "tp_buffer",
> > > [PTR_TO_XDP_SOCK] = "xdp_sock",
> > > - [PTR_TO_BTF_ID] = "ptr_",
> > > + [PTR_TO_BTF_ID] = "ptr_or_null_",
but this one I would keep as-is. imo ptr_ is more correct here.
> > > [PTR_TO_BTF_ID_OR_NULL] = "ptr_or_null_",
> > > [PTR_TO_MEM] = "mem",
> > > [PTR_TO_MEM_OR_NULL] = "mem_or_null",
> > >
>
>
prev parent reply other threads:[~2020-08-02 3:11 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-31 22:35 [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null John Fastabend
2020-08-01 3:30 ` Andrii Nakryiko
2020-08-02 2:39 ` John Fastabend
2020-08-02 3:10 ` Alexei Starovoitov [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=20200802031058.rk4rlygvbbmny3bl@ast-mbp.dhcp.thefacebook.com \
--to=alexei.starovoitov@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.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 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).