Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: KP Singh <kpsingh@chromium.org>
To: Martin KaFai Lau <kafai@fb.com>
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 14:08:55 +0200
Message-ID: <adbfc73e-bd32-d9ba-4dab-4ccc39b40fdd@chromium.org> (raw)
In-Reply-To: <20200731010822.fctk5lawnr3p7blf@kafai-mbp.dhcp.thefacebook.com>



On 31.07.20 03:08, Martin KaFai Lau wrote:
> On Thu, Jul 30, 2020 at 04:07:14PM +0200, KP Singh wrote:
>> From: KP Singh <kpsingh@google.com>
>>
>> Similar to bpf_local_storage for sockets, add local storage for inodes.
>> The life-cycle of storage is managed with the life-cycle of the inode.
>> i.e. the storage is destroyed along with the owning inode.
>>
>> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
>> security blob which are now stackable and can co-exist with other LSMs.
>>
> 
> [ ... ]
> 
>> +static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
>> +					 void *value, u64 map_flags)
>> +{
>> +	struct bpf_local_storage_data *sdata;
>> +	struct file *f;
>> +	int fd;
>> +
>> +	fd = *(int *)key;
>> +	f = fcheck(fd);
>> +	if (!f)
>> +		return -EINVAL;
>> +
>> +	sdata = bpf_local_storage_update(f->f_inode, map, value, map_flags);
> n00b question.  inode will not be going away here (like another
> task calls close(fd))?  and there is no chance that bpf_local_storage_update()
> will be adding new storage after bpf_inode_storage_free() was called?

Good catch, I think we need to guard this update by grabbing a reference 
to the file here.

> 
> A few comments will be useful here.
> 
>> +	return PTR_ERR_OR_ZERO(sdata);
>> +}
>> +
>> +static int inode_storage_delete(struct inode *inode, struct bpf_map *map)
>> +{
>> +	struct bpf_local_storage_data *sdata;
>> +
>> +	sdata = inode_storage_lookup(inode, map, false);
>> +	if (!sdata)
>> +		return -ENOENT;
>> +
>> +	bpf_selem_unlink(SELEM(sdata));
>> +
>> +	return 0;
>> +}
>> +
>> +static int bpf_fd_inode_storage_delete_elem(struct bpf_map *map, void *key)
>> +{
>> +	struct file *f;
>> +	int fd;
>> +
>> +	fd = *(int *)key;
>> +	f = fcheck(fd);
>> +	if (!f)
>> +		return -EINVAL;
>> +
>> +	return inode_storage_delete(f->f_inode, map);
>> +}
>> +
>> +BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
>> +	   void *, value, u64, flags)
>> +{
>> +	struct bpf_local_storage_data *sdata;
>> +
>> +	if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
>> +		return (unsigned long)NULL;
>> +
>> +	sdata = inode_storage_lookup(inode, map, true);
>> +	if (sdata)
>> +		return (unsigned long)sdata->data;
>> +
>> +	if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) {
>> +		sdata = bpf_local_storage_update(inode, map, value,
>> +						 BPF_NOEXIST);
> The same question here

This is slightly different. The helper gets called from the BPF program.
We are only allowing this from LSM hooks which have better guarantees
w.r.t the lifetime of the object unlike tracing programs.

I will add a comment that explains this. Once we have sleepable BPF we can
also grab a reference to the inode here.

> 
>> +		return IS_ERR(sdata) ? (unsigned long)NULL :
>> +					     (unsigned long)sdata->data;
>> +	}
>> +
>> +	return (unsigned long)NULL;
>> +}
>> +
>> +BPF_CALL_2(bpf_inode_storage_delete,
>> +	   struct bpf_map *, map, struct inode *, inode)
>> +{
>> +	return inode_storage_delete(inode, map);
>> +}
>> +
>> +static int notsupp_get_next_key(struct bpf_map *map, void *key,
>> +				void *next_key)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>> +static struct bpf_map *inode_storage_map_alloc(union bpf_attr *attr)
>> +{
>> +	struct bpf_local_storage_map *smap;
>> +
>> +	smap = bpf_local_storage_map_alloc(attr);
>> +	if (IS_ERR(smap))
>> +		return ERR_CAST(smap);
>> +
>> +	smap->cache_idx = bpf_local_storage_cache_idx_get(&inode_cache);
>> +	return &smap->map;
>> +}
>> +
>> +static void inode_storage_map_free(struct bpf_map *map)
>> +{
>> +	struct bpf_local_storage_map *smap;
>> +
>> +	smap = (struct bpf_local_storage_map *)map;
>> +	bpf_local_storage_cache_idx_free(&inode_cache, smap->cache_idx);
>> +	bpf_local_storage_map_free(smap);
>> +}
>> +
>> +static int sk_storage_map_btf_id;

This name needs to be fixed as well.

>> +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.

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 [this message]
2020-07-31 19:02       ` Martin KaFai Lau
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=adbfc73e-bd32-d9ba-4dab-4ccc39b40fdd@chromium.org \
    --to=kpsingh@chromium.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=jannh@google.com \
    --cc=kafai@fb.com \
    --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