| Message ID | 170666026049.21664.14325649754684255655@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: don't call locks_release_private() twice concurrently | expand |
On Wed, Jan 31, 2024 at 11:17:40AM +1100, NeilBrown wrote: > > It is possible for free_blocked_lock() to be called twice concurrently, > once from nfsd4_lock() and once from nfsd4_release_lockowner() calling > remove_blocked_locks(). This is why a kref was added. > > It is perfectly safe for locks_delete_block() and kref_put() to be > called in parallel as they use locking or atomicity respectively as > protection. However locks_release_private() has no locking. It is > safe for it to be called twice sequentially, but not concurrently. > > This patch moves that call from free_blocked_lock() where it could race > with itself, to free_nbl() where it cannot. This will slightly delay > the freeing of private info or release of the owner - but not by much. > It is arguably more natural for this freeing to happen in free_nbl() > where the structure itself is freed. > > This bug was found by code inspection - it has not been seen in practice. > > Fixes: 47446d74f170 ("nfsd4: add refcount for nfsd4_blocked_lock") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index a66d66b9f769..12534e12dbb3 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -318,6 +318,7 @@ free_nbl(struct kref *kref) > struct nfsd4_blocked_lock *nbl; > > nbl = container_of(kref, struct nfsd4_blocked_lock, nbl_kref); > + locks_release_private(&nbl->nbl_lock); > kfree(nbl); > } > > @@ -325,7 +326,6 @@ static void > free_blocked_lock(struct nfsd4_blocked_lock *nbl) > { > locks_delete_block(&nbl->nbl_lock); > - locks_release_private(&nbl->nbl_lock); > kref_put(&nbl->nbl_kref, free_nbl); > } > > -- > 2.43.0 Applied to nfsd-next (for v6.9).
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index a66d66b9f769..12534e12dbb3 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -318,6 +318,7 @@ free_nbl(struct kref *kref) struct nfsd4_blocked_lock *nbl; nbl = container_of(kref, struct nfsd4_blocked_lock, nbl_kref); + locks_release_private(&nbl->nbl_lock); kfree(nbl); } @@ -325,7 +326,6 @@ static void free_blocked_lock(struct nfsd4_blocked_lock *nbl) { locks_delete_block(&nbl->nbl_lock); - locks_release_private(&nbl->nbl_lock); kref_put(&nbl->nbl_kref, free_nbl); }
It is possible for free_blocked_lock() to be called twice concurrently, once from nfsd4_lock() and once from nfsd4_release_lockowner() calling remove_blocked_locks(). This is why a kref was added. It is perfectly safe for locks_delete_block() and kref_put() to be called in parallel as they use locking or atomicity respectively as protection. However locks_release_private() has no locking. It is safe for it to be called twice sequentially, but not concurrently. This patch moves that call from free_blocked_lock() where it could race with itself, to free_nbl() where it cannot. This will slightly delay the freeing of private info or release of the owner - but not by much. It is arguably more natural for this freeing to happen in free_nbl() where the structure itself is freed. This bug was found by code inspection - it has not been seen in practice. Fixes: 47446d74f170 ("nfsd4: add refcount for nfsd4_blocked_lock") Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)