[PATCH 0/3] nfsd: fix use-after-free oops in v4.0 (and some other minor cleanups)

[PATCH 0/3] nfsd: fix use-after-free oops in v4.0 (and some other minor cleanups)

[Jeff Layton]

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

[PATCH 1/3] nfsd: return correct openowner when there is a race to put one in the hash

[Jeff Layton]

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: <stable@vger.kernel.org> # v3.17+
Cc: Trond Myklebust <trond.myklebust@primarydata.com>
Reported-by: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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

Re: [PATCH 1/3] nfsd: return correct openowner when there is a race to put one in the hash

[J. Bruce Fields]

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 <bfields@redhat.com>
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: stable@vger.kernel.org>
    Cc: Trond Myklebust <trond.myklebust@primarydata.com>
    Cc: 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 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

Re: [PATCH 1/3] nfsd: return correct openowner when there is a race to put one in the hash

[Jeff Layton]

On Mon, 23 Mar 2015 11:36:14 -0400
"J. Bruce Fields" <bfields@fieldses.org> 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 <bfields@redhat.com>
> 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: stable@vger.kernel.org>
>     Cc: Trond Myklebust <trond.myklebust@primarydata.com>
>     Cc: 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 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 <jeff.layton@primarydata.com>

Re: [PATCH 1/3] nfsd: return correct openowner when there is a race to put one in the hash

[Christoph Hellwig]

Good catch!

Reviewed-by: Christoph Hellwig <hch@lst.de>

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;
 }

Re: [PATCH 1/3] nfsd: return correct openowner when there is a race to put one in the hash

[Jeff Layton]

On Wed, 25 Mar 2015 01:45:16 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> Good catch!
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> 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 <jlayton@poochiereds.net>

Re: [PATCH 1/3] nfsd: return correct openowner when there is a race to put one in the hash

[Christoph Hellwig]

Let's just keep your version for simplicity for now..

[PATCH 2/3] nfsd: remove bogus setting of status in nfsd4_process_open2

[Jeff Layton]

status is always reset after this (and it doesn't make much sense there
anyway).

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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

Re: [PATCH 2/3] nfsd: remove bogus setting of status in nfsd4_process_open2

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH 3/3] nfsd: remove unused status arg to nfsd4_cleanup_open_state

[Jeff Layton]

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 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

Re: [PATCH 3/3] nfsd: remove unused status arg to nfsd4_cleanup_open_state

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/3] nfsd: fix use-after-free oops in v4.0 (and some other minor cleanups)

[J. Bruce Fields]

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] nfsd: fix use-after-free oops in v4.0 (and some other minor cleanups)

[Jeff Layton]

On Mon, 23 Mar 2015 11:12:57 -0400
"J. Bruce Fields" <bfields@fieldses.org> 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 majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@poochiereds.net>