fix for find_lockowner_str crash
Subsystems (2)
cover fix for find_lockowner_str crash
From: "J. Bruce Fields" <[email protected]>
This is for 3.15 and stable.
Jeff, cc'ing you since I seem to recall Trond's series changes to using a single lockowner. I think the following is the quicker fix for stable, but you could convince me otherwise.
J. Bruce Fields (2):
nfsd4: remove lockowner when removing lock stateid
nfsd4: warn on finding lockowner without stateid's
fs/nfsd/nfs4state.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
Signature · 1 line
1.9.0
PATCH 1/2 nfsd4: remove lockowner when removing lock stateid
From: "J. Bruce Fields" <[email protected]>
The nfsv4 state code has always assumed a one-to-one correspondance between lock stateid's and lockowners even if it appears not to in some places.
We may actually change that, but for now when FREE_STATEID releases a lock stateid it also needs to release the parent lockowner.
Symptoms were a subsequent LOCK crashing in find_lockowner_str when it calls same_lockowner_ino on a lockowner that unexpectedly has an empty so_stateids list.
--- fs/nfsd/nfs4state.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
fs/nfsd/nfs4state.c +9 -2
@@ -3717,9 +3717,16 @@ out:static __be32 nfsd4_free_lock_stateid (struct nfs4_ol_stateid *stp) { -if (check_for_locks (stp->st_file,lockowner (stp->st_stateowner))) +struct nfs4_lockowner *lo =lockowner (stp->st_stateowner); + +if (check_for_locks (stp->st_file, lo))return nfserr_locks_held; -release_lock_stateid (stp); +/* +* Currently there's a 1-1 lock stateid<->lockowner +* correspondance, and we have to delete the lockowner when we +* delete the lock stateid: +*/ +unhash_lockowner (lo);return nfs_ok; }
Signature · 1 line
1.9.0
Re: [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid
Quoted 29 lines · On Wed, 21 May 2014 12:05:24 -0400 "J. Bruce Fields" <[email protected]> wrote:
From: "J. Bruce Fields" <[email protected]>
The nfsv4 state code has always assumed a one-to-one correspondance between lock stateid's and lockowners even if it appears not to in some places.
We may actually change that, but for now when
FREE_STATEIDreleases a lock stateid it also needs to release the parent lockowner.Symptoms were a subsequent LOCK crashing in
find_lockowner_strwhen it callssame_lockowner_inoon a lockowner that unexpectedly has an emptyso_stateidslist.Signed-off-by J. Bruce Fields <[email protected]>--- fs/nfsd/nfs4state.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)fs/nfsd/nfs4state.c +1
-1@@ -3717,9 +3717,16 @@ out:static __be32 nfsd4_free_lock_stateid (struct nfs4_ol_stateid *stp) { -if (check_for_locks (stp->st_file,lockowner (stp->st_stateowner))) +struct nfs4_lockowner *lo =lockowner (stp->st_stateowner);
fs/nfsd/nfs4state.c out: · line 3729
Quoted 10 lines
fs/nfsd/nfs4state.c +8
-1+ +if (check_for_locks (stp->st_file, lo))return nfserr_locks_held; -release_lock_stateid (stp); +/* +* Currently there's a 1-1 lock stateid<->lockowner +* correspondance, and we have to delete the lockowner when we +* delete the lock stateid: +*/ +unhash_lockowner (lo);
Shouldn't this be release_lockowner(lo) ? If not, what's going to free the lockowner afterward?
Quoted 3 lines
fs/nfsd/nfs4state.c +0
-0return nfs_ok; }
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid
Quoted 37 lines · Jeff Layton
Quoted 34 lines · On Wed, 21 May 2014 12:05:24 -0400 "J. Bruce Fields" <[email protected]> wrote:
From: "J. Bruce Fields" <[email protected]>
The nfsv4 state code has always assumed a one-to-one correspondance between lock stateid's and lockowners even if it appears not to in some places.
We may actually change that, but for now when
FREE_STATEIDreleases a lock stateid it also needs to release the parent lockowner.Symptoms were a subsequent LOCK crashing in
find_lockowner_strwhen it callssame_lockowner_inoon a lockowner that unexpectedly has an emptyso_stateidslist.Signed-off-by J. Bruce Fields <[email protected]>--- fs/nfsd/nfs4state.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)fs/nfsd/nfs4state.c +4
-2@@ -3717,9 +3717,16 @@ out:static __be32 nfsd4_free_lock_stateid (struct nfs4_ol_stateid *stp) { -if (check_for_locks (stp->st_file,lockowner (stp->st_stateowner))) +struct nfs4_lockowner *lo =lockowner (stp->st_stateowner); + +if (check_for_locks (stp->st_file, lo))return nfserr_locks_held; -release_lock_stateid (stp); +/*
Quoted 8 lines
fs/nfsd/nfs4state.c out: · line 3729
Quoted 5 lines
fs/nfsd/nfs4state.c +5
-0+* Currently there's a 1-1 lock stateid<->lockowner +* correspondance, and we have to delete the lockowner when we +* delete the lock stateid: +*/ +unhash_lockowner (lo);Shouldn't this be
release_lockowner(lo)? If not, what's going to free the lockowner afterward?
Yes, thank you! I'll probably send along the following soon....
--b.
commit bc0336505f20
Author: J. Bruce Fields <[email protected]> Date: Tue May 27 11:14:26 2014 -0400
nfsd4: fix FREE_STATEID lockowner leak
27b11428b7de ("nfsd4: remove lockowner when removing lock stateid") introduced a memory leak.
fs/nfsd/nfs4state.c +1 -1
@@ -3726,7 +3726,7 @@ nfsd4_free_lock_stateid(struct nfs4_ol_stateid *stp)* correspondance, and we have to delete the lockowner when we * delete the lock stateid: */ -unhash_lockowner (lo); +release_lockowner (lo);return nfs_ok; }
PATCH 2/2 nfsd4: warn on finding lockowner without stateid's
In reply to J. Bruce FieldsFrom: "J. Bruce Fields" <[email protected]>
The current code assumes a one-to-one lockowner<->lock stateid correspondance.
--- fs/nfsd/nfs4state.c | 4 ++++ 1 file changed, 4 insertions(+)
fs/nfsd/nfs4state.c +4 -0
@@ -4166,6 +4166,10 @@ static bool same_lockowner_ino(struct nfs4_lockowner *lo, struct inode *inode, cif (!same_owner_str (&lo->lo_owner, owner, clid))return false; +if (list_empty (&lo->lo_owner.so_stateids)) { +WARN_ON_ONCE (1 ); +return false; + } lst =list_first_entry (&lo->lo_owner.so_stateids, struct nfs4_ol_stateid, st_perstateowner);return lst->st_file->fi_inode == inode;
Signature · 1 line
1.9.0
Quoted 8 lines · On Wed, 21 May 2014 12:05:23 -0400 "J. Bruce Fields" <[email protected]> wrote:
From: "J. Bruce Fields" <[email protected]>
This is for 3.15 and stable.
Jeff, cc'ing you since I seem to recall Trond's series changes to using a single lockowner. I think the following is the quicker fix for stable, but you could convince me otherwise.
Quoted 7 lines
J. Bruce Fields (2):
nfsd4: remove lockowner when removing lock stateid
nfsd4: warn on finding lockowner without stateid'sfs/nfsd/nfs4state.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
Looks reasonable as a less invasive fix for stable. I'm still getting comfortable with the nfsd locking overhaul, so I won't suggest anything else just yet.
Signature · 1 line
Jeff Layton <[email protected]>