* [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null @ 2020-07-31 22:35 John Fastabend 2020-08-01 3:30 ` Andrii Nakryiko 0 siblings, 1 reply; 4+ messages in thread From: John Fastabend @ 2020-07-31 22:35 UTC (permalink / raw) To: yhs, john.fastabend, ast, daniel; +Cc: bpf 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", ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null 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 0 siblings, 1 reply; 4+ messages in thread From: Andrii Nakryiko @ 2020-08-01 3:30 UTC (permalink / raw) To: John Fastabend; +Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, bpf 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", > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null 2020-08-01 3:30 ` Andrii Nakryiko @ 2020-08-02 2:39 ` John Fastabend 2020-08-02 3:10 ` Alexei Starovoitov 0 siblings, 1 reply; 4+ messages in thread From: John Fastabend @ 2020-08-02 2:39 UTC (permalink / raw) To: Andrii Nakryiko, John Fastabend Cc: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, bpf 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", > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bpf-next PATCH] bpf: Add comment in bpf verifier to note PTR_TO_BTF_ID can be null 2020-08-02 2:39 ` John Fastabend @ 2020-08-02 3:10 ` Alexei Starovoitov 0 siblings, 0 replies; 4+ messages in thread From: Alexei Starovoitov @ 2020-08-02 3:10 UTC (permalink / raw) To: John Fastabend Cc: Andrii Nakryiko, Yonghong Song, Alexei Starovoitov, Daniel Borkmann, bpf 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", > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-08-02 3:11 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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.