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