On 8/25/20 2:52 AM, Martin KaFai Lau wrote:

On Sun, Aug 23, 2020 at 06:56:10PM +0200, KP Singh wrote:

quoted

From: KP Singh <redacted>

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.

[ ... ]

quoted

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

[...]

quoted

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

quoted

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

quoted

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

[...]

quoted

+	return (unsigned long)NULL;
+}
+

quoted

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!

quoted

 #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),

[...]

quoted

+	.blobs = &bpf_lsm_blob_sizes
 };