On 6/23/20 11:23 AM, Andrii Nakryiko wrote:
On Tue, Jun 23, 2020 at 7:52 AM Yonghong Song <yhs@xxxxxx> wrote:
On 6/22/20 11:39 PM, Andrii Nakryiko wrote:
On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@xxxxxx> wrote:
The helper is used in tracing programs to cast a socket
pointer to a tcp6_sock pointer.
The return value could be NULL if the casting is illegal.
A new helper return type RET_PTR_TO_BTF_ID_OR_NULL is added
so the verifier is able to deduce proper return types for the helper.
Different from the previous BTF_ID based helpers,
the bpf_skc_to_tcp6_sock() argument can be several possible
btf_ids. More specifically, all possible socket data structures
with sock_common appearing in the first in the memory layout.
This patch only added socket types related to tcp and udp.
All possible argument btf_id and return value btf_id
for helper bpf_skc_to_tcp6_sock() are pre-calculcated and
cached. In the future, it is even possible to precompute
these btf_id's at kernel build time.
Acked-by: Martin KaFai Lau <kafai@xxxxxx>
Signed-off-by: Yonghong Song <yhs@xxxxxx>
---
Looks good to me as is, but see a few suggestions, they will probably
save me time at some point as well :)
Acked-by: Andrii Nakryiko <andriin@xxxxxx>
include/linux/bpf.h | 12 +++++
include/uapi/linux/bpf.h | 9 +++-
kernel/bpf/btf.c | 1 +
kernel/bpf/verifier.c | 43 +++++++++++++-----
kernel/trace/bpf_trace.c | 2 +
net/core/filter.c | 80 ++++++++++++++++++++++++++++++++++
scripts/bpf_helpers_doc.py | 2 +
tools/include/uapi/linux/bpf.h | 9 +++-
8 files changed, 146 insertions(+), 12 deletions(-)
[...]
@@ -4815,6 +4826,18 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
regs[BPF_REG_0].id = ++env->id_gen;
regs[BPF_REG_0].mem_size = meta.mem_size;
+ } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
+ int ret_btf_id;
+
+ mark_reg_known_zero(env, regs, BPF_REG_0);
+ regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
+ ret_btf_id = *fn->ret_btf_id;
might be a good idea to check fb->ret_btf_id for NULL and print a
warning + return -EFAULT. It's not supposed to happen on properly
configured kernel, but during development this will save a bunch of
time and frustration for next person trying to add something with
RET_PTR_TO_BTF_ID_OR_NULL.
I would like prefer to delay this with current code. Otherwise,
it gives an impression fn->ret_btf_id might be NULL and it is
actually never NULL. We can add NULL check if the future change
requires it. I am not sure what the future change could be,
but you need some way to get the return btf_id, the above is
one of them.
It's not **supposed** to be NULL, same as a bunch of other invariants
about BPF helper proto definitions, but verifier does check sanity for
such cases, instead of crashing. But up to you. I'm pretty sure
someone will trip up on this.
I think there are certain expectation for argument reg_state vs. certain
fields in the structure.
int btf_resolve_helper_id(struct bpf_verifier_log *log,
const struct bpf_func_proto *fn, int arg)
{
int *btf_id = &fn->btf_id[arg];
int ret;
if (fn->arg_type[arg] != ARG_PTR_TO_BTF_ID)
return -EINVAL;
ret = READ_ONCE(*btf_id);
...
}
If ARG_PTR_TO_BTF_ID, the verifier did not really check
whether btf_id pointer is valid or not. It just use it.
The same applies to the new return type. If in func_proto,
somebody sets RET_PTR_TO_BTF_ID_OR_NULL, it is expected
that func_proto->ret_btf_id is valid.
Code review or feature selftest should catch errors
if they are out-of-sync.
+ if (ret_btf_id == 0) {
This also has to be struct/union (after typedef/mods stripping, of
course). Or are there other cases?
This is an "int". The func_proto difinition is below:
int *ret_btf_id; /* return value btf_id */
I meant the BTF type itself that this btf_id points to. Is there any
use case where this won't be a pointer to struct/union and instead
something like a pointer to an int?
Maybe you misunderstood. The mechanism is similar to the argument btf_id
encoding in func_proto's:
static int bpf_seq_printf_btf_ids[5];
...
.btf_id = bpf_seq_printf_btf_ids,
func_proto->ret_btf_id will be a pointer to int which encodes the
btf_id, not the btf_type.
+ verbose(env, "invalid return type %d of func %s#%d\n",
+ fn->ret_type, func_id_name(func_id), func_id);
+ return -EINVAL;
+ }
+ regs[BPF_REG_0].btf_id = ret_btf_id;
} else {
verbose(env, "unknown return type %d of func %s#%d\n",
fn->ret_type, func_id_name(func_id), func_id);
[...]
+void init_btf_sock_ids(struct btf *btf)
+{
+ int i, btf_id;
+
+ for (i = 0; i < MAX_BTF_SOCK_TYPE; i++) {
+ btf_id = btf_find_by_name_kind(btf, bpf_sock_types[i],
+ BTF_KIND_STRUCT);
+ if (btf_id > 0)
+ btf_sock_ids[i] = btf_id;
+ }
+}
This will hopefully go away with Jiri's work on static BTF IDs, right?
So looking forward to that :)
Yes. That's the plan.
+
+static bool check_arg_btf_id(u32 btf_id, u32 arg)
+{
+ int i;
+
+ /* only one argument, no need to check arg */
+ for (i = 0; i < MAX_BTF_SOCK_TYPE; i++)
+ if (btf_sock_ids[i] == btf_id)
+ return true;
+ return false;
+}
+
[...]
