[PATCH] xfs: correct statx's result_mask value

[PATCH] xfs: correct statx's result_mask value

[Su Yanjun]

For statx syscall, xfs return the wrong result_mask.

Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
---
 fs/xfs/xfs_iops.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index f48ffd7..3811457 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -521,6 +521,9 @@ xfs_vn_getattr(
 			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
 		}
 	}
+	
+	/* Only return mask that we care */
+	stat->result_mask &= request_mask;
 
 	if (ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE)
 		stat->attributes |= STATX_ATTR_IMMUTABLE;
-- 
2.7.4

Re: [PATCH] xfs: correct statx's result_mask value

[Darrick J. Wong]

On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
> For statx syscall, xfs return the wrong result_mask.
> 
> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
> ---
>  fs/xfs/xfs_iops.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index f48ffd7..3811457 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -521,6 +521,9 @@ xfs_vn_getattr(
>  			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
>  		}
>  	}
> +	
> +	/* Only return mask that we care */
> +	stat->result_mask &= request_mask;

Why not just:

	stat->result_mask = STATX_BASIC_STATS;

at the top of the function?

I don't see the need to mask off result_mask at all, since we could some
day elect to return more than what's in request_mask...

...waitaminute, are you seeing garbage in the result_mask that's
returned to userspace?  I also noticed the vfs stat functions declare
"struct kstat stat;" without explicitly zeroing the structure fields,
which means (I think) that we can leak stack information if the kernel
isn't built with the stackleak plugin?

--D

>  
>  	if (ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE)
>  		stat->attributes |= STATX_ATTR_IMMUTABLE;
> -- 
> 2.7.4
> 
> 
> 

Re: [PATCH] xfs: correct statx's result_mask value

[Darrick J. Wong]

On Mon, Jan 07, 2019 at 09:52:29AM -0800, Darrick J. Wong wrote:
> On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
> > For statx syscall, xfs return the wrong result_mask.
> > 
> > Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
> > ---
> >  fs/xfs/xfs_iops.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index f48ffd7..3811457 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -521,6 +521,9 @@ xfs_vn_getattr(
> >  			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
> >  		}
> >  	}
> > +	
> > +	/* Only return mask that we care */
> > +	stat->result_mask &= request_mask;
> 
> Why not just:
> 
> 	stat->result_mask = STATX_BASIC_STATS;
> 
> at the top of the function?
> 
> I don't see the need to mask off result_mask at all, since we could some
> day elect to return more than what's in request_mask...
> 
> ...waitaminute, are you seeing garbage in the result_mask that's
> returned to userspace?  I also noticed the vfs stat functions declare
> "struct kstat stat;" without explicitly zeroing the structure fields,
> which means (I think) that we can leak stack information if the kernel
> isn't built with the stackleak plugin?

Ignore the above; vfs_getattr_nosec actually does zero the kstat buffer
before calling ->getattr.  We also set result_mask to STATX_BASIC_STATS.

Now I'm really confused: why is this necessary at all?  What incorrect
masks did you see, and under what circumstances?

--D

> --D
> 
> >  
> >  	if (ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE)
> >  		stat->attributes |= STATX_ATTR_IMMUTABLE;
> > -- 
> > 2.7.4
> > 
> > 
> > 

Re: [PATCH] xfs: correct statx's result_mask value

[Eric Sandeen]

On 1/7/19 11:52 AM, Darrick J. Wong wrote:
> On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
>> For statx syscall, xfs return the wrong result_mask.
>>
>> Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com>
>> ---
>>  fs/xfs/xfs_iops.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>> index f48ffd7..3811457 100644
>> --- a/fs/xfs/xfs_iops.c
>> +++ b/fs/xfs/xfs_iops.c
>> @@ -521,6 +521,9 @@ xfs_vn_getattr(
>>  			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
>>  		}
>>  	}
>> +	
>> +	/* Only return mask that we care */
>> +	stat->result_mask &= request_mask;
> 
> Why not just:
> 
> 	stat->result_mask = STATX_BASIC_STATS;
> 
> at the top of the function?
> 
> I don't see the need to mask off result_mask at all, since we could some
> day elect to return more than what's in request_mask...
> 
> ...waitaminute, are you seeing garbage in the result_mask that's
> returned to userspace?  I also noticed the vfs stat functions declare
> "struct kstat stat;" without explicitly zeroing the structure fields,
> which means (I think) that we can leak stack information if the kernel
> isn't built with the stackleak plugin?

A clear problem statement and reproducer steps would be hugely useful
here.

-Eric

Re: [PATCH] xfs: correct statx's result_mask value

[Su Yanjun <suyj.fnst@cn.fujitsu.com>]

