* [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
* [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
* 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
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).