Hi Bruce,
After staring at this code for a _long_ time, I think I've finally found
the source of the use-after-free oops that HCH spotted. The first patch
in this series should fix that. The other two patches are just cleanups
that I generated while staring at the code.
The first one obviously needs to go into v4.0 (and stable) ASAP. The
other two can wait for v4.1.
Jeff Layton (3):
nfsd: return correct openowner when there is a race to put one in the
hash
nfsd: remove bogus setting of status in nfsd4_process_open2
nfsd: remove unused status arg to nfsd4_cleanup_open_state
fs/nfsd/nfs4proc.c | 2 +-
fs/nfsd/nfs4state.c | 5 ++---
fs/nfsd/xdr4.h | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
--
2.1.0
alloc_init_open_stateowner can return an already freed entry if there is
a race to put openowners in the hashtable.
In commit 7ffb588086e9, we changed it so that we allocate and initialize
an openowner, and then check to see if a matching one got stuffed into
the hashtable in the meantime. If it did, then we free the one we just
allocated and take a reference on the one already there. There is a bug
here though. The code will then return the pointer to the one that was
allocated (and has now been freed).
This wasn't evident before as this race almost never occurred. The Linux
kernel client used to serialize requests for a single openowner. That
has changed now with v4.0 kernels, and this race can now easily occur.
Fixes: 7ffb588086e9
Cc: <[email protected]> # v3.17+
Cc: Trond Myklebust <[email protected]>
Reported-by: Christoph Hellwig <[email protected]>
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d2f2c37dc2db..cf29d1a698b3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3221,7 +3221,7 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
} else
nfs4_free_openowner(&oo->oo_owner);
spin_unlock(&clp->cl_lock);
- return oo;
+ return ret;
}
static void init_open_stateid(struct nfs4_ol_stateid *stp, struct nfs4_file *fp, struct nfsd4_open *open) {
--
2.1.0
status is always reset after this (and it doesn't make much sense there
anyway).
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4state.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cf29d1a698b3..a66e297dc01c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4049,7 +4049,6 @@ nfsd4_process_open2(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nf
status = nfserr_bad_stateid;
if (nfsd4_is_deleg_cur(open))
goto out;
- status = nfserr_jukebox;
}
/*
--
2.1.0
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfs4proc.c | 2 +-
fs/nfsd/nfs4state.c | 2 +-
fs/nfsd/xdr4.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index d30bea8d0277..e54f18ae5b2e 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -470,7 +470,7 @@ out:
fh_put(resfh);
kfree(resfh);
}
- nfsd4_cleanup_open_state(cstate, open, status);
+ nfsd4_cleanup_open_state(cstate, open);
nfsd4_bump_seqid(cstate, status);
return status;
}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a66e297dc01c..3d9b0b9b1e2c 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4117,7 +4117,7 @@ out:
}
void nfsd4_cleanup_open_state(struct nfsd4_compound_state *cstate,
- struct nfsd4_open *open, __be32 status)
+ struct nfsd4_open *open)
{
if (open->op_openowner) {
struct nfs4_stateowner *so = &open->op_openowner->oo_owner;
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 0bda93e58e1b..83e80d6d94a6 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -683,7 +683,7 @@ extern __be32 nfsd4_process_open2(struct svc_rqst *rqstp,
struct svc_fh *current_fh, struct nfsd4_open *open);
extern void nfsd4_cstate_clear_replay(struct nfsd4_compound_state *cstate);
extern void nfsd4_cleanup_open_state(struct nfsd4_compound_state *cstate,
- struct nfsd4_open *open, __be32 status);
+ struct nfsd4_open *open);
extern __be32 nfsd4_open_confirm(struct svc_rqst *rqstp,
struct nfsd4_compound_state *, struct nfsd4_open_confirm *oc);
extern __be32 nfsd4_close(struct svc_rqst *rqstp,
--
2.1.0
On Mon, Mar 23, 2015 at 10:53:41AM -0400, Jeff Layton wrote:
> After staring at this code for a _long_ time, I think I've finally found
> the source of the use-after-free oops that HCH spotted. The first patch
> in this series should fix that. The other two patches are just cleanups
> that I generated while staring at the code.
>
> The first one obviously needs to go into v4.0 (and stable) ASAP. The
> other two can wait for v4.1.
Zowie, thanks! It's a relief to have this one found....--b.
>
> Jeff Layton (3):
> nfsd: return correct openowner when there is a race to put one in the
> hash
> nfsd: remove bogus setting of status in nfsd4_process_open2
> nfsd: remove unused status arg to nfsd4_cleanup_open_state
>
> fs/nfsd/nfs4proc.c | 2 +-
> fs/nfsd/nfs4state.c | 5 ++---
> fs/nfsd/xdr4.h | 2 +-
> 3 files changed, 4 insertions(+), 5 deletions(-)
>
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 23 Mar 2015 11:12:57 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Mon, Mar 23, 2015 at 10:53:41AM -0400, Jeff Layton wrote:
> > After staring at this code for a _long_ time, I think I've finally found
> > the source of the use-after-free oops that HCH spotted. The first patch
> > in this series should fix that. The other two patches are just cleanups
> > that I generated while staring at the code.
> >
> > The first one obviously needs to go into v4.0 (and stable) ASAP. The
> > other two can wait for v4.1.
>
> Zowie, thanks! It's a relief to have this one found....--b.
>
Definitely a relief. Just to reiterate what you spotted on IRC too, it
looks like there's a similar bug in alloc_init_lock_stateowner so we'll
need a patch for that as well.
> >
> > Jeff Layton (3):
> > nfsd: return correct openowner when there is a race to put one in the
> > hash
> > nfsd: remove bogus setting of status in nfsd4_process_open2
> > nfsd: remove unused status arg to nfsd4_cleanup_open_state
> >
> > fs/nfsd/nfs4proc.c | 2 +-
> > fs/nfsd/nfs4state.c | 5 ++---
> > fs/nfsd/xdr4.h | 2 +-
> > 3 files changed, 4 insertions(+), 5 deletions(-)
> >
> > --
> > 2.1.0
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jeff Layton <[email protected]>
On Mon, Mar 23, 2015 at 10:53:42AM -0400, Jeff Layton wrote:
> alloc_init_open_stateowner can return an already freed entry if there is
> a race to put openowners in the hashtable.
Looks like alloc_init_lock_stateowner has the same bug, so I'll apply
something like this pending testing.
I wonder if it's actually possible to hit this one?
--b.
commit bdff3084f09f
Author: J. Bruce Fields <[email protected]>
Date: Mon Mar 23 11:02:30 2015 -0400
nfsd: return correct lockowner when there is a race on hash insert
alloc_init_lock_stateowner can return an already freed entry if there is
a race to put openowners in the hashtable.
Noticed by inspection after Jeff Layton fixed the same bug for open
owners. Depending on client behavior, this one may be trickier to
trigger in practice.
Fixes: c58c6610ec24 "nfsd: Protect adding/removing lock owners using client_lock"
Cc: [email protected]>
Cc: Trond Myklebust <[email protected]>
Cc: Jeff Layton <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index d2f2c37dc2db..49ae6116992f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -5062,7 +5062,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
} else
nfs4_free_lockowner(&lo->lo_owner);
spin_unlock(&clp->cl_lock);
- return lo;
+ return ret;
}
static void
On Mon, 23 Mar 2015 11:36:14 -0400
"J. Bruce Fields" <[email protected]> wrote:
> On Mon, Mar 23, 2015 at 10:53:42AM -0400, Jeff Layton wrote:
> > alloc_init_open_stateowner can return an already freed entry if there is
> > a race to put openowners in the hashtable.
>
> Looks like alloc_init_lock_stateowner has the same bug, so I'll apply
> something like this pending testing.
>
> I wonder if it's actually possible to hit this one?
>
> --b.
>
> commit bdff3084f09f
> Author: J. Bruce Fields <[email protected]>
> Date: Mon Mar 23 11:02:30 2015 -0400
>
> nfsd: return correct lockowner when there is a race on hash insert
>
> alloc_init_lock_stateowner can return an already freed entry if there is
> a race to put openowners in the hashtable.
>
> Noticed by inspection after Jeff Layton fixed the same bug for open
> owners. Depending on client behavior, this one may be trickier to
> trigger in practice.
>
> Fixes: c58c6610ec24 "nfsd: Protect adding/removing lock owners using client_lock"
> Cc: [email protected]>
> Cc: Trond Myklebust <[email protected]>
> Cc: Jeff Layton <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index d2f2c37dc2db..49ae6116992f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5062,7 +5062,7 @@ alloc_init_lock_stateowner(unsigned int strhashval, struct nfs4_client *clp,
> } else
> nfs4_free_lockowner(&lo->lo_owner);
> spin_unlock(&clp->cl_lock);
> - return lo;
> + return ret;
> }
>
> static void
Maybe. Commit b4019c0e219 allows us to do parallel LOCK/LOCKU calls for
the same owner, but it also says:
"Note, however, that we still serialise on the open stateid if the lock
stateid is unconfirmed."
...which I take to mean that the initial LOCK calls likely wouldn't race
this way. Still, it's probably possible to craft a pynfs test or
something that does that.
Either way, it's clearly a bug that should be fixed:
Acked-by: Jeff Layton <[email protected]>
Good catch!
Reviewed-by: Christoph Hellwig <[email protected]>
But maybe it's better to write the code so that the intent is obvious:
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f6b2a09..3c4c3d2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3215,11 +3215,13 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
INIT_LIST_HEAD(&oo->oo_close_lru);
spin_lock(&clp->cl_lock);
ret = find_openstateowner_str_locked(strhashval, open, clp);
- if (ret == NULL) {
- hash_openowner(oo, clp, strhashval);
- ret = oo;
- } else
+ if (ret) {
nfs4_free_openowner(&oo->oo_owner);
+ spin_unlock(&clp->cl_lock);
+ return ret;
+ }
+
+ hash_openowner(oo, clp, strhashval);
spin_unlock(&clp->cl_lock);
return oo;
}
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
On Wed, 25 Mar 2015 01:45:16 -0700
Christoph Hellwig <[email protected]> wrote:
> Good catch!
>
> Reviewed-by: Christoph Hellwig <[email protected]>
>
> But maybe it's better to write the code so that the intent is obvious:
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index f6b2a09..3c4c3d2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3215,11 +3215,13 @@ alloc_init_open_stateowner(unsigned int strhashval, struct nfsd4_open *open,
> INIT_LIST_HEAD(&oo->oo_close_lru);
> spin_lock(&clp->cl_lock);
> ret = find_openstateowner_str_locked(strhashval, open, clp);
> - if (ret == NULL) {
> - hash_openowner(oo, clp, strhashval);
> - ret = oo;
> - } else
> + if (ret) {
> nfs4_free_openowner(&oo->oo_owner);
> + spin_unlock(&clp->cl_lock);
> + return ret;
> + }
> +
> + hash_openowner(oo, clp, strhashval);
> spin_unlock(&clp->cl_lock);
> return oo;
> }
Sure, that'd be fine too. Care to write up a changelog entry and send
it to Bruce? I think that would likely apply just fine to stable
kernels.
--
Jeff Layton <[email protected]>
Let's just keep your version for simplicity for now..