[PATCH vfs/vfs-7.2.xattr v3] bpf: Add simple xattr support to bpffs

[PATCH vfs/vfs-7.2.xattr v3] bpf: Add simple xattr support to bpffs

[Daniel Borkmann]

Add support for extended attributes on bpffs inodes so that user space
and BPF LSM programs can attach metadata, for example, a content hash
or a security label - to a pinned object or directory. BPF LSM or user
space tooling can then uniformly look at this (e.g. security.bpf.*) in
similar way to other fs'es. The store is in-memory and non-persistent:
it lives only for the lifetime of the mount, like everything else in
bpffs. The modelling is similar to tmpfs.

bpffs serves the trusted.* and security.* namespaces; user.* is left
unsupported. As bpffs is FS_USERNS_MOUNT, security.* is reachable by
the unprivileged mounter in a user namespace, and thus we are using
the simple_xattr_set_limited infra there (trusted.* needs global
CAP_SYS_ADMIN).

bpf_fill_super() is open-coded instead of using simple_fill_super(),
because the root inode must now be allocated through bpf_fs_alloc_inode()
i.e. carry the bpf_fs_inode wrapper and come from the right cache -
which requires s_op (and s_xattr) to be installed before the first
inode is created. While at it, also harden s_iflags with SB_I_NOEXEC
and SB_I_NODEV.

bpf_fs_listxattr() is only reachable through the filesystem via
i_op->listxattr, so the BPF token inode is left untouched. Name-based
fsetxattr()/fgetxattr() on a token fd still work since the get/set
handlers are installed at the superblock.

For security.* namespace, we use simple_xattr_set_limited() but
there was no simple_xattr_add_limited() API yet which was needed
in bpf_fs_initxattrs() to avoid underflows in the accounting. The
symlink target is freed in bpf_free_inode() rather than in
bpf_destroy_inode() so that it is released only after an RCU grace
period, as an RCU path walk following the symlink may still
dereference inode->i_link in security_inode_follow_link(). Lastly,
the bpf_symlink() allocated the symlink target is switched to
GFP_KERNEL_ACCOUNT, so the string is charged to the caller's memcg.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Cc: Christian Brauner <brauner@kernel.org>
---
 v2->v3:
  - Add simple_xattr_add_limited and use it for
    bpf_fs_initxattrs (sashiko)
  - Use GFP_KERNEL_ACCOUNT for bpf_symlink (sashiko)
 v1->v2:
  - Rebase against vfs-7.2.xattr for routing via vfs given it
    contains simple xattr rework (Christian, Alexei)
  - Use simple_xattr_set_limited for security.* namespace (sashiko)
  - Dropped user.* namespace (sashiko)

 fs/xattr.c            |  33 ++++++
 include/linux/bpf.h   |   3 +
 include/linux/xattr.h |   4 +
 kernel/bpf/inode.c    | 256 ++++++++++++++++++++++++++++++++++++++----
 4 files changed, 277 insertions(+), 19 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index af28029239ce..abe5457b6d0a 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -1675,6 +1675,39 @@ int simple_xattr_add(struct simple_xattr_cache *cache, struct list_head *xattrs,
 	return 0;
 }
 
