| Message ID | 20230404045029.82870-2-alexei.starovoitov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7d64c513284408fee5178a0953a686e9410f2399 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Follow up to RCU enforcement in the verifier. | expand |
On Mon, Apr 3, 2023 at 9:50 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > From: Alexei Starovoitov <ast@kernel.org> > > Remove duplicated if (atype == BPF_READ) btf_struct_access() from > btf_struct_access() callback and invoke it only for writes. It would be nice to elaborate a bit why this is ok. As far as I can tell, it's because custom btf_struct_access() callbacks are only checking and overriding write accesses, delegating reads to generic btf_struct_access(). Is that right? If so, can you please note it down in the commit message? Further, given btf_struct_access *callbacks* are now write-only, while we still keep generic btf_struct_access for reads, should we distinguish callback's write-only nature by renaming it to something like "btf_struct_write_access"? > > Signed-off-by: Alexei Starovoitov <ast@kernel.org> > --- > kernel/bpf/verifier.c | 2 +- > net/bpf/bpf_dummy_struct_ops.c | 2 +- > net/core/filter.c | 6 ------ > net/ipv4/bpf_tcp_ca.c | 3 --- > 4 files changed, 2 insertions(+), 11 deletions(-) > [...]
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index eaf9c5291cf0..83984568ccb4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5504,7 +5504,7 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env, return -EACCES; } - if (env->ops->btf_struct_access && !type_is_alloc(reg->type)) { + if (env->ops->btf_struct_access && !type_is_alloc(reg->type) && atype == BPF_WRITE) { if (!btf_is_kernel(reg->btf)) { verbose(env, "verifier internal error: reg->btf must be kernel btf\n"); return -EFAULT; diff --git a/net/bpf/bpf_dummy_struct_ops.c b/net/bpf/bpf_dummy_struct_ops.c index ff4f89a2b02a..9535c8506cda 100644 --- a/net/bpf/bpf_dummy_struct_ops.c +++ b/net/bpf/bpf_dummy_struct_ops.c @@ -198,7 +198,7 @@ static int bpf_dummy_ops_btf_struct_access(struct bpf_verifier_log *log, if (err < 0) return err; - return atype == BPF_READ ? err : NOT_INIT; + return NOT_INIT; } static const struct bpf_verifier_ops bpf_dummy_verifier_ops = { diff --git a/net/core/filter.c b/net/core/filter.c index 3370efad1dda..8b9f409a2ec3 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -8753,9 +8753,6 @@ static int tc_cls_act_btf_struct_access(struct bpf_verifier_log *log, { int ret = -EACCES; - if (atype == BPF_READ) - return btf_struct_access(log, reg, off, size, atype, next_btf_id, flag); - mutex_lock(&nf_conn_btf_access_lock); if (nfct_btf_struct_access) ret = nfct_btf_struct_access(log, reg, off, size, atype, next_btf_id, flag); @@ -8830,9 +8827,6 @@ static int xdp_btf_struct_access(struct bpf_verifier_log *log, { int ret = -EACCES; - if (atype == BPF_READ) - return btf_struct_access(log, reg, off, size, atype, next_btf_id, flag); - mutex_lock(&nf_conn_btf_access_lock); if (nfct_btf_struct_access) ret = nfct_btf_struct_access(log, reg, off, size, atype, next_btf_id, flag); diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c index ea21c96c03aa..d6465876bbf6 100644 --- a/net/ipv4/bpf_tcp_ca.c +++ b/net/ipv4/bpf_tcp_ca.c @@ -78,9 +78,6 @@ static int bpf_tcp_ca_btf_struct_access(struct bpf_verifier_log *log, const struct btf_type *t; size_t end; - if (atype == BPF_READ) - return btf_struct_access(log, reg, off, size, atype, next_btf_id, flag); - t = btf_type_by_id(reg->btf, reg->btf_id); if (t != tcp_sock_type) { bpf_log(log, "only read is supported\n");