2022-04-28 02:23:24

by NeilBrown

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

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


2022-04-28 04:06:53

by NeilBrown

[permalink] [raw]
Subject: [PATCH 2/2] NFS: limit use of ACCESS cache for negative responses

NFS currently caches the results of ACCESS lookups indefinitely while the
inode doesn't change (i.e. while ctime, changeid, owner/group/mode etc
don't change). This is a problem if the result from the server might
change.

When the result from the server depends purely on the credentials
provided by the client and the information in the inode, it is not
expected that the result from the server will change, and the current
behaviour is safe.

However in some configurations the server can include another lookup
step. This happens with the Linux NFS server when the "--manage-gids"
option is given to rpc.mountd. NetApp servers have similar functionality
with "AUTH_SYS Extended Groups" functionality in ONTAP9. With these,
the user reported by the client is mapped on the server to a list of
group ids. If this mapping changes, the result of ACCESS can change.

This is particularly a problem when a new group is added to a user. If
they had already tried and failed to access the file (or more commonly a
directory) then adding them to the group will not successfully give them
access as the failure is cached. Even if the user logs out and back in
again to get the new credential on the client, they might try to access
the file before the server is aware of the change. By default the Linux
server caches group information for 30 minutes. This can be reduced but
there will always be a window after the group has been added when the
server can still report ACCESS based on old group information.

The inverse is less of a problem. Removing someone from a group has
never been a guaranteed way to remove any access - at the very least a
user normally needs to log off before they lose access to any groups that
they were a member of.

If a user is removed from a group they may still be granted "access" on
the client, but this isn't sufficient to change anything for which write
access has been removed, and normally only provides read access to
cached content. So caching positive ACCESS rights indefinitely is not a
significant security problem.

The value of the ACCESS cache is realised primarily for successful
tests. These happen often, for example the test for X permissions
during filename lookups. Having a quick (even lock-free) result helps
this common operation. When the ACCESS cache denies the access there is
less cost in taking longer to confirm the access, and this is the case
where a stale cache is more problematic.

So, this patch changes the way that the access cache is used.
- If the requested access is found in the cache, and is granted, then the
call uses the cached information no matter how old it is.
- If the requested access is found in the cache and is denied, then the
cached result is only used if is it newer than the current MINATTRTIMEO
for the file.
- If the requested access is found in the cache, is denied, and is more
than MINATTRTIMEO old, then a new ACCESS request is made to the server
- If the requested access is NOT found in the cache, obviously a new
ACCESS request is made to the server, and this will be cached.

Signed-off-by: NeilBrown <[email protected]>
---
fs/nfs/dir.c | 24 +++++++++++++++++++-----
fs/nfs/nfs4proc.c | 1 +
include/linux/nfs_fs.h | 1 +
3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index b4e2c6a34234..c461029515b5 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2900,7 +2900,8 @@ static struct nfs_access_entry *nfs_access_search_rbtree(struct inode *inode, co
return NULL;
}

