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

[KP Singh]

On 8/25/20 2:52 AM, Martin KaFai Lau wrote:
> On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:
>> From: KP Singh <kpsingh@xxxxxxxxxx>
>>
>> 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.
>>
> [ ... ]
> 
>> diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
>> new file mode 100644
>> index 000000000000..b0b283c224c1
>> --- /dev/null
>> +++ b/kernel/bpf/bpf_inode_storage.c

[...]

>> +
>> +DEFINE_BPF_STORAGE_CACHE(inode_cache);
>> +
>> +static struct bpf_local_storage __rcu **
>> +inode_storage_ptr(void *owner)
>> +{
>> +	struct inode *inode = owner;
>> +	struct bpf_storage_blob *bsb;
>> +
>> +	bsb = bpf_inode(inode);
>> +	if (!bsb)
>> +		return NULL;
> just noticed this one.  NULL could be returned here.  When will it happen?

This can happen if CONFIG_BPF_LSM is enabled but "bpf" is not in the list of
active LSMs.

> 
>> +	return &bsb->storage;
>> +}
>> +
>> +static struct bpf_local_storage_data *inode_storage_lookup(struct inode *inode,
>> +							   struct bpf_map *map,
>> +							   bool cacheit_lockit)
>> +{

[...]

> path first before calling the bpf_local_storage_update() since
> this case is specific to inode local storage.
> 
> Same for the other bpf_local_storage_update() cases.

If you're okay with this I can send a new series with the following updates.

diff --git a/kernel/bpf/bpf_inode_storage.c b/kernel/bpf/bpf_inode_storage.c
index b0b283c224c1..74546cee814d 100644
--- a/kernel/bpf/bpf_inode_storage.c
+++ b/kernel/bpf/bpf_inode_storage.c
@@ -125,7 +125,7 @@ static int bpf_fd_inode_storage_update_elem(struct bpf_map *map, void *key,
 
        fd = *(int *)key;
        f = fget_raw(fd);
-       if (!f)
+       if (!f || !inode_storage_ptr(f->f_inode))
                return -EBADF;
 
        sdata = bpf_local_storage_update(f->f_inode,
@@ -171,6 +171,14 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
        if (flags & ~(BPF_LOCAL_STORAGE_GET_F_CREATE))
                return (unsigned long)NULL;
 
+       /* explicitly check that the inode_storage_ptr is not
+        * NULL as inode_storage_lookup returns NULL in this case and
+        * and bpf_local_storage_update expects the owner to have a
+        * valid storage pointer.
+        */
+       if (!inode_storage_ptr(inode))
+               return (unsigned long)NULL;
+
        sdata = inode_storage_lookup(inode, map, true);
        if (sdata)
                return (unsigned long)sdata->data;


> 
>> +					 (struct bpf_local_storage_map *)map,
>> +					 value, map_flags);
>> +	fput(f);
>> +	return PTR_ERR_OR_ZERO(sdata);
>> +}
>> +
> 

[...]

>> +	return (unsigned long)NULL;
>> +}
>> +
> 
>> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
>> index 32d32d485451..35f9b19259e5 100644
>> --- a/security/bpf/hooks.c
>> +++ b/security/bpf/hooks.c
>> @@ -3,6 +3,7 @@
>>  /*
>>   * Copyright (C) 2020 Google LLC.
>>   */
>> +#include <linux/bpf_local_storage.h>
> Is it needed?

No. Removed. Thanks!

> 
>>  #include <linux/lsm_hooks.h>
>>  #include <linux/bpf_lsm.h>
>>  
>> @@ -11,6 +12,7 @@ static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
>>  	LSM_HOOK_INIT(NAME, bpf_lsm_##NAME),

[...]

>> +	.blobs = &bpf_lsm_blob_sizes
>>  };