+/**
+ * simple_xattr_add_limited - add an xattr object, charging per-inode limits
+ * @cache: anchor for the hash table
+ * @xattrs: the header of the xattr object
+ * @limits: per-inode limit counters
+ * @new_xattr: the xattr object to add
+ *
+ * Like simple_xattr_add(), but also accounts @new_xattr against @limits so
+ * that a later removal or replacement of it through simple_xattr_set_limited()
+ * decrements counters that were actually incremented, rather than underflowing
+ * them. Use this instead of simple_xattr_add() when seeding initial xattrs
+ * that share a namespace with the limited set/remove path.
+ *
+ * Return: On success zero is returned. On failure a negative error code is
+ * returned.
+ */
+int simple_xattr_add_limited(struct simple_xattr_cache *cache,
+			     struct list_head *xattrs,
+			     struct simple_xattr_limits *limits,
+			     struct simple_xattr *new_xattr)
+{
+	int err;
+
+	err = simple_xattr_limits_inc(limits, new_xattr->size);
+	if (err)
+		return err;
+
+	err = simple_xattr_add(cache, xattrs, new_xattr);
+	if (err)
+		simple_xattr_limits_dec(limits, new_xattr->size);
+	return err;
+}
+
 /**
  * simple_xattrs_free - free xattrs
  * @cache: anchor for the hash table
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index b4b703c90ca9..434ba91401c6 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -31,6 +31,7 @@
 #include <linux/static_call.h>
 #include <linux/memcontrol.h>
 #include <linux/cfi.h>
+#include <linux/xattr.h>
 #include <asm/rqspinlock.h>
 
 struct bpf_verifier_env;
@@ -1918,6 +1919,8 @@ struct bpf_mount_opts {
 	u64 delegate_maps;
 	u64 delegate_progs;
 	u64 delegate_attachs;
+
+	struct simple_xattr_cache xa_cache;
 };
 
 struct bpf_token {
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 7aaaf4f8aff5..54ac3cbc133f 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -155,6 +155,10 @@ ssize_t simple_xattr_list(struct inode *inode, struct list_head *xattrs,
 			  char *buffer, size_t size);
 int simple_xattr_add(struct simple_xattr_cache *cache, struct list_head *xattrs,
 		     struct simple_xattr *new_xattr);
+int simple_xattr_add_limited(struct simple_xattr_cache *cache,
+			     struct list_head *xattrs,
+			     struct simple_xattr_limits *limits,
+			     struct simple_xattr *new_xattr);
 int xattr_list_one(char **buffer, ssize_t *remaining_size, const char *name);
 
 void simple_xattr_cache_cleanup(struct simple_xattr_cache *cache);
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index 25c06a011825..c3f79b5a2f8c 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -21,6 +21,9 @@
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/kstrtox.h>
+#include <linux/xattr.h>
+#include <linux/security.h>
+
 #include "preload/bpf_preload.h"
 
 enum bpf_type {
@@ -30,6 +33,23 @@ enum bpf_type {
 	BPF_TYPE_LINK,
 };
 
+struct bpf_fs_inode {
+	struct list_head		xattrs;
+	struct simple_xattr_limits	xlimits;
+	struct inode			vfs_inode;
+};
+
+static inline struct bpf_fs_inode *BPF_FS_I(struct inode *inode)
+{
+	return container_of(inode, struct bpf_fs_inode, vfs_inode);
+}
+
+static struct kmem_cache *bpf_fs_inode_cachep __ro_after_init;
+
+static int bpf_fs_initxattrs(struct inode *inode,
+			     const struct xattr *xattr_array, void *fs_info);
+static ssize_t bpf_fs_listxattr(struct dentry *dentry, char *buf, size_t size);
+
 static void *bpf_any_get(void *raw, enum bpf_type type)
 {
 	switch (type) {
@@ -94,10 +114,17 @@ static void *bpf_fd_probe_obj(u32 ufd, enum bpf_type *type)
 }
 
 static const struct inode_operations bpf_dir_iops;
+static const struct inode_operations bpf_symlink_iops;
 
-static const struct inode_operations bpf_prog_iops = { };
-static const struct inode_operations bpf_map_iops  = { };
-static const struct inode_operations bpf_link_iops  = { };
+static const struct inode_operations bpf_prog_iops = {
+	.listxattr	= bpf_fs_listxattr,
+};
+static const struct inode_operations bpf_map_iops  = {
+	.listxattr	= bpf_fs_listxattr,
+};
+static const struct inode_operations bpf_link_iops  = {
+	.listxattr	= bpf_fs_listxattr,
+};
 
 struct inode *bpf_get_inode(struct super_block *sb,
 			    const struct inode *dir,
@@ -153,11 +180,19 @@ static struct dentry *bpf_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 				struct dentry *dentry, umode_t mode)
 {
 	struct inode *inode;
+	int ret;
 
 	inode = bpf_get_inode(dir->i_sb, dir, mode | S_IFDIR);
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
+	ret = security_inode_init_security(inode, dir, &dentry->d_name,
+					   bpf_fs_initxattrs, NULL);
+	if (ret && ret != -EOPNOTSUPP) {
+		iput(inode);
+		return ERR_PTR(ret);
+	}
+
 	inode->i_op = &bpf_dir_iops;
 	inode->i_fop = &simple_dir_operations;
 
@@ -330,10 +365,20 @@ static int bpf_mkobj_ops(struct dentry *dentry, umode_t mode, void *raw,
 			 const struct file_operations *fops)
 {
 	struct inode *dir = dentry->d_parent->d_inode;
-	struct inode *inode = bpf_get_inode(dir->i_sb, dir, mode);
+	struct inode *inode;
+	int ret;
+
+	inode = bpf_get_inode(dir->i_sb, dir, mode);
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 
+	ret = security_inode_init_security(inode, dir, &dentry->d_name,
+					   bpf_fs_initxattrs, NULL);
+	if (ret && ret != -EOPNOTSUPP) {
+		iput(inode);
+		return ret;
+	}
+
 	inode->i_op = iops;
 	inode->i_fop = fops;
 	inode->i_private = raw;
@@ -382,9 +427,11 @@ bpf_lookup(struct inode *dir, struct dentry *dentry, unsigned flags)
 static int bpf_symlink(struct mnt_idmap *idmap, struct inode *dir,
 		       struct dentry *dentry, const char *target)
 {
-	char *link = kstrdup(target, GFP_USER | __GFP_NOWARN);
 	struct inode *inode;
+	char *link;
+	int ret;
 
+	link = kstrdup(target, GFP_KERNEL_ACCOUNT | __GFP_NOWARN);
 	if (!link)
 		return -ENOMEM;
 
@@ -394,13 +441,25 @@ static int bpf_symlink(struct mnt_idmap *idmap, struct inode *dir,
 		return PTR_ERR(inode);
 	}
 
-	inode->i_op = &simple_symlink_inode_operations;
+	inode->i_op = &bpf_symlink_iops;
 	inode->i_link = link;
 
+	ret = security_inode_init_security(inode, dir, &dentry->d_name,
+					   bpf_fs_initxattrs, NULL);
+	if (ret && ret != -EOPNOTSUPP) {
+		iput(inode);
+		return ret;
+	}
+
 	bpf_dentry_finalize(dentry, inode, dir);
 	return 0;
 }
 
+static const struct inode_operations bpf_symlink_iops = {
+	.get_link	= simple_get_link,
+	.listxattr	= bpf_fs_listxattr,
+};
+
 static const struct inode_operations bpf_dir_iops = {
 	.lookup		= bpf_lookup,
 	.mkdir		= bpf_mkdir,
@@ -409,6 +468,7 @@ static const struct inode_operations bpf_dir_iops = {
 	.rename		= simple_rename,
 	.link		= simple_link,
 	.unlink		= simple_unlink,
+	.listxattr	= bpf_fs_listxattr,
 };
 
 /* pin iterator link into bpffs */
