From: David Vernet <void@manifault.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <martin.lau@kernel.org>,
Dave Marchevsky <davemarchevsky@meta.com>,
Tejun Heo <tj@kernel.org>,
Kumar Kartikeya Dwivedi <memxor@gmail.com>,
Network Development <netdev@vger.kernel.org>,
bpf <bpf@vger.kernel.org>, Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer.
Date: Tue, 4 Apr 2023 15:44:19 -0500 [thread overview]
Message-ID: <20230404204419.GC3896@maniforge> (raw)
In-Reply-To: <CAADnVQJEBJdXp11NE_zti0jBHbMmodDKh7YuBFGkN3q_wOHJtA@mail.gmail.com>
On Tue, Apr 04, 2023 at 01:17:25PM -0700, Alexei Starovoitov wrote:
> On Tue, Apr 4, 2023 at 7:46 AM David Vernet <void@manifault.com> wrote:
> >
> > On Mon, Apr 03, 2023 at 09:50:25PM -0700, Alexei Starovoitov wrote:
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > bpf_[sk|inode|task|cgrp]_storage_[get|delete]() and bpf_get_socket_cookie() helpers
> > > perform run-time check that sk|inode|task|cgrp pointer != NULL.
> > > Teach verifier about this fact and allow bpf programs to pass
> > > PTR_TO_BTF_ID | PTR_MAYBE_NULL into such helpers.
> > > It will be used in the subsequent patch that will do
> > > bpf_sk_storage_get(.., skb->sk, ...);
> > > Even when 'skb' pointer is trusted the 'sk' pointer may be NULL.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > > kernel/bpf/bpf_cgrp_storage.c | 4 ++--
> > > kernel/bpf/bpf_inode_storage.c | 4 ++--
> > > kernel/bpf/bpf_task_storage.c | 8 ++++----
> > > net/core/bpf_sk_storage.c | 4 ++--
> > > net/core/filter.c | 2 +-
> > > 5 files changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/kernel/bpf/bpf_cgrp_storage.c b/kernel/bpf/bpf_cgrp_storage.c
> > > index d17d5b694668..d44fe8dd9732 100644
> > > --- a/kernel/bpf/bpf_cgrp_storage.c
> > > +++ b/kernel/bpf/bpf_cgrp_storage.c
> > > @@ -224,7 +224,7 @@ const struct bpf_func_proto bpf_cgrp_storage_get_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &bpf_cgroup_btf_id[0],
> > > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg4_type = ARG_ANYTHING,
> > > @@ -235,6 +235,6 @@ const struct bpf_func_proto bpf_cgrp_storage_delete_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_INTEGER,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &bpf_cgroup_btf_id[0],
> > > };
> > > diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
> > > index e17ad581b9be..a4d93df78c75 100644
> > > --- a/kernel/bpf/bpf_inode_storage.c
> > > +++ b/kernel/bpf/bpf_inode_storage.c
> > > @@ -229,7 +229,7 @@ const struct bpf_func_proto bpf_inode_storage_get_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &bpf_inode_storage_btf_ids[0],
> > > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg4_type = ARG_ANYTHING,
> > > @@ -240,6 +240,6 @@ const struct bpf_func_proto bpf_inode_storage_delete_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_INTEGER,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &bpf_inode_storage_btf_ids[0],
> > > };
> > > diff --git a/kernel/bpf/bpf_task_storage.c b/kernel/bpf/bpf_task_storage.c
> > > index d1af0c8f9ce4..adf6dfe0ba68 100644
> > > --- a/kernel/bpf/bpf_task_storage.c
> > > +++ b/kernel/bpf/bpf_task_storage.c
> > > @@ -338,7 +338,7 @@ const struct bpf_func_proto bpf_task_storage_get_recur_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg4_type = ARG_ANYTHING,
> > > @@ -349,7 +349,7 @@ const struct bpf_func_proto bpf_task_storage_get_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg4_type = ARG_ANYTHING,
> > > @@ -360,7 +360,7 @@ const struct bpf_func_proto bpf_task_storage_delete_recur_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_INTEGER,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > > };
> > >
> > > @@ -369,6 +369,6 @@ const struct bpf_func_proto bpf_task_storage_delete_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_INTEGER,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &btf_tracing_ids[BTF_TRACING_TYPE_TASK],
> > > };
> > > diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
> > > index 085025c7130a..d4172534dfa8 100644
> > > --- a/net/core/bpf_sk_storage.c
> > > +++ b/net/core/bpf_sk_storage.c
> > > @@ -412,7 +412,7 @@ const struct bpf_func_proto bpf_sk_storage_get_tracing_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> > > .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> > > .arg4_type = ARG_ANYTHING,
> > > @@ -424,7 +424,7 @@ const struct bpf_func_proto bpf_sk_storage_delete_tracing_proto = {
> > > .gpl_only = false,
> > > .ret_type = RET_INTEGER,
> > > .arg1_type = ARG_CONST_MAP_PTR,
> > > - .arg2_type = ARG_PTR_TO_BTF_ID,
> > > + .arg2_type = ARG_PTR_TO_BTF_ID_OR_NULL,
> > > .arg2_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> > > .allowed = bpf_sk_storage_tracing_allowed,
> > > };
> >
> > Should we also add PTR_MAYBE_NULL to the ARG_PTR_TO_BTF_ID_SOCK_COMMON
> > arg in bpf_sk_storage_get_proto and bpf_sk_storage_delete_proto?
>
> It makes sense. I'd like to do it in the follow up though.
> I haven't seen networking progs passing null-able pointer into these helpers.
> Only tracing progs do.
> I need to craft a test case, etc.
Sounds good
> While this set is good to go imo.
Agreed, the series LGTM.
next prev parent reply other threads:[~2023-04-04 20:44 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-04 4:50 [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier Alexei Starovoitov
2023-04-04 4:50 ` [PATCH bpf-next 1/8] bpf: Invoke btf_struct_access() callback only for writes Alexei Starovoitov
2023-04-04 23:29 ` Andrii Nakryiko
2023-04-04 4:50 ` [PATCH bpf-next 2/8] bpf: Remove unused arguments from btf_struct_access() Alexei Starovoitov
2023-04-04 23:31 ` Andrii Nakryiko
2023-04-04 4:50 ` [PATCH bpf-next 3/8] bpf: Refactor btf_nested_type_is_trusted() Alexei Starovoitov
2023-04-04 4:50 ` [PATCH bpf-next 4/8] bpf: Teach verifier that certain helpers accept NULL pointer Alexei Starovoitov
2023-04-04 14:46 ` David Vernet
2023-04-04 20:17 ` Alexei Starovoitov
2023-04-04 20:44 ` David Vernet [this message]
2023-04-05 0:10 ` Martin KaFai Lau
2023-04-05 0:17 ` Alexei Starovoitov
2023-04-04 4:50 ` [PATCH bpf-next 5/8] bpf: Refactor NULL-ness check in check_reg_type() Alexei Starovoitov
2023-04-04 4:50 ` [PATCH bpf-next 6/8] bpf: Allowlist few fields similar to __rcu tag Alexei Starovoitov
2023-04-04 4:50 ` [PATCH bpf-next 7/8] bpf: Undo strict enforcement for walking untagged fields Alexei Starovoitov
2023-04-04 4:50 ` [PATCH bpf-next 8/8] selftests/bpf: Add tracing tests for walking skb and req Alexei Starovoitov
2023-04-04 14:51 ` [PATCH bpf-next 0/8] bpf: Follow up to RCU enforcement in the verifier David Vernet
2023-04-05 0:02 ` Andrii Nakryiko
2023-04-05 0:16 ` Alexei Starovoitov
2023-04-05 1:51 ` Jakub Kicinski
2023-04-05 17:22 ` Andrii Nakryiko
2023-04-05 18:19 ` Jakub Kicinski
2023-04-05 20:11 ` Andrii Nakryiko
2023-04-06 5:13 ` Alexei Starovoitov
2023-04-06 15:42 ` Jakub Kicinski
2023-04-07 1:17 ` Alexei Starovoitov
2023-04-07 1:23 ` Jakub Kicinski
2023-04-07 1:32 ` Alexei Starovoitov
2023-04-07 1:57 ` Jakub Kicinski
2023-04-05 19:24 ` Daniel Borkmann
2023-04-05 0:10 ` patchwork-bot+netdevbpf
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=20230404204419.GC3896@maniforge \
--to=void@manifault.com \
--cc=alexei.starovoitov@gmail.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=davemarchevsky@meta.com \
--cc=kernel-team@fb.com \
--cc=martin.lau@kernel.org \
--cc=memxor@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=tj@kernel.org \
/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 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.