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

[Su Yanjun <suyj.fnst@xxxxxxxxxxxxxx>]

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

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