@@ -762,22 +822,147 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root)
 	return 0;
 }
 
+static struct inode *bpf_fs_alloc_inode(struct super_block *sb)
+{
+	struct bpf_fs_inode *bi;
+
+	bi = alloc_inode_sb(sb, bpf_fs_inode_cachep, GFP_KERNEL);
+	if (!bi)
+		return NULL;
+	INIT_LIST_HEAD_RCU(&bi->xattrs);
+	simple_xattr_limits_init(&bi->xlimits);
+	return &bi->vfs_inode;
+}
+
 static void bpf_destroy_inode(struct inode *inode)
 {
+	struct bpf_mount_opts *opts = inode->i_sb->s_fs_info;
+	struct bpf_fs_inode *bi = BPF_FS_I(inode);
 	enum bpf_type type;
 
-	if (S_ISLNK(inode->i_mode))
-		kfree(inode->i_link);
 	if (!bpf_inode_type(inode, &type))
 		bpf_any_put(inode->i_private, type);
-	free_inode_nonrcu(inode);
+	simple_xattrs_free(&opts->xa_cache, &bi->xattrs, NULL);
+}
+
+static void bpf_free_inode(struct inode *inode)
+{
+	if (S_ISLNK(inode->i_mode))
+		kfree(inode->i_link);
+	kmem_cache_free(bpf_fs_inode_cachep, BPF_FS_I(inode));
+}
+
+static int bpf_fs_xattr_get(const struct xattr_handler *handler,
+			    struct dentry *unused, struct inode *inode,
+			    const char *name, void *value, size_t size)
+{
+	struct bpf_mount_opts *opts = inode->i_sb->s_fs_info;
+	struct bpf_fs_inode *bi = BPF_FS_I(inode);
+
+	name = xattr_full_name(handler, name);
+	return simple_xattr_get(&opts->xa_cache, &bi->xattrs, name, value, size);
+}
+
+enum {
+	BPF_FS_XATTR_UNSPEC,
+	BPF_FS_XATTR_SECURITY,
+	BPF_FS_XATTR_TRUSTED,
+};
+
+static int bpf_fs_xattr_set(const struct xattr_handler *handler,
+			    struct mnt_idmap *idmap, struct dentry *unused,
+			    struct inode *inode, const char *name,
+			    const void *value, size_t size, int flags)
+{
+	struct bpf_mount_opts *opts = inode->i_sb->s_fs_info;
+	struct bpf_fs_inode *bi = BPF_FS_I(inode);
+	struct simple_xattr *old;
+	int err = -EINVAL;
+
+	name = xattr_full_name(handler, name);
+	switch (handler->flags) {
+	case BPF_FS_XATTR_SECURITY:
+		err = simple_xattr_set_limited(&opts->xa_cache, &bi->xattrs,
+					       &bi->xlimits, name, value, size,
+					       flags);
+		break;
+	case BPF_FS_XATTR_TRUSTED:
+		old = simple_xattr_set(&opts->xa_cache, &bi->xattrs, name,
+				       value, size, flags);
+		err = IS_ERR(old) ? PTR_ERR(old) : 0;
+		if (!err)
+			simple_xattr_free_rcu(old);
+		break;
+	}
+	if (err)
+		return err;
+	inode_set_ctime_current(inode);
+	return 0;
+}
+
+static const struct xattr_handler bpf_fs_trusted_xattr_handler = {
+	.prefix	= XATTR_TRUSTED_PREFIX,
+	.flags	= BPF_FS_XATTR_TRUSTED,
+	.get	= bpf_fs_xattr_get,
+	.set	= bpf_fs_xattr_set,
+};
+
+static const struct xattr_handler bpf_fs_security_xattr_handler = {
+	.prefix	= XATTR_SECURITY_PREFIX,
+	.flags	= BPF_FS_XATTR_SECURITY,
+	.get	= bpf_fs_xattr_get,
+	.set	= bpf_fs_xattr_set,
+};
+
+static const struct xattr_handler * const bpf_fs_xattr_handlers[] = {
+	&bpf_fs_trusted_xattr_handler,
+	&bpf_fs_security_xattr_handler,
+	NULL,
+};
+
+static ssize_t bpf_fs_listxattr(struct dentry *dentry, char *buf, size_t size)
+{
+	struct inode *inode = d_inode(dentry);
+
+	return simple_xattr_list(inode, &BPF_FS_I(inode)->xattrs, buf, size);
+}
+
+static int bpf_fs_initxattrs(struct inode *inode,
+			     const struct xattr *xattr_array, void *fs_info)
+{
+	struct bpf_mount_opts *opts = inode->i_sb->s_fs_info;
+	struct bpf_fs_inode *bi = BPF_FS_I(inode);
+	const struct xattr *xattr;
+	int err;
+
+	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
+		CLASS(simple_xattr, new_xattr)(xattr->value, xattr->value_len);
+		if (IS_ERR(new_xattr))
+			return PTR_ERR(new_xattr);
+
+		new_xattr->name = kasprintf(GFP_KERNEL_ACCOUNT,
+					    XATTR_SECURITY_PREFIX "%s",
+					    xattr->name);
+		if (!new_xattr->name)
+			return -ENOMEM;
+
+		err = simple_xattr_add_limited(&opts->xa_cache, &bi->xattrs,
+					       &bi->xlimits, new_xattr);
+		if (err)
+			return err;
+
+		retain_and_null_ptr(new_xattr);
+	}
+	return 0;
 }
 
 const struct super_operations bpf_super_ops = {
 	.statfs		= simple_statfs,
 	.drop_inode	= inode_just_drop,
 	.show_options	= bpf_show_options,
+	.alloc_inode	= bpf_fs_alloc_inode,
 	.destroy_inode	= bpf_destroy_inode,
+	.free_inode	= bpf_free_inode,
 };
 
 enum {
@@ -996,25 +1181,38 @@ static int populate_bpffs(struct dentry *parent)
 
 static int bpf_fill_super(struct super_block *sb, struct fs_context *fc)
 {
-	static const struct tree_descr bpf_rfiles[] = { { "" } };
 	struct bpf_mount_opts *opts = sb->s_fs_info;
 	struct inode *inode;
-	int ret;
 
 	/* Mounting an instance of BPF FS requires privileges */
 	if (fc->user_ns != &init_user_ns && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
-	ret = simple_fill_super(sb, BPF_FS_MAGIC, bpf_rfiles);
-	if (ret)
-		return ret;
-
+	sb->s_blocksize = PAGE_SIZE;
+	sb->s_blocksize_bits = PAGE_SHIFT;
+	sb->s_magic = BPF_FS_MAGIC;
 	sb->s_op = &bpf_super_ops;
+	sb->s_xattr = bpf_fs_xattr_handlers;
+	sb->s_iflags |= SB_I_NOEXEC;
+	sb->s_iflags |= SB_I_NODEV;
+	sb->s_time_gran = 1;
+
+	inode = bpf_get_inode(sb, NULL, S_IFDIR | 0777);
+	if (IS_ERR(inode))
+		return PTR_ERR(inode);
+
+	inode->i_ino = 1;
+	inode->i_op = &bpf_dir_iops;
+	inode->i_fop = &simple_dir_operations;
+	set_nlink(inode, 2);
+
+	sb->s_root = d_make_root(inode);
+	if (!sb->s_root)
+		return -ENOMEM;
 
-	inode = sb->s_root->d_inode;
+	inode = d_inode(sb->s_root);
 	inode->i_uid = opts->uid;
 	inode->i_gid = opts->gid;
-	inode->i_op = &bpf_dir_iops;
 	inode->i_mode &= ~S_IALLUGO;
 	populate_bpffs(sb->s_root);
 	inode->i_mode |= S_ISVTX | opts->mode;
@@ -1068,6 +1266,7 @@ static void bpf_kill_super(struct super_block *sb)
 	struct bpf_mount_opts *opts = sb->s_fs_info;
 
 	kill_anon_super(sb);
+	simple_xattr_cache_cleanup(&opts->xa_cache);
 	kfree(opts);
 }
 
@@ -1080,18 +1279,37 @@ static struct file_system_type bpf_fs_type = {
 	.fs_flags	= FS_USERNS_MOUNT,
 };
 
+static void bpf_fs_inode_init_once(void *foo)
+{
+	struct bpf_fs_inode *bi = foo;
+
+	inode_init_once(&bi->vfs_inode);
+}
+
 static int __init bpf_init(void)
 {
 	int ret;
 
+	bpf_fs_inode_cachep = kmem_cache_create("bpf_fs_inode_cache",
+						sizeof(struct bpf_fs_inode),
+						0, SLAB_ACCOUNT,
+						bpf_fs_inode_init_once);
+	if (!bpf_fs_inode_cachep)
+		return -ENOMEM;
+
 	ret = sysfs_create_mount_point(fs_kobj, "bpf");
 	if (ret)
-		return ret;
+		goto out_cache;
 
 	ret = register_filesystem(&bpf_fs_type);
-	if (ret)
+	if (ret) {
 		sysfs_remove_mount_point(fs_kobj, "bpf");
+		goto out_cache;
+	}
 
+	return 0;
+out_cache:
+	kmem_cache_destroy(bpf_fs_inode_cachep);
 	return ret;
 }
 fs_initcall(bpf_init);
-- 
2.43.0

Re: [PATCH vfs/vfs-7.2.xattr v3] bpf: Add simple xattr support to bpffs

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 7734 bytes --]

> diff --git a/fs/xattr.c b/fs/xattr.c
> index 09ecbaaa1660..a7abb39f0041 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -1689,6 +1689,39 @@ static void simple_xattr_ht_free(void *ptr, void *arg)
>  	simple_xattr_free(xattr);
>  }
>
> +/**
> + * simple_xattr_add_limited - add an xattr object, charging per-inode limits
> + * @cache: anchor for the hash table
> + * @xattrs: the header of the xattr object
> + * @limits: per-inode limit counters
> + * @new_xattr: the xattr object to add
> + *
> + * Like simple_xattr_add(), but also accounts @new_xattr against @limits so
> + * that a later removal or replacement of it through simple_xattr_set_limited()
> + * decrements counters that were actually incremented, rather than underflowing
> + * them. Use this instead of simple_xattr_add() when seeding initial xattrs
> + * that share a namespace with the limited set/remove path.
> + *
> + * Return: On success zero is returned. On failure a negative error code is
> + * returned.
> + */
> +int simple_xattr_add_limited(struct simple_xattr_cache *cache,
> +			     struct list_head *xattrs,
> +			     struct simple_xattr_limits *limits,
> +			     struct simple_xattr *new_xattr)
> +{
> +	int err;
> +
> +	err = simple_xattr_limits_inc(limits, new_xattr->size);
> +	if (err)
> +		return err;
> +
> +	err = simple_xattr_add(cache, xattrs, new_xattr);
> +	if (err)
> +		simple_xattr_limits_dec(limits, new_xattr->size);
> +	return err;
> +}

