mbox series

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

Message ID 165110909570.7595.8578730126480600782.stgit@noble.brown (mailing list archive)
Headers show
Series NFS: limit use of ACCESS cache for negative responses | expand

Message

NeilBrown April 28, 2022, 1:37 a.m. UTC
Since Commit 57b691819ee2 ("NFS: Cache access checks more aggressively")
(Linux 4.8) NFS has cached the results of ACCESS indefinitely while the
inode isn't changing.

This is often a good choice, but doesn't take into account the
possibility that changes out side of the inode can change effective
permissions.

Depending on configuration, some servers can map the user provided in
the RPC credential to a group list at time of request.  If the group
list for a user is changed, the result of ACCESS can change.

This is particularly a problem when extra permissions are given on the
server.  The client may make decisions based on outdated ACCESS results
and not even try operations which would in fact succeed.

These two patches change the ACCESS cache so that when the cache grants
an access, that is trusted indefinitely just as it currently does.
However when the cache denies an access, that is only trusted if the
cached data is less than acmin seconds old.  Otherwise a new ACCESS
request is made.

This allows additions to group membership to become effective with
only a modest delay.

The second patch contains even more explanatory detail.

Thanks,
NeilBrown

---

NeilBrown (2):
      NFS: change nfs_access_get_cached() to nfs_access_check_cached()
      NFS: limit use of ACCESS cache for negative responses


 fs/nfs/dir.c           | 80 +++++++++++++++++++++++++-----------------
 fs/nfs/nfs4proc.c      | 25 ++++++-------
 include/linux/nfs_fs.h |  5 +--
 3 files changed, 61 insertions(+), 49 deletions(-)

--
Signature

Comments

NeilBrown May 17, 2022, 12:05 a.m. UTC | #1
Hi,
 any thoughts on these patches?

Thanks,
NeilBrown


On Thu, 28 Apr 2022, NeilBrown wrote:
> Since Commit 57b691819ee2 ("NFS: Cache access checks more aggressively")
> (Linux 4.8) NFS has cached the results of ACCESS indefinitely while the
> inode isn't changing.
> 
> This is often a good choice, but doesn't take into account the
> possibility that changes out side of the inode can change effective
> permissions.
> 
> Depending on configuration, some servers can map the user provided in
> the RPC credential to a group list at time of request.  If the group
> list for a user is changed, the result of ACCESS can change.
> 
> This is particularly a problem when extra permissions are given on the
> server.  The client may make decisions based on outdated ACCESS results
> and not even try operations which would in fact succeed.
> 
> These two patches change the ACCESS cache so that when the cache grants
> an access, that is trusted indefinitely just as it currently does.
> However when the cache denies an access, that is only trusted if the
> cached data is less than acmin seconds old.  Otherwise a new ACCESS
> request is made.
> 
> This allows additions to group membership to become effective with
> only a modest delay.
> 
> The second patch contains even more explanatory detail.
> 
> Thanks,
> NeilBrown
> 
> ---
> 
> NeilBrown (2):
>       NFS: change nfs_access_get_cached() to nfs_access_check_cached()
>       NFS: limit use of ACCESS cache for negative responses
> 
> 
>  fs/nfs/dir.c           | 80 +++++++++++++++++++++++++-----------------
>  fs/nfs/nfs4proc.c      | 25 ++++++-------
>  include/linux/nfs_fs.h |  5 +--
>  3 files changed, 61 insertions(+), 49 deletions(-)
> 
> --
> Signature
> 
>
Trond Myklebust May 17, 2022, 12:20 a.m. UTC | #2
On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> 
> Hi,
>  any thoughts on these patches?


To me, this problem is simply not worth breaking hot path functionality
for. If the credential changes on the server, but not on the client (so
that the cred cache comparison still matches), then why do we care?

IOW: I'm a NACK until convinced otherwise.