-static int nfs_access_get_cached_locked(struct inode *inode, const struct cred *cred, u32 *mask, bool may_block)
+static int nfs_access_get_cached_locked(struct inode *inode, const struct cred *cred,
+ u32 *mask, unsigned long *cjiffies, bool may_block)
{
struct nfs_inode *nfsi = NFS_I(inode);
struct nfs_access_entry *cache;
@@ -2931,6 +2932,7 @@ static int nfs_access_get_cached_locked(struct inode *inode, const struct cred *
retry = false;
}
*mask = cache->mask;
+ *cjiffies = cache->jiffies;
list_move_tail(&cache->lru, &nfsi->access_cache_entry_lru);
err = 0;
out:
@@ -2942,7 +2944,8 @@ static int nfs_access_get_cached_locked(struct inode *inode, const struct cred *
return -ENOENT;
}

-static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cred, u32 *mask)
+static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cred,
+ u32 *mask, unsigned long *cjiffies)
{
/* Only check the most recently returned cache entry,
* but do it without locking.
@@ -2965,6 +2968,7 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre
if (nfs_check_cache_invalid(inode, NFS_INO_INVALID_ACCESS))
goto out;
*mask = cache->mask;
+ *cjiffies = cache->jiffies;
err = 0;
out:
rcu_read_unlock();
@@ -2974,16 +2978,24 @@ static int nfs_access_get_cached_rcu(struct inode *inode, const struct cred *cre
int nfs_access_check_cached(struct inode *inode, const struct cred *cred,
u32 want, bool may_block)
{
+ unsigned long cjiffies;
u32 have;
int status;

- status = nfs_access_get_cached_rcu(inode, cred, &have);
+ status = nfs_access_get_cached_rcu(inode, cred, &have, &cjiffies);
if (status != 0)
status = nfs_access_get_cached_locked(inode, cred, &have,
- may_block);
+ &cjiffies, may_block);

- if (status == 0 && (want & ~have) != 0)
+ if (status == 0 && (want & ~have) != 0) {
status = -EACCES;
+ /* Don't trust a negative result beyond MINATTRTIMEO,
+ * even if we hold a delegation
+ */
+ if (!time_in_range_open(jiffies, cjiffies,
+ cjiffies + NFS_MINATTRTIMEO(inode)))
+ status = -ENOENT;
+ }
return status;
}
EXPORT_SYMBOL_GPL(nfs_access_check_cached);
@@ -3036,6 +3048,7 @@ void nfs_access_add_cache(struct inode *inode, struct nfs_access_entry *set,
cache->fsgid = cred->fsgid;
cache->group_info = get_group_info(cred->group_info);
cache->mask = set->mask;
+ cache->jiffies = set->jiffies;

/* The above field assignments must be visible
* before this item appears on the lru. We cannot easily
@@ -3121,6 +3134,7 @@ static int nfs_do_access(struct inode *inode, const struct cred *cred, int mask)
*/
cache.mask = NFS_ACCESS_READ | NFS_ACCESS_MODIFY | NFS_ACCESS_EXTEND |
nfs_access_xattr_mask(NFS_SERVER(inode));
+ cache.jiffies = jiffies;
if (S_ISDIR(inode->i_mode))
cache.mask |= NFS_ACCESS_DELETE | NFS_ACCESS_LOOKUP;
else
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1e80d88df588..a78b62c5bad1 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -2644,6 +2644,7 @@ static int nfs4_opendata_access(const struct cred *cred,
mask = NFS4_ACCESS_READ;

nfs_access_set_mask(&cache, opendata->o_res.access_result);
+ cache.jiffies = jiffies;
nfs_access_add_cache(state->inode, &cache, cred);

flags = NFS4_ACCESS_READ | NFS4_ACCESS_EXECUTE | NFS4_ACCESS_LOOKUP;
diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index 7614f621c42d..e40bd61c80c5 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -56,6 +56,7 @@
struct nfs_access_entry {
struct rb_node rb_node;
struct list_head lru;
+ unsigned long jiffies;
kuid_t fsuid;
kgid_t fsgid;
struct group_info *group_info;


2022-05-17 03:42:08

by NeilBrown

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


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

2022-05-17 03:43:20

by Trond Myklebust

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

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

--
Trond Myklebust
CTO, Hammerspace Inc
4984 El Camino Real, Suite 208
Los Altos, CA 94022

http://www.hammer.space

2022-05-17 03:45:43

by NeilBrown

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

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

2022-05-17 04:15:14

by Trond Myklebust

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

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.


--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]


2022-05-17 06:06:48

by NeilBrown

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

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
> [email protected]
>
>
>

2022-05-17 06:45:20

by Trond Myklebust

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

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

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]


2022-05-17 07:02:19

by NeilBrown

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

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

2022-05-17 08:15:06

by Trond Myklebust

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

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.

--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]