On 1/8/2019 2:04 AM, Eric Sandeen wrote:
> On 1/7/19 11:52 AM, Darrick J. Wong wrote:
>> On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
>>> For statx syscall, xfs return the wrong result_mask.
>>>
>>> Signed-off-by: Su Yanjun<suyj.fnst@cn.fujitsu.com>
>>> ---
>>>   fs/xfs/xfs_iops.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>>> index f48ffd7..3811457 100644
>>> --- a/fs/xfs/xfs_iops.c
>>> +++ b/fs/xfs/xfs_iops.c
>>> @@ -521,6 +521,9 @@ xfs_vn_getattr(
>>>   			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
>>>   		}
>>>   	}
>>> +	
>>> +	/* Only return mask that we care */
>>> +	stat->result_mask &= request_mask;
>> Why not just:
>>
>> 	stat->result_mask = STATX_BASIC_STATS;
>>
>> at the top of the function?
>>
>> I don't see the need to mask off result_mask at all, since we could some
>> day elect to return more than what's in request_mask...
When we run xfstests with nfs, the generic/423 case runs failed. So i 
review the nfs'
nfs_getattr code it does validate the request_mask.

Then i review the xfs' getattr code, it has no such check. Whatever 
request_mask
  is set, the stat's result_mask always the 0x7ff.

Maybe it has Unclear semantics about statx's result_mask.
>> ...waitaminute, are you seeing garbage in the result_mask that's
>> returned to userspace?  I also noticed the vfs stat functions declare
>> "struct kstat stat;" without explicitly zeroing the structure fields,
>> which means (I think) that we can leak stack information if the kernel
>> isn't built with the stackleak plugin?
No such problem.
> A clear problem statement and reproducer steps would be hugely useful
> here.
>
> -Eric
Thanks,
Su

Re: [PATCH] xfs: correct statx's result_mask value

[Darrick J. Wong]

On Tue, Jan 08, 2019 at 12:58:43PM +0800, Su Yanjun <suyj.fnst@cn.fujitsu.com> wrote:
> 
> 
> On 1/8/2019 2:04 AM, Eric Sandeen wrote:
> > On 1/7/19 11:52 AM, Darrick J. Wong wrote:
> > > On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
> > > > For statx syscall, xfs return the wrong result_mask.
> > > > 
> > > > Signed-off-by: Su Yanjun<suyj.fnst@cn.fujitsu.com>
> > > > ---
> > > >   fs/xfs/xfs_iops.c | 3 +++
> > > >   1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > > > index f48ffd7..3811457 100644
> > > > --- a/fs/xfs/xfs_iops.c
> > > > +++ b/fs/xfs/xfs_iops.c
> > > > @@ -521,6 +521,9 @@ xfs_vn_getattr(
> > > >   			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
> > > >   		}
> > > >   	}
> > > > +	
> > > > +	/* Only return mask that we care */
> > > > +	stat->result_mask &= request_mask;
> > > Why not just:
> > > 
> > > 	stat->result_mask = STATX_BASIC_STATS;
> > > 
> > > at the top of the function?
> > > 
> > > I don't see the need to mask off result_mask at all, since we could some
> > > day elect to return more than what's in request_mask...
> When we run xfstests with nfs, the generic/423 case runs failed. So i review
> the nfs'
> nfs_getattr code it does validate the request_mask.
> 
> Then i review the xfs' getattr code, it has no such check. Whatever
> request_mask
>  is set, the stat's result_mask always the 0x7ff.

Yes, statx can return more data than what userspace callers ask for:

> Maybe it has Unclear semantics about statx's result_mask.

"A filesystem may also fill in fields that the caller didn't ask for if
it has values for them available and the information is available at no
extra cost.  If this happens, the  corresponding  bits will be set in
stx_mask."

--D

> > > ...waitaminute, are you seeing garbage in the result_mask that's
> > > returned to userspace?  I also noticed the vfs stat functions declare
> > > "struct kstat stat;" without explicitly zeroing the structure fields,
> > > which means (I think) that we can leak stack information if the kernel
> > > isn't built with the stackleak plugin?
> No such problem.
> > A clear problem statement and reproducer steps would be hugely useful
> > here.
> > 
> > -Eric
> Thanks,
> Su
> 
> 

Re: [PATCH] xfs: correct statx's result_mask value

[Su Yanjun <suyj.fnst@cn.fujitsu.com>]

