[bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null

[bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null

[John Fastabend]

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.

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",

Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null

[Andrii Nakryiko]

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.

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.

>
> 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",
>

Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null

[John Fastabend]

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",
> >

Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null

[Alexei Starovoitov]

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",
> > >
> 
>