Hi Christian,
Thanks for the review!
> On Jan 24, 2025, at 1:05 AM, Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Thu, Jan 09, 2025 at 05:13:41PM -0800, Song Liu wrote:
>> 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@xxxxxxxxxx>
[...]
>> +
>> + 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.
>
>> + }
>> +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
[...]
