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