linux-kernel - Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fbc3501c07fd48e65c0ffc8634870bf57a6f7fe1.camel@hammerspace.com>
Date:   Sat, 20 Oct 2018 17:46:51 +0000
From:   Trond Myklebust <trondmy@...merspace.com>
To:     "miklos@...redi.hu" <miklos@...redi.hu>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "amir73il@...il.com" <amir73il@...il.com>,
        "mszeredi@...hat.com" <mszeredi@...hat.com>,
        "adilger@...ger.ca" <adilger@...ger.ca>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "dhowells@...hat.com" <dhowells@...hat.com>,
        "fw@...eb.enyo.de" <fw@...eb.enyo.de>,
        "mtk.manpages@...il.com" <mtk.manpages@...il.com>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask

On Fri, 2018-10-19 at 22:48 +0200, Miklos Szeredi wrote:
> On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust
> <trondmy@...merspace.com> wrote:
> > On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
> > > How is it then that only STATX_ATIME is cleared and not the other
> > > fields?
> > 
> > It isn't just the atime. We can also fail to revalidate the ctime
> > and
> > mtime if they are not being requested by the user.
> > 
> > > 
> > > Note: junk != stale.  The statx definition doesn't talk about the
> > > fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
> > > attributes are okay, and do not warrant clearing the result_mask.
> > > 
> > 
> > I disagree. stale == junk here, because the default of
> > AT_STATX_SYNC_AS_STAT is described by the manpage as "Do  whatever
> > stat(2) does." which this is not.
> 
> Ah, you are talking about this:
> 
>     /* Is the user requesting attributes that might need
> revalidation? */
>     if (!(request_mask &
> (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
>                     STATX_MTIME|STATX_UID|STATX_GID|
>                     STATX_SIZE|STATX_BLOCKS)))
>         goto out_no_update;
> 
> Well, if this is triggered for statx(...,  STATX_ATIME,
> AT_STATX_SYNC_AS_STAT) and MNT_NOATIME, then yes, result will be
> junk.
> Which means that the code is wrong, it shouldn't do that.

The problem is that vfs_getattr_nosec() populates stat->result_mask
with a default of STATX_BASIC_STATS, which makes no sense unless you
assume that the user will always ask for a superset of
STATX_BASIC_STATS (or you assume that those attributes never need
revalidation, which is obviously braindead).

> Otherwise (if something other than STATX_ATIME or STATX_INO or
> STATX_TYPE is given as well) it *will* do the same thing as what
> stat(2) does, so in that case STATX_ATIME should not  be cleared (yet
> it is cleared).

As far as I'm concerned, we can definitely get rid of the

        /*
         * We may force a getattr if the user cares about atime.
         *
         * Note that we only have to check the vfsmount flags here:
         *  - NFS always sets S_NOATIME by so checking it would give a
         *    bogus result
         *  - NFS never sets SB_NOATIME or SB_NODIRATIME so there is
         *    no point in checking those.
         */
        if ((path->mnt->mnt_flags & MNT_NOATIME) ||
            ((path->mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode)))
                request_mask &= ~STATX_ATIME;


however the rest needs to stay, or there is no way we can use statx()
to allow optimised retrieval of only those attributes that your
application cares about.
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@...merspace.com