Re: [PATCH v9 bpf-next 6/7] bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs

[Song Liu <songliubraving@meta.com>]

Hi Christian, 

Thanks for the review!

> On Jan 24, 2025, at 1:05 AM, Christian Brauner <brauner@kernel.org> 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@kernel.org>

[...]

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


[...]