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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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