Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Martin KaFai Lau <kafai@fb.com>
To: KP Singh <kpsingh@chromium.org>
Cc: <linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>,
	<linux-security-module@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Paul Turner <pjt@google.com>, Jann Horn <jannh@google.com>,
	Florent Revest <revest@chromium.org>
Subject: Re: [PATCH bpf-next v7 5/7] bpf: Implement bpf_local_storage for inodes
Date: Fri, 31 Jul 2020 12:02:31 -0700
Message-ID: <20200731190226.6ugmk6cnl2yortgt@kafai-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <adbfc73e-bd32-d9ba-4dab-4ccc39b40fdd@chromium.org>

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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30 14:07 [PATCH bpf-next v7 0/7] Generalizing bpf_local_storage KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 1/7] A purely mechanical change to split the renaming from the actual generalization KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 2/7] bpf: Generalize caching for sk_storage KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 3/7] bpf: Generalize bpf_sk_storage KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 4/7] bpf: Split bpf_local_storage to bpf_sk_storage KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 5/7] bpf: Implement bpf_local_storage for inodes KP Singh
2020-07-31  1:08   ` Martin KaFai Lau
2020-07-31 12:08     ` KP Singh
2020-07-31 19:02       ` Martin KaFai Lau [this message]
2020-08-03 15:41         ` KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 6/7] bpf: Allow local storage to be used from LSM programs KP Singh
2020-07-30 14:07 ` [PATCH bpf-next v7 7/7] bpf: Add selftests for local_storage KP Singh

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=20200731190226.6ugmk6cnl2yortgt@kafai-mbp.dhcp.thefacebook.com \
    --to=kafai@fb.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jannh@google.com \
    --cc=kpsingh@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=pjt@google.com \
    --cc=revest@chromium.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

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git