Does simple_xattr_add() accept three arguments here? Looking at the existing
simple_xattr_add() signature in include/linux/xattr.h:

int simple_xattr_add(struct simple_xattrs *xattrs,
		     struct simple_xattr *new_xattr);

The in-tree function takes only two arguments with a single struct
simple_xattrs pointer. However, this new code is calling it with three
arguments of different types:

err = simple_xattr_add(cache, xattrs, new_xattr);

where cache is a struct simple_xattr_cache * and xattrs is a struct
list_head *. The existing implementation uses a rhashtable internally
(struct simple_xattrs contains struct rhashtable ht), not a separate cache
and list_head pair. Won't this fail to compile due to the argument count
and type mismatches?

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4b703c90ca9..434ba91401c6 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -31,6 +31,7 @@
>  #include <linux/static_call.h>
>  #include <linux/memcontrol.h>
>  #include <linux/cfi.h>
> +#include <linux/xattr.h>
>  #include <asm/rqspinlock.h>
>
>  struct bpf_verifier_env;
> @@ -1918,6 +1919,8 @@ struct bpf_mount_opts {
>  	u64 delegate_maps;
>  	u64 delegate_progs;
>  	u64 delegate_attachs;
> +
> +	struct simple_xattr_cache xa_cache;
>  };

