Hi Christian, 

Thanks for the review!

On Jan 24, 2025, at 1:05 AM, Christian Brauner [off-list ref] wrote:

On Thu, Jan 09, 2025 at 05:13:41PM -0800, Song Liu wrote:

quoted

Add the following kfuncs to set and remove xattrs from BPF programs:

 bpf_set_dentry_xattr
 bpf_remove_dentry_xattr
 bpf_set_dentry_xattr_locked
 bpf_remove_dentry_xattr_locked

The _locked version of these kfuncs are called from hooks where
dentry->d_inode is already locked. Instead of requiring the user
to know which version of the kfuncs to use, the verifier will pick
the proper kfunc based on the calling hook.

Signed-off-by: Song Liu <song@kernel.org>

[...]

quoted

+
+ ret = __vfs_setxattr(&nop_mnt_idmap, dentry, inode, name,
+      value, value_len, flags);
+ if (!ret) {
+ fsnotify_xattr(dentry);
+
+ /* This xattr is set by BPF LSM, so we do not call
+  * security_inode_post_setxattr. This is the same as
+  * security_inode_setsecurity().
+  */

If you did you would risk deadlocks as you could end up calling yourself
again afaict.

Exactly. Let state it more clearly in the comment. 

quoted

+ }
+out:
+ if (lock_inode)
+ inode_unlock(inode);
+ return ret;
+}
+
+/**
+ * bpf_set_dentry_xattr - set a xattr of a dentry
+ * @dentry: dentry to get xattr from
+ * @name__str: name of the xattr
+ * @value_p: xattr value
+ * @flags: flags to pass into filesystem operations
+ *
+ * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
+ *
+ * For security reasons, only *name__str* with prefix "security.bpf."
+ * is allowed.
+ *
+ * The caller has not locked dentry->d_inode.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_set_dentry_xattr(struct dentry *dentry, const char *name__str,
+      const struct bpf_dynptr *value_p, int flags)
+{
+ return __bpf_set_dentry_xattr(dentry, name__str, value_p, flags, true);
+}
+
+/**
+ * bpf_set_dentry_xattr_locked - set a xattr of a dentry
+ * @dentry: dentry to get xattr from
+ * @name__str: name of the xattr
+ * @value_p: xattr value
+ * @flags: flags to pass into filesystem operations
+ *
+ * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
+ *
+ * For security reasons, only *name__str* with prefix "security.bpf."
+ * is allowed.
+ *
+ * The caller already locked dentry->d_inode.
+ *
+ * Return: 0 on success, a negative value on error.
+ */
+__bpf_kfunc int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
+     const struct bpf_dynptr *value_p, int flags)
+{
+ return __bpf_set_dentry_xattr(dentry, name__str, value_p, flags, false);

That boolean argument is not needed if you pull

value_len = __bpf_dynptr_size(value_ptr);
value = __bpf_dynptr_data(value_ptr, value_len);
if (!value)
return -EINVAL;

into the two functions and then put:

inode_lock()
bpf_set_dentry_xattr_unlocked();
inode_unlock()

for the locked variant. Similar comment applied to the remove functions.

Sounds good. Let me update them in the next version. 

Song


[...]