* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).