2024-01-31 00:17:52

by NeilBrown

Subject: [PATCH] nfsd: don't call locks_release_private() twice concurrently

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

2024-01-31 15:44:58

by Chuck Lever III

Subject: Re: [PATCH] nfsd: don't call locks_release_private() twice concurrently

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


--
Chuck Lever