Re: [PATCH v2] vfs: only allow SETFLAGS to set DAX flag on files and dirs

[JeffleXu <jefflexu@linux.alibaba.com>]

On 7/22/21 7:28 AM, Darrick J. Wong wrote:
> On Tue, Jul 20, 2021 at 03:33:20PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 1:43 AM, Darrick J. Wong wrote:
>>> On Mon, Jul 19, 2021 at 10:38:34AM +0800, Jeffle Xu wrote:
>>>> This is similar to commit dbc77f31e58b ("vfs: only allow FSSETXATTR to
>>>> set DAX flag on files and dirs").
>>>>
>>>> Though the underlying filesystems may have filtered invalid flags, e.g.,
>>>> ext4_mask_flags() called in ext4_fileattr_set(), also check it in VFS
>>>> layer.
>>>>
>>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>>>> ---
>>>> changes since v1:
>>>> - add separate parentheses surrounding flag tests
>>>> ---
>>>>  fs/ioctl.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/ioctl.c b/fs/ioctl.c
>>>> index 1e2204fa9963..90cfaa4db03a 100644
>>>> --- a/fs/ioctl.c
>>>> +++ b/fs/ioctl.c
>>>> @@ -835,7 +835,7 @@ static int fileattr_set_prepare(struct inode *inode,
>>>>  	 * It is only valid to set the DAX flag on regular files and
>>>>  	 * directories on filesystems.
>>>>  	 */
>>>> -	if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
>>>> +	if (((fa->fsx_xflags & FS_XFLAG_DAX) || (fa->flags & FS_DAX_FL)) &&
>>>
>>> Isn't fileattr_fill_flags supposed to fill out fa->fsx_xflags from
>>> fa->flags for a SETFLAGS call?
>>
>> Yes, but fa->fsx_xflags inherited from fa->flags (at least in ext4 it
>> is) is the original flags/xflags of the file before SETFLAG/FSSETXATTR.
> 
> How?  old_ma is the original flags/xflags of the file.  fa reflects what
> we copied in from userspace.  We use old_ma to set flags in fa that
> couldn't possibly have been set by userspace, but neither DAX flag is in
> that set.
> 
> Ugh, this is so much bookkeeping code to read it makes my head hurt.  Do
> you have a reproducer?  I can't figure out how to trip this problem.
> 
>> Here we want to check *new* flags/xflags.
> 
> AFAICT, SETFLAGS will call ioctl_setflags, which will...
> ...read flags from userspace

> ...fill out struct fileattr via fileattr_fill_flags, which will set
>    fa.fsx_flags from fa.flags, so the state of both fields' DAX flags
>    will be whatever userspace gave us

Sorry I omitted this step and mistakenly thought that fa.fsx_flags was
*completely* copied from old_ma.fsx_xflags...

When calling SETFLAGS ioctl, FS_DAX_FL will still be checked by
following code snippet from fileattr_set_prepare().

```c
	/*
	 * It is only valid to set the DAX flag on regular files and
	 * directories on filesystems.
	 */
	if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
	    !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
		return -EINVAL;
```

I didn't encountered the issue in real environment. I thought it was a
simple fix while I was reading the code...

Sorry for the noise and really thanks to the detailed clarification.


Thanks
Jeffle


> ...call vfs_fileattr_set, which will...
> ...call vfs_fileattr_get to fill out out_ma
> ...update the rest of xflags with the xflags from out_ma that weren't
>    already set
> ...call fileattr_set_prepare, where it shouldn't matter if it checks
>    (fa->xflags & FS_XFLAG_DAX) or (fa->flags & FS_DAX_FL), since they
>    have the same value
> 
> FSSETXATTR will call ioctl_fssetxattr, which will...
> ...call copy_fsxattr_from_user to read fsxattr from userspace
> ...call fileattr_fill_xflags to set fa->flags from fa->xflags, so the
>    state of both fields' DAX flags will be whatever userspace gave us
> ...call vfs_fileattr_set, which will...
> ...call vfs_fileattr_get to fill out out_ma
> ...update the rest of flags with the flags from out_ma that weren't
>    already set
> ...call fileattr_set_prepare, where it shouldn't matter if it checks
>    (fa->xflags & FS_XFLAG_DAX) or (fa->flags & FS_DAX_FL), since they
>    have the same value
> 
> So where did I go wrong?
> 
> --D
> 
>>
>>>
>>>>  	    !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
>>>>  		return -EINVAL;
>>>>  
>>>> -- 
>>>> 2.27.0
>>>>
>>
>> -- 
>> Thanks,
>> Jeffle

-- 
Thanks,
Jeffle