Re: [PATCH 0/2] NFS: limit use of ACCESS cache for negative responses

["NeilBrown" <neilb@suse.de>]

On Tue, 30 Aug 2022, Jeff Layton wrote:
> On Mon, 2022-08-29 at 09:32 +1000, NeilBrown wrote:
> > On Sat, 27 Aug 2022, Trond Myklebust wrote:
> > > On Sat, 2022-08-27 at 09:39 +1000, NeilBrown wrote:
> > > > On Sat, 27 Aug 2022, Trond Myklebust wrote:
> > > > > On Fri, 2022-08-26 at 10:59 -0400, Benjamin Coddington wrote:
> > > > > > On 16 May 2022, at 21:36, Trond Myklebust wrote:
> > > > > > > So until you have a different solution that doesn't impact the
> > > > > > > client's
> > > > > > > ability to cache permissions, then the answer is going to be
> > > > > > > "no"
> > > > > > > to
> > > > > > > these patches.
> > > > > > 
> > > > > > Hi Trond,
> > > > > > 
> > > > > > We have some folks negatively impacted by this issue as well. 
> > > > > > Are
> > > > > > you
> > > > > > willing to consider this via a mount option?
> > > > > > 
> > > > > > Ben
> > > > > > 
> > > > > 
> > > > > I don't see how that answers my concern.
> > > > 
> > > > Could you please spell out again what your concerns are?  I still
> > > > don't
> > > > understand. 
> > > > The only performance impact is when a permission test fails.  In what
> > > > circumstance is permission failure expected on a fast-path?
> > > > 
> > > 
> > > You're treating the problem as if it were a timeout issue, when clearly
> > > it has nothing at all to do with timeouts. There is no problem of
> > > 'group membership changes on a regular basis' to be solved.
> > 
> > You are the one who suggested a timeout.  I quote:
> > 
> > > That way, you have a mechanism that serves all purposes: it can do an
> > > immediate one-time only flush, or you can set up a userspace job that
> > > issues a global flush once every so often, e.g. using a cron job.
> > 
> > "every so often" == "timeout".  I thought that maybe this was somewhere
> > that we could find some agreement.  As I said, I would set the timeout
> > to zero.  I don't really want the timeout - not more than the ac
> > timeouts we already have.
> > 
> > > 
> > > The problem to be solved is that on the very rare occasion when a group
> > > membership does change, then the server and the client may update their
> > > view of that membership at completely different times. In the
> > > particular case when the client updates its view of the group
> > > membership before the server does, then the access cache is polluted,
> > > and there is no remedy.
> > 
> > Agreed.
> > 
> > > 
> > > So my concerns are around the mismatch of problem and solution. I see
> > > multiple issues.
> > > 
> > >    1. Your timeouts are per inode. That means that if inode A sees the
> > >       problem being solved, then there is no guarantee that inode B
> > >       sees the same problem as being solved (and the converse is true
> > >       as well).
> > 
> > Is that a problem?  Is that even anything new?
> > If I chmod file A and file B on the server, then the client may see
> > the change to file A before the change to file B (or vice-versa).
> > i.e.  the inconsistent-cache problem might be solved for one but not the
> > other.  It has always been this way.
> > 
> > >    2. There is no quick on-the-spot solution. If your admin updates the
> > >       group membership, then you are only guaranteed that the client
> > >       and server are in sync once the server has picked up the solution
> > >       (however you arrange that), and the client cache has expired.
> > >       IOW: your only solution is to wait 1 client cache expiration
> > >       period after the server is known to be fixed (or to reboot the
> > >       client).
> > 
> > This is also the only solution to seeing other changes that have been
> > made to inodes.
> > For file/directory content you can open the file/directory and this
> > triggers CTO consistency checks.  But stat() or access() doesn't.
> > 
> > Hmmm.. What if we add an ACCESS check to the OPEN request for files, and
> > the equivalent GETATTR for directories?  That would provide a direct
> > way to force a refresh without adding any extra RPC requests??
> > 
> > >    3. There is no solution at all for the positive cache case. If your
> > >       sysadmin is trying to revoke an access due to a group membership
> > >       change, their only solution is to reboot the client.
> > 
> > Yes.  Revoking read/execute access that you have already granted is not
> > really possible.  The application may have already read the file.  It
> > might even have emailed the content to $BLACKHAT.  Even rebooting the
> > client isn't really a solution.
> > Revoking write access already works fine as does revoking read access to
> > a file before putting new content in it - the new content is safe.
> > 
> > >    4. You are tying the access cache timeout to the completely
> > >       unrelated 'acregmin' and 'acdirmin' values. Not only does that
> > >       mean that the default values for regular files are extremely
> > >       small (3 seconds), meaning that we have to refresh extremely
> > >       often. However it also means that you have to explain why
> > >       directories behave differently (longer default timeouts) despite
> > >       the fact that the group membership changed at exactly the same
> > >       time for both types of object.
> > >          1. Bonus points for explaining why our default values are designed
> > >             for a group membership that changes every 3 seconds.
> > 
> > I don't see why you treat the access information as different from all
> > the other attributes.  "bob has group x access to the directory" and
> > "file size if 42000 bytes" are just attributes of the inode.  We collect
> > them different ways, but they are not deeply different.
> > 
> > The odd thing here is that we cache these "access" attributes
> > indefinitely when ctime doesn't change - even though there is no
> > guarantee that ctime captures access changes.  I think that choice needs
> > to be justified.
> 
> Maybe I'm being pedantic, but I don't see the first as an inode
> attribute. There are really 2 pieces to that access control example:
> 
> - bob is a member of group x
> - group x has access to the directory
> 
> The first has nothing directly to do with the inode and so it's no
> surprise that its ctime and i_version aren't affected when group
> membership changes.

The whole point of the NFS ACCESS request is that the client cannot know
how to interpret any access controls that the server might have in
place.  Who knows, they might even be time based access controls
(games can only be played on the weekend).  Ok, that is a bit extreme,
but the principle remains.

*You* might "know" that there are two pieces (in this specific example)
and that one doesn't directly relate to the inodes, but the NFS client
doesn't know that.  The NFS client only knows that it can request ACCESS
information for a filehandle in the context of a credential.  The Linux
NFS client then caches that against the inode (so it is all somehow
related to the inode).

The spec doesn't give the client permission to cached these details AT
ALL.  Caching positive results makes perfect sense from a performance
perspective and is easily defensible.  Caching negative results ...  not
so much.

Thanks,
NeilBrown

> 
> > Using the cached value indefinitely when it grants access is defensible
> > because it is a frequent operation (checking x access in a directory
> > while following a patch).  I think that adequately justifies the choice.
> > I cannot see the justification when the access is denied by the cache.
> > 
> > >    5. 'noac' suddenly now turns off access caching, but only for
> > >       negative cached values.
> > 
> > Is this a surprise?
> > 
> > But what do you think of adding an ACCESS check when opening a dir/file?
> > At least for NFSv4?
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
>