On Fri, Jul 31, 2020 at 02:08:55PM +0200, KP Singh wrote:
[ ... ]
> >> +const struct bpf_map_ops inode_storage_map_ops = {
> >> + .map_alloc_check = bpf_local_storage_map_alloc_check,
> >> + .map_alloc = inode_storage_map_alloc,
> >> + .map_free = inode_storage_map_free,
> >> + .map_get_next_key = notsupp_get_next_key,
> >> + .map_lookup_elem = bpf_fd_inode_storage_lookup_elem,
> >> + .map_update_elem = bpf_fd_inode_storage_update_elem,
> >> + .map_delete_elem = bpf_fd_inode_storage_delete_elem,
> >> + .map_check_btf = bpf_local_storage_map_check_btf,
> >> + .map_btf_name = "bpf_local_storage_map",
> >> + .map_btf_id = &sk_storage_map_btf_id,
> >> + .map_owner_storage_ptr = inode_storage_ptr,
> >> +};
> >> +
> >> +BTF_ID_LIST(bpf_inode_storage_get_btf_ids)
> >> +BTF_ID(struct, inode)
> > The ARG_PTR_TO_BTF_ID is the second arg instead of the first
> > arg in bpf_inode_storage_get_proto.
> > Does it just happen to work here without needing BTF_ID_UNUSED?
>
>
> Yeah, this surprised me as to why it worked, so I did some debugging:
>
>
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index 9cd1428c7199..95e84bcf1a74 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -52,6 +52,8 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> {
> switch (func_id) {
> case BPF_FUNC_inode_storage_get:
> + pr_err("btf_ids[0]=%d\n", bpf_inode_storage_get_proto.btf_id[0]);
> + pr_err("btf_ids[1]=%d\n", bpf_inode_storage_get_proto.btf_id[1]);
> return &bpf_inode_storage_get_proto;
> case BPF_FUNC_inode_storage_delete:
> return &bpf_inode_storage_delete_proto;
>
> ./test_progs -t test_local_storage
>
> [ 21.694473] btf_ids[0]=915
> [ 21.694974] btf_ids[1]=915
>
> btf dump file /sys/kernel/btf/vmlinux | grep "STRUCT 'inode'"
> "[915] STRUCT 'inode' size=984 vlen=48
>
> So it seems like btf_id[0] and btf_id[1] are set to the BTF ID
> for inode. Now I think this might just be a coincidence as
> the next helper (bpf_inode_storage_delete)
> also has a BTF argument of type inode.
It seems the next BTF_ID_LIST(bpf_inode_storage_delete_btf_ids)
is not needed because they are the same. I think one
BTF_ID_LIST(bpf_inode_btf_ids) can be used for both helpers.
>
> and sure enough if I call:
>
> bpf_inode_storage_delete from my selftests program,
> it does not load:
>
> arg#0 type is not a struct
> Unrecognized arg#0 type PTR
> ; int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim)
> 0: (79) r6 = *(u64 *)(r1 +8)
> func 'bpf_lsm_inode_unlink' arg1 has btf_id 912 type STRUCT 'dentry'
> ; __u32 pid = bpf_get_current_pid_tgid() >> 32;
>
> [...]
>
> So, The BTF_ID_UNUSED is actually needed here. I also added a call to
> bpf_inode_storage_delete in my test to catch any issues with it.
>
> After adding BTF_ID_UNUSED, the result is what we expect:
>
> ./test_progs -t test_local_storage
> [ 20.577223] btf_ids[0]=0
> [ 20.577702] btf_ids[1]=915
>
> Thanks for noticing this!
>
> - KP
>
> >
> >> +
> >> +const struct bpf_func_proto bpf_inode_storage_get_proto = {
> >> + .func = bpf_inode_storage_get,
> >> + .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,
> >> + .arg3_type = ARG_PTR_TO_MAP_VALUE_OR_NULL,
> >> + .arg4_type = ARG_ANYTHING,
> >> + .btf_id = bpf_inode_storage_get_btf_ids,
> >> +};
> >> +
> >> +BTF_ID_LIST(bpf_inode_storage_delete_btf_ids)
> >> +BTF_ID(struct, inode)
> >> +
> >> +const struct bpf_func_proto bpf_inode_storage_delete_proto = {
> >> + .func = bpf_inode_storage_delete,
> >> + .gpl_only = false,
> >> + .ret_type = RET_INTEGER,
> >> + .arg1_type = ARG_CONST_MAP_PTR,
> >> + .arg2_type = ARG_PTR_TO_BTF_ID,
> >> + .btf_id = bpf_inode_storage_delete_btf_ids,
> >> +};
