* [PATCH 0/2] fix for find_lockowner_str crash
@ 2014-05-21 16:05 J. Bruce Fields
2014-05-21 16:05 ` [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid J. Bruce Fields
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: J. Bruce Fields @ 2014-05-21 16:05 UTC (permalink / raw)
To: linux-nfs; +Cc: Jeff Layton, J. Bruce Fields
From: "J. Bruce Fields" <bfields@redhat.com>
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(-)
--
1.9.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid 2014-05-21 16:05 [PATCH 0/2] fix for find_lockowner_str crash J. Bruce Fields @ 2014-05-21 16:05 ` J. Bruce Fields 2014-05-27 12:50 ` Jeff Layton 2014-05-21 16:05 ` [PATCH 2/2] nfsd4: warn on finding lockowner without stateid's J. Bruce Fields 2014-05-21 19:14 ` [PATCH 0/2] fix for find_lockowner_str crash Jeff Layton 2 siblings, 1 reply; 6+ messages in thread From: J. Bruce Fields @ 2014-05-21 16:05 UTC (permalink / raw) To: linux-nfs; +Cc: Jeff Layton, J. Bruce Fields, stable From: "J. Bruce Fields" <bfields@redhat.com> 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. Cc: stable@vger.kernel.org Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 32b699b..89e4240 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -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; } -- 1.9.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid 2014-05-21 16:05 ` [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid J. Bruce Fields @ 2014-05-27 12:50 ` Jeff Layton 2014-05-27 15:28 ` J. Bruce Fields 0 siblings, 1 reply; 6+ messages in thread From: Jeff Layton @ 2014-05-27 12:50 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs, Jeff Layton, stable On Wed, 21 May 2014 12:05:24 -0400 "J. Bruce Fields" <bfields@redhat.com> wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > 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. > > Cc: stable@vger.kernel.org > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > fs/nfsd/nfs4state.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 32b699b..89e4240 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -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); Shouldn't this be release_lockowner(lo) ? If not, what's going to free the lockowner afterward? > return nfs_ok; > } > -- Jeff Layton <jlayton@primarydata.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid 2014-05-27 12:50 ` Jeff Layton @ 2014-05-27 15:28 ` J. Bruce Fields 0 siblings, 0 replies; 6+ messages in thread From: J. Bruce Fields @ 2014-05-27 15:28 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-nfs, stable On Tue, May 27, 2014 at 08:50:07AM -0400, Jeff Layton wrote: > On Wed, 21 May 2014 12:05:24 -0400 > "J. Bruce Fields" <bfields@redhat.com> wrote: > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > 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. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > fs/nfsd/nfs4state.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 32b699b..89e4240 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -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); > > 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 <bfields@redhat.com> 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. Reported-by: Jeff Layton <jeff.layton@primarydata.com> Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 9a77a5a21557..6134ee283798 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -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; } ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nfsd4: warn on finding lockowner without stateid's 2014-05-21 16:05 [PATCH 0/2] fix for find_lockowner_str crash J. Bruce Fields 2014-05-21 16:05 ` [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid J. Bruce Fields @ 2014-05-21 16:05 ` J. Bruce Fields 2014-05-21 19:14 ` [PATCH 0/2] fix for find_lockowner_str crash Jeff Layton 2 siblings, 0 replies; 6+ messages in thread From: J. Bruce Fields @ 2014-05-21 16:05 UTC (permalink / raw) To: linux-nfs; +Cc: Jeff Layton, J. Bruce Fields, stable From: "J. Bruce Fields" <bfields@redhat.com> The current code assumes a one-to-one lockowner<->lock stateid correspondance. Cc: stable@vger.kernel.org Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/nfsd/nfs4state.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 89e4240..9a77a5a 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4166,6 +4166,10 @@ static bool same_lockowner_ino(struct nfs4_lockowner *lo, struct inode *inode, c if (!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; -- 1.9.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] fix for find_lockowner_str crash 2014-05-21 16:05 [PATCH 0/2] fix for find_lockowner_str crash J. Bruce Fields 2014-05-21 16:05 ` [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid J. Bruce Fields 2014-05-21 16:05 ` [PATCH 2/2] nfsd4: warn on finding lockowner without stateid's J. Bruce Fields @ 2014-05-21 19:14 ` Jeff Layton 2 siblings, 0 replies; 6+ messages in thread From: Jeff Layton @ 2014-05-21 19:14 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs On Wed, 21 May 2014 12:05:23 -0400 "J. Bruce Fields" <bfields@redhat.com> wrote: > From: "J. Bruce Fields" <bfields@redhat.com> > > 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(-) > 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. -- Jeff Layton <jlayton@primarydata.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-27 15:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-21 16:05 [PATCH 0/2] fix for find_lockowner_str crash J. Bruce Fields 2014-05-21 16:05 ` [PATCH 1/2] nfsd4: remove lockowner when removing lock stateid J. Bruce Fields 2014-05-27 12:50 ` Jeff Layton 2014-05-27 15:28 ` J. Bruce Fields 2014-05-21 16:05 ` [PATCH 2/2] nfsd4: warn on finding lockowner without stateid's J. Bruce Fields 2014-05-21 19:14 ` [PATCH 0/2] fix for find_lockowner_str crash Jeff Layton
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.