Re: [PATCH bpf-next v7 5/7] bpf: Implement bpf_local_storage for inodes

[Martin KaFai Lau <kafai@fb.com>]

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,
> >> +};