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@xxxxxxxxxxxxxxxxx>
>>>> ---
>>>> 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
