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

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

[Jeffle Xu]

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)) &&
 	    !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
 		return -EINVAL;
 
-- 
2.27.0

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

[Darrick J. Wong]

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?

--D

>  	    !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
>  		return -EINVAL;
>  
> -- 
> 2.27.0
> 

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

[JeffleXu]

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.
Here we want to check *new* flags/xflags.

> 
>>  	    !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
>>  		return -EINVAL;
>>  
>> -- 
>> 2.27.0
>>

-- 
Thanks,
Jeffle

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

[Darrick J. Wong]

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

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

[JeffleXu]

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