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