On 1/8/2019 1:07 PM, Darrick J. Wong wrote:
> On Tue, Jan 08, 2019 at 12:58:43PM +0800, Su Yanjun <suyj.fnst@cn.fujitsu.com> wrote:
>>
>> On 1/8/2019 2:04 AM, Eric Sandeen wrote:
>>> On 1/7/19 11:52 AM, Darrick J. Wong wrote:
>>>> On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
>>>>> For statx syscall, xfs return the wrong result_mask.
>>>>>
>>>>> Signed-off-by: Su Yanjun<suyj.fnst@cn.fujitsu.com>
>>>>> ---
>>>>>    fs/xfs/xfs_iops.c | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>>>>> index f48ffd7..3811457 100644
>>>>> --- a/fs/xfs/xfs_iops.c
>>>>> +++ b/fs/xfs/xfs_iops.c
>>>>> @@ -521,6 +521,9 @@ xfs_vn_getattr(
>>>>>    			stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
>>>>>    		}
>>>>>    	}
>>>>> +	
>>>>> +	/* Only return mask that we care */
>>>>> +	stat->result_mask &= request_mask;
>>>> Why not just:
>>>>
>>>> 	stat->result_mask = STATX_BASIC_STATS;
>>>>
>>>> at the top of the function?
>>>>
>>>> I don't see the need to mask off result_mask at all, since we could some
>>>> day elect to return more than what's in request_mask...
>> When we run xfstests with nfs, the generic/423 case runs failed. So i review
>> the nfs'
>> nfs_getattr code it does validate the request_mask.
>>
>> Then i review the xfs' getattr code, it has no such check. Whatever
>> request_mask
>>   is set, the stat's result_mask always the 0x7ff.
> Yes, statx can return more data than what userspace callers ask for:
>
>> Maybe it has Unclear semantics about statx's result_mask.
> "A filesystem may also fill in fields that the caller didn't ask for if
> it has values for them available and the information is available at no
> extra cost.  If this happens, the  corresponding  bits will be set in
> stx_mask."
>
> --D

I get it, then the testcase generic/423 may need update in xfstests.
Thanks for your reply.

>>>> ...waitaminute, are you seeing garbage in the result_mask that's
>>>> returned to userspace?  I also noticed the vfs stat functions declare
>>>> "struct kstat stat;" without explicitly zeroing the structure fields,
>>>> which means (I think) that we can leak stack information if the kernel
>>>> isn't built with the stackleak plugin?
>> No such problem.
>>> A clear problem statement and reproducer steps would be hugely useful
>>> here.
>>>
>>> -Eric
>> Thanks,
>> Su
>>
>>
>

Re: [PATCH] xfs: correct statx's result_mask value

[Eric Sandeen]

On 1/7/19 11:15 PM, Su Yanjun <suyj.fnst@cn.fujitsu.com> wrote:
> 
> 
> On 1/8/2019 1:07 PM, Darrick J. Wong wrote:
>> On Tue, Jan 08, 2019 at 12:58:43PM +0800, Su Yanjun <suyj.fnst@cn.fujitsu.com> wrote:
>>>
>>> On 1/8/2019 2:04 AM, Eric Sandeen wrote:
>>>> On 1/7/19 11:52 AM, Darrick J. Wong wrote:
>>>>> On Mon, Jan 07, 2019 at 04:53:10AM -0500, Su Yanjun wrote:
>>>>>> For statx syscall, xfs return the wrong result_mask.
>>>>>>
>>>>>> Signed-off-by: Su Yanjun<suyj.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>    fs/xfs/xfs_iops.c | 3 +++
>>>>>>    1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
>>>>>> index f48ffd7..3811457 100644
>>>>>> --- a/fs/xfs/xfs_iops.c
>>>>>> +++ b/fs/xfs/xfs_iops.c
>>>>>> @@ -521,6 +521,9 @@ xfs_vn_getattr(
>>>>>>                stat->btime.tv_nsec = ip->i_d.di_crtime.t_nsec;
>>>>>>            }
>>>>>>        }
>>>>>> +   
>>>>>> +    /* Only return mask that we care */
>>>>>> +    stat->result_mask &= request_mask;
>>>>> Why not just:
>>>>>
>>>>>     stat->result_mask = STATX_BASIC_STATS;
>>>>>
>>>>> at the top of the function?
>>>>>
>>>>> I don't see the need to mask off result_mask at all, since we could some
>>>>> day elect to return more than what's in request_mask...
>>> When we run xfstests with nfs, the generic/423 case runs failed. So i review
>>> the nfs'
>>> nfs_getattr code it does validate the request_mask.
>>>
>>> Then i review the xfs' getattr code, it has no such check. Whatever
>>> request_mask
>>>   is set, the stat's result_mask always the 0x7ff.
>> Yes, statx can return more data than what userspace callers ask for:
>>
>>> Maybe it has Unclear semantics about statx's result_mask.
>> "A filesystem may also fill in fields that the caller didn't ask for if
>> it has values for them available and the information is available at no
>> extra cost.  If this happens, the  corresponding  bits will be set in
>> stx_mask."
>>
>> --D
> 
> I get it, then the testcase generic/423 may need update in xfstests.
> Thanks for your reply.

Can you please share the details of the failure you're seeing when you run
it over nfs?

Thanks,
-=Eric