> 
> Thanks,
> NeilBrown
> 
> 
> On Thu, 28 Apr 2022, NeilBrown wrote:
> > Since Commit 57b691819ee2 ("NFS: Cache access checks more
> > aggressively")
> > (Linux 4.8) NFS has cached the results of ACCESS indefinitely while
> > the
> > inode isn't changing.
> > 
> > This is often a good choice, but doesn't take into account the
> > possibility that changes out side of the inode can change effective
> > permissions.
> > 
> > Depending on configuration, some servers can map the user provided
> > in
> > the RPC credential to a group list at time of request.  If the
> > group
> > list for a user is changed, the result of ACCESS can change.
> > 
> > This is particularly a problem when extra permissions are given on
> > the
> > server.  The client may make decisions based on outdated ACCESS
> > results
> > and not even try operations which would in fact succeed.
> > 
> > These two patches change the ACCESS cache so that when the cache
> > grants
> > an access, that is trusted indefinitely just as it currently does.
> > However when the cache denies an access, that is only trusted if
> > the
> > cached data is less than acmin seconds old.  Otherwise a new ACCESS
> > request is made.
> > 
> > This allows additions to group membership to become effective with
> > only a modest delay.
> > 
> > The second patch contains even more explanatory detail.
> > 
> > Thanks,
> > NeilBrown
> > 
> > ---
> > 
> > NeilBrown (2):
> >       NFS: change nfs_access_get_cached() to
> > nfs_access_check_cached()
> >       NFS: limit use of ACCESS cache for negative responses
> > 
> > 
> >  fs/nfs/dir.c           | 80 +++++++++++++++++++++++++-------------
> > ----
> >  fs/nfs/nfs4proc.c      | 25 ++++++-------
> >  include/linux/nfs_fs.h |  5 +--
> >  3 files changed, 61 insertions(+), 49 deletions(-)
> > 
> > --
> > Signature
> > 
> >
NeilBrown May 17, 2022, 12:40 a.m. UTC | #3
On Tue, 17 May 2022, Trond Myklebust wrote:
> On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > 
> > Hi,
> >  any thoughts on these patches?
> 
> 
> To me, this problem is simply not worth breaking hot path functionality
> for. If the credential changes on the server, but not on the client (so
> that the cred cache comparison still matches), then why do we care?
> 
> IOW: I'm a NACK until convinced otherwise.

In what way is the hot path broken?  It only affect a permission test
failure.  Why is that considered "hot path"??

RFC 1813 says - for NFSv3 at least - 

      The information returned by the server in response to an
      ACCESS call is not permanent. It was correct at the exact
      time that the server performed the checks, but not
      necessarily afterwards. The server can revoke access
      permission at any time.

Clearly the server can allow allow access at any time.
This seems to argue against caching - or at least against relying too
heavily on the cache.

RFC 8881 has the same text for NFSv4.1 - section 18.1.4

"why do we care?" Because the server has changed to grant access, but
the client is ignoring the possibility of that change - so the user is
prevented from doing work.

NeilBrown
Trond Myklebust May 17, 2022, 12:55 a.m. UTC | #4
On Tue, 2022-05-17 at 10:40 +1000, NeilBrown wrote:
> On Tue, 17 May 2022, Trond Myklebust wrote:
> > On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > > 
> > > Hi,
> > >  any thoughts on these patches?
> > 
> > 
> > To me, this problem is simply not worth breaking hot path
> > functionality
> > for. If the credential changes on the server, but not on the client
> > (so
> > that the cred cache comparison still matches), then why do we care?
> > 
> > IOW: I'm a NACK until convinced otherwise.
> 
> In what way is the hot path broken?  It only affect a permission test
> failure.  Why is that considered "hot path"??

It is a permission test that is critical for caching path resolution on
a per-user basis.

> 
> RFC 1813 says - for NFSv3 at least - 
> 
>       The information returned by the server in response to an
>       ACCESS call is not permanent. It was correct at the exact
>       time that the server performed the checks, but not
>       necessarily afterwards. The server can revoke access
>       permission at any time.
> 
> Clearly the server can allow allow access at any time.
> This seems to argue against caching - or at least against relying too
> heavily on the cache.
> 
> RFC 8881 has the same text for NFSv4.1 - section 18.1.4
> 
> "why do we care?" Because the server has changed to grant access, but
> the client is ignoring the possibility of that change - so the user
> is
> prevented from doing work.

The server enforces permissions in NFS. The client permissions checks
are performed in order to gate access to data that is already in cache.
NACK
NeilBrown May 17, 2022, 1:05 a.m. UTC | #5
On Tue, 17 May 2022, Trond Myklebust wrote:
> On Tue, 2022-05-17 at 10:40 +1000, NeilBrown wrote:
> > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > > > 
> > > > Hi,
> > > >  any thoughts on these patches?
> > > 
> > > 
> > > To me, this problem is simply not worth breaking hot path
> > > functionality
> > > for. If the credential changes on the server, but not on the client
> > > (so
> > > that the cred cache comparison still matches), then why do we care?
> > > 
> > > IOW: I'm a NACK until convinced otherwise.
> > 
> > In what way is the hot path broken?  It only affect a permission test
> > failure.  Why is that considered "hot path"??
> 
> It is a permission test that is critical for caching path resolution on
> a per-user basis.
> 
> > 
> > RFC 1813 says - for NFSv3 at least - 
> > 
> >       The information returned by the server in response to an
> >       ACCESS call is not permanent. It was correct at the exact
> >       time that the server performed the checks, but not
> >       necessarily afterwards. The server can revoke access
> >       permission at any time.
> > 
> > Clearly the server can allow allow access at any time.
> > This seems to argue against caching - or at least against relying too
> > heavily on the cache.
> > 
> > RFC 8881 has the same text for NFSv4.1 - section 18.1.4
> > 
> > "why do we care?" Because the server has changed to grant access, but
> > the client is ignoring the possibility of that change - so the user
> > is
> > prevented from doing work.
> 
> The server enforces permissions in NFS. The client permissions checks
> are performed in order to gate access to data that is already in cache.

So if the permission check fails, then the client should ignore the
cache and ask the server for the requested data, so that the server has
a chance to enforce the permissions - whether denying or allowing the
access.

I completely agree with you, and that is effectively what my patch
implements.

NeilBrown


> NACK
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 
>
Trond Myklebust May 17, 2022, 1:14 a.m. UTC | #6
On Tue, 2022-05-17 at 11:05 +1000, NeilBrown wrote:
> On Tue, 17 May 2022, Trond Myklebust wrote:
> > On Tue, 2022-05-17 at 10:40 +1000, NeilBrown wrote:
> > > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > > On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > > > > 
> > > > > Hi,
> > > > >  any thoughts on these patches?
> > > > 
> > > > 
> > > > To me, this problem is simply not worth breaking hot path
> > > > functionality
> > > > for. If the credential changes on the server, but not on the
> > > > client
> > > > (so
> > > > that the cred cache comparison still matches), then why do we
> > > > care?
> > > > 
> > > > IOW: I'm a NACK until convinced otherwise.
> > > 
> > > In what way is the hot path broken?  It only affect a permission
> > > test
> > > failure.  Why is that considered "hot path"??
> > 
> > It is a permission test that is critical for caching path
> > resolution on
> > a per-user basis.
> > 
> > > 
> > > RFC 1813 says - for NFSv3 at least - 
> > > 
> > >       The information returned by the server in response to an
> > >       ACCESS call is not permanent. It was correct at the exact
> > >       time that the server performed the checks, but not
> > >       necessarily afterwards. The server can revoke access
> > >       permission at any time.
> > > 
> > > Clearly the server can allow allow access at any time.
> > > This seems to argue against caching - or at least against relying
> > > too
> > > heavily on the cache.
> > > 
> > > RFC 8881 has the same text for NFSv4.1 - section 18.1.4
> > > 
> > > "why do we care?" Because the server has changed to grant access,
> > > but
> > > the client is ignoring the possibility of that change - so the
> > > user
> > > is
> > > prevented from doing work.
> > 
> > The server enforces permissions in NFS. The client permissions
> > checks
> > are performed in order to gate access to data that is already in
> > cache.
> 
> So if the permission check fails, then the client should ignore the
> cache and ask the server for the requested data, so that the server
> has
> a chance to enforce the permissions - whether denying or allowing the
> access.
> 
> I completely agree with you, and that is effectively what my patch
> implements.
> 

No. I'm saying that no matter how many spec paragraphs you quote at me,
I'm not willing to introduce a timeout on a cache that is critical for
path resolution in order to address a corner case of a corner case.

I'm saying that if you want this problem fixed, then you need to look
at a different solution that doesn't have side-effects for the
99.99999% cases or more that I do care about.
NeilBrown May 17, 2022, 1:22 a.m. UTC | #7
On Tue, 17 May 2022, Trond Myklebust wrote:
> On Tue, 2022-05-17 at 11:05 +1000, NeilBrown wrote:
> > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > On Tue, 2022-05-17 at 10:40 +1000, NeilBrown wrote:
> > > > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > > > On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > > > > > 
> > > > > > Hi,
> > > > > >  any thoughts on these patches?
> > > > > 
> > > > > 
> > > > > To me, this problem is simply not worth breaking hot path
> > > > > functionality
> > > > > for. If the credential changes on the server, but not on the
> > > > > client
> > > > > (so
> > > > > that the cred cache comparison still matches), then why do we
> > > > > care?
> > > > > 
> > > > > IOW: I'm a NACK until convinced otherwise.
> > > > 
> > > > In what way is the hot path broken?  It only affect a permission
> > > > test
> > > > failure.  Why is that considered "hot path"??
> > > 
> > > It is a permission test that is critical for caching path
> > > resolution on
> > > a per-user basis.
> > > 
> > > > 
> > > > RFC 1813 says - for NFSv3 at least - 
> > > > 
> > > >       The information returned by the server in response to an
> > > >       ACCESS call is not permanent. It was correct at the exact
> > > >       time that the server performed the checks, but not
> > > >       necessarily afterwards. The server can revoke access
> > > >       permission at any time.
> > > > 
> > > > Clearly the server can allow allow access at any time.
> > > > This seems to argue against caching - or at least against relying
> > > > too
> > > > heavily on the cache.
> > > > 
> > > > RFC 8881 has the same text for NFSv4.1 - section 18.1.4
> > > > 
> > > > "why do we care?" Because the server has changed to grant access,
> > > > but
> > > > the client is ignoring the possibility of that change - so the
> > > > user
> > > > is
> > > > prevented from doing work.
> > > 
> > > The server enforces permissions in NFS. The client permissions
> > > checks
> > > are performed in order to gate access to data that is already in
> > > cache.
> > 
> > So if the permission check fails, then the client should ignore the
> > cache and ask the server for the requested data, so that the server
> > has
> > a chance to enforce the permissions - whether denying or allowing the
> > access.
> > 
> > I completely agree with you, and that is effectively what my patch
> > implements.
> > 
> 
> No. I'm saying that no matter how many spec paragraphs you quote at me,
> I'm not willing to introduce a timeout on a cache that is critical for
> path resolution in order to address a corner case of a corner case.
> 
> I'm saying that if you want this problem fixed, then you need to look
> at a different solution that doesn't have side-effects for the
> 99.99999% cases or more that I do care about.

What, specifically, as the cases that you do care about?

I assumed that the cases you care about are cases where the user *does*
have access, and where this access is correctly cached, so that a 
  permission(..., MAY_EXEC)
call returns immediately with a positive answer.
I care about these cases too, and I've ensured that the patch doesn't
change the behaviour for these cases.

What other cases - cases where permission() returns an error - do you care
about? 

Thanks,
NeilBrown
Trond Myklebust May 17, 2022, 1:36 a.m. UTC | #8
On Tue, 2022-05-17 at 11:22 +1000, NeilBrown wrote:
> On Tue, 17 May 2022, Trond Myklebust wrote:
> > On Tue, 2022-05-17 at 11:05 +1000, NeilBrown wrote:
> > > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > > On Tue, 2022-05-17 at 10:40 +1000, NeilBrown wrote:
> > > > > On Tue, 17 May 2022, Trond Myklebust wrote:
> > > > > > On Tue, 2022-05-17 at 10:05 +1000, NeilBrown wrote:
> > > > > > > 
> > > > > > > Hi,
> > > > > > >  any thoughts on these patches?
> > > > > > 
> > > > > > 
> > > > > > To me, this problem is simply not worth breaking hot path
> > > > > > functionality
> > > > > > for. If the credential changes on the server, but not on
> > > > > > the
> > > > > > client
> > > > > > (so
> > > > > > that the cred cache comparison still matches), then why do
> > > > > > we
> > > > > > care?
> > > > > > 
> > > > > > IOW: I'm a NACK until convinced otherwise.
> > > > > 
> > > > > In what way is the hot path broken?  It only affect a
> > > > > permission
> > > > > test
> > > > > failure.  Why is that considered "hot path"??
> > > > 
> > > > It is a permission test that is critical for caching path
> > > > resolution on
> > > > a per-user basis.
> > > > 
> > > > > 
> > > > > RFC 1813 says - for NFSv3 at least - 
> > > > > 
> > > > >       The information returned by the server in response to
> > > > > an
> > > > >       ACCESS call is not permanent. It was correct at the
> > > > > exact
> > > > >       time that the server performed the checks, but not
> > > > >       necessarily afterwards. The server can revoke access
> > > > >       permission at any time.
> > > > > 
> > > > > Clearly the server can allow allow access at any time.
> > > > > This seems to argue against caching - or at least against
> > > > > relying
> > > > > too
> > > > > heavily on the cache.
> > > > > 
> > > > > RFC 8881 has the same text for NFSv4.1 - section 18.1.4
> > > > > 
> > > > > "why do we care?" Because the server has changed to grant
> > > > > access,
> > > > > but
> > > > > the client is ignoring the possibility of that change - so
> > > > > the
> > > > > user
> > > > > is
> > > > > prevented from doing work.
> > > > 
> > > > The server enforces permissions in NFS. The client permissions
> > > > checks
> > > > are performed in order to gate access to data that is already
> > > > in
> > > > cache.
> > > 
> > > So if the permission check fails, then the client should ignore
> > > the
> > > cache and ask the server for the requested data, so that the
> > > server
> > > has
> > > a chance to enforce the permissions - whether denying or allowing
> > > the
> > > access.
> > > 
> > > I completely agree with you, and that is effectively what my
> > > patch
> > > implements.
> > > 
> > 
> > No. I'm saying that no matter how many spec paragraphs you quote at
> > me,
> > I'm not willing to introduce a timeout on a cache that is critical
> > for
> > path resolution in order to address a corner case of a corner case.
> > 
> > I'm saying that if you want this problem fixed, then you need to
> > look
> > at a different solution that doesn't have side-effects for the
> > 99.99999% cases or more that I do care about.
> 
> What, specifically, as the cases that you do care about?

The general case, where the group membership is not changing on the
server without also changing on the client. Whether it is positive or
negative lookups. I care about the ability of the client to manage its
cache without gratuitous invalidations.

This isn't even something that is specific to NFS. This is how local
filesystems operate too.

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.