Is struct simple_xattr_cache defined anywhere? Looking through
include/linux/xattr.h, the VFS simple_xattr infrastructure defines struct
simple_xattrs, struct simple_xattr, and struct simple_xattr_limits, but
there is no struct simple_xattr_cache type. Embedding an undefined struct
by value would be a compile error. Are the VFS xattr headers expected to
provide this type?

> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index 8b6601367eae..5ee49199eb22 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -154,6 +154,10 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
>  			  char *buffer, size_t size);
>  int simple_xattr_add(struct simple_xattrs *xattrs,
>  		     struct simple_xattr *new_xattr);
> +int simple_xattr_add_limited(struct simple_xattr_cache *cache,
> +			     struct list_head *xattrs,
> +			     struct simple_xattr_limits *limits,
> +			     struct simple_xattr *new_xattr);
>  int xattr_list_one(char **buffer, ssize_t *remaining_size, const char *name);

Following up on the previous point - this declaration references struct
simple_xattr_cache in its first parameter, but that type is not defined in
this header or anywhere else in the tree. Won't this create an incomplete
type that causes compilation failures when the function implementation
attempts to dereference the cache pointer?

Looking at other simple_xattr functions like simple_xattr_get/set/list, they
all take struct simple_xattrs * as their first parameter rather than a
separate cache pointer and list_head. Was there a separate patch series that
introduces the cache-based API that this depends on?

> diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
> index 25c06a011825..c3f79b5a2f8c 100644
> --- a/kernel/bpf/inode.c
> +++ b/kernel/bpf/inode.c

[ ... ]

> @@ -762,22 +822,147 @@ static int bpf_show_options(struct seq_file *m, struct dentry *root)
>  	return 0;
>  }

[ ... ]

>  static void bpf_destroy_inode(struct inode *inode)
>  {
> +	struct bpf_mount_opts *opts = inode->i_sb->s_fs_info;
> +	struct bpf_fs_inode *bi = BPF_FS_I(inode);
>  	enum bpf_type type;
>
> -	if (S_ISLNK(inode->i_mode))
> -		kfree(inode->i_link);
>  	if (!bpf_inode_type(inode, &type))
>  		bpf_any_put(inode->i_private, type);
> -	free_inode_nonrcu(inode);
> +	simple_xattrs_free(&opts->xa_cache, &bi->xattrs, NULL);
>  }

Does simple_xattrs_free() take three arguments? Looking at the in-tree
implementation, simple_xattrs_free takes two parameters:

void simple_xattrs_free(struct simple_xattrs *xattrs, size_t *freed_space)

But this call is passing three arguments: a cache pointer, a list_head
pointer, and NULL. This appears to be the same API mismatch - won't this
fail to compile?

