BPF Archive on lore.kernel.org
 help / color / Atom feed
From: John Fastabend <john.fastabend@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	John Fastabend <john.fastabend@gmail.com>
Cc: 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, 01 Aug 2020 19:39:13 -0700
Message-ID: <5f262751ab97c_11b82ae318aac5b44d@john-XPS-13-9370.notmuch> (raw)
In-Reply-To: <CAEf4BzZ9=av=EvbyzhoyCg0ZvTOA2GBPgq5vyb1SaoNmqwL6XQ@mail.gmail.com>

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 */
> >         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_",
> >         [PTR_TO_BTF_ID_OR_NULL] = "ptr_or_null_",
> >         [PTR_TO_MEM]            = "mem",
> >         [PTR_TO_MEM_OR_NULL]    = "mem_or_null",
> >



  reply index

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-31 22:35 John Fastabend
2020-08-01  3:30 ` Andrii Nakryiko
2020-08-02  2:39   ` John Fastabend [this message]
2020-08-02  3:10     ` 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=5f262751ab97c_11b82ae318aac5b44d@john-XPS-13-9370.notmuch \
    --to=john.fastabend@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --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

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git