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