Similarly, throughout the bpffs xattr handlers (bpf_fs_xattr_get,
bpf_fs_xattr_set, bpf_fs_listxattr, bpf_fs_initxattrs), the code is calling
simple_xattr functions with an extra leading &opts->xa_cache parameter:

simple_xattr_get(&opts->xa_cache, &bi->xattrs, name, value, size)
simple_xattr_set(&opts->xa_cache, &bi->xattrs, name, ...)
simple_xattr_set_limited(&opts->xa_cache, &bi->xattrs, &bi->xlimits, ...)
simple_xattr_list(inode, &BPF_FS_I(inode)->xattrs, buf, size)
simple_xattr_add_limited(&opts->xa_cache, &bi->xattrs, &bi->xlimits, ...)

where bi->xattrs is a struct list_head. However, the existing VFS functions
expect struct simple_xattrs * as their first parameter (a structure
containing an rhashtable, not a list_head). For comparison, fs/kernfs/inode.c
uses simple_xattr_set_limited like:

simple_xattr_set_limited(xattrs, &attrs->xattr_limits, ...)

with just the struct simple_xattrs pointer. Are all these calls expecting a
different API than what currently exists in fs/xattr.c?

[ ... ]

> @@ -1068,6 +1266,7 @@ static void bpf_kill_super(struct super_block *sb)
>  	struct bpf_mount_opts *opts = sb->s_fs_info;
>
>  	kill_anon_super(sb);
> +	simple_xattr_cache_cleanup(&opts->xa_cache);
>  	kfree(opts);
>  }

Is simple_xattr_cache_cleanup() defined? A tree-wide search doesn't show any
definition of this function - only this call site. Won't this cause an
undefined reference at link time?

All of these issues appear related: the patch adds a cache-based simple_xattr
API (struct simple_xattr_cache, simple_xattr_add_limited with cache
parameter, simple_xattr_cache_cleanup) and calls it throughout the bpffs
xattr handlers, but the VFS side (fs/xattr.c and include/linux/xattr.h) only
partially implements this API. The struct simple_xattr_cache type is never
defined, simple_xattr_add_limited's implementation calls the two-argument
simple_xattr_add with three arguments, and simple_xattr_cache_cleanup is
never defined. Does this depend on a separate VFS xattr rework that hasn't
been included?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/26835946342

Re: [PATCH vfs/vfs-7.2.xattr v3] bpf: Add simple xattr support to bpffs

[Daniel Borkmann]

On 6/2/26 8:07 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index 09ecbaaa1660..a7abb39f0041 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -1689,6 +1689,39 @@ static void simple_xattr_ht_free(void *ptr, void *arg)
>>   	simple_xattr_free(xattr);
>>   }
>>
>> +/**
>> + * simple_xattr_add_limited - add an xattr object, charging per-inode limits
>> + * @cache: anchor for the hash table
>> + * @xattrs: the header of the xattr object
>> + * @limits: per-inode limit counters
>> + * @new_xattr: the xattr object to add
>> + *
>> + * Like simple_xattr_add(), but also accounts @new_xattr against @limits so
>> + * that a later removal or replacement of it through simple_xattr_set_limited()
>> + * decrements counters that were actually incremented, rather than underflowing
>> + * them. Use this instead of simple_xattr_add() when seeding initial xattrs
>> + * that share a namespace with the limited set/remove path.
>> + *
>> + * Return: On success zero is returned. On failure a negative error code is
>> + * returned.
>> + */
>> +int simple_xattr_add_limited(struct simple_xattr_cache *cache,
>> +			     struct list_head *xattrs,
>> +			     struct simple_xattr_limits *limits,
>> +			     struct simple_xattr *new_xattr)
>> +{
>> +	int err;
>> +
>> +	err = simple_xattr_limits_inc(limits, new_xattr->size);
>> +	if (err)
>> +		return err;
>> +
>> +	err = simple_xattr_add(cache, xattrs, new_xattr);
>> +	if (err)
>> +		simple_xattr_limits_dec(limits, new_xattr->size);
>> +	return err;
>> +}
> 
> Does simple_xattr_add() accept three arguments here? Looking at the existing
> simple_xattr_add() signature in include/linux/xattr.h:
> 
> int simple_xattr_add(struct simple_xattrs *xattrs,
> 		     struct simple_xattr *new_xattr);
(this is on top of https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs-7.2.xattr)

Re: [PATCH vfs/vfs-7.2.xattr v3] bpf: Add simple xattr support to bpffs

[Christian Brauner]

On Tue, 02 Jun 2026 09:40:12 +0200, Daniel Borkmann wrote:
> Add support for extended attributes on bpffs inodes so that user space
> and BPF LSM programs can attach metadata, for example, a content hash
> or a security label - to a pinned object or directory. BPF LSM or user
> space tooling can then uniformly look at this (e.g. security.bpf.*) in
> similar way to other fs'es. The store is in-memory and non-persistent:
> it lives only for the lifetime of the mount, like everything else in
> bpffs. The modelling is similar to tmpfs.
> 
> [...]

Applied to the vfs-7.2.xattr branch of the vfs/vfs.git tree.
Patches in the vfs-7.2.xattr branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-7.2.xattr

[1/1] bpf: Add simple xattr support to bpffs
      https://git.kernel.org/vfs/vfs/c/a146500e1144

Re: [PATCH vfs/vfs-7.2.xattr v3] bpf: Add simple xattr support to bpffs

[Carlos Llamas]

On Wed, Jun 03, 2026 at 09:07:55AM +0200, Christian Brauner wrote:
> On Tue, 02 Jun 2026 09:40:12 +0200, Daniel Borkmann wrote:
> > Add support for extended attributes on bpffs inodes so that user space
> > and BPF LSM programs can attach metadata, for example, a content hash
> > or a security label - to a pinned object or directory. BPF LSM or user
> > space tooling can then uniformly look at this (e.g. security.bpf.*) in
> > similar way to other fs'es. The store is in-memory and non-persistent:
> > it lives only for the lifetime of the mount, like everything else in
> > bpffs. The modelling is similar to tmpfs.
> > 
> > [...]
> 
> Applied to the vfs-7.2.xattr branch of the vfs/vfs.git tree.
> Patches in the vfs-7.2.xattr branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs-7.2.xattr
> 
> [1/1] bpf: Add simple xattr support to bpffs
>       https://git.kernel.org/vfs/vfs/c/a146500e1144

Hey Christian / Daniel,

I'm getting a regression with this patch when using genfs for bpffs. It
seems like calling selinux_inode_init_security() with !SBLABEL_MNT will
leave the isec->initialized before doing the EOPNOTSUPP exit.

The genfs path is later skipped because inode_doinit_with_dentry() sees
the inode initialized and I get denials in userspace.

I don't know if this would be correct but doing the check for
SBLABEL_MNT before marking the isec as initialized worked for me.

Any thoughts? Should I send a formal patch?

Carlos Llamas

---
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0f704380a8c8..9cb1724d2bbc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2980,6 +2980,10 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 	if (rc)
 		return rc;
 
+	if (!selinux_initialized() ||
+	    !(sbsec->flags & SBLABEL_MNT))
+		return -EOPNOTSUPP;
+
 	/* Possibly defer initialization to selinux_complete_init. */
 	if (sbsec->flags & SE_SBINITIALIZED) {
 		struct inode_security_struct *isec = selinux_inode(inode);
@@ -2988,10 +2992,6 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
 		isec->initialized = LABEL_INITIALIZED;
 	}
 
-	if (!selinux_initialized() ||
-	    !(sbsec->flags & SBLABEL_MNT))
-		return -EOPNOTSUPP;
-
 	xattr = lsm_get_xattr_slot(xattrs, xattr_count);
 	if (xattr) {
 		rc = security_sid_to_context_force(newsid,