[PATCH] nfsd4: fix leak of inode reference on delegation failure

[PATCH] nfsd4: fix leak of inode reference on delegation failure

[J. Bruce Fields]

From: "J. Bruce Fields" <bfields@redhat.com>

commit bf7bd3e98be5c74813bee6ad496139fb0a011b3b upstream, for >= 3.10.

This fixes a regression from 68a3396178e6688ad7367202cdf0af8ed03c8727
"nfsd4: shut down more of delegation earlier".

After that commit, nfs4_set_delegation() failures result in
nfs4_put_delegation being called, but nfs4_put_delegation doesn't free
the nfs4_file that has already been set by alloc_init_deleg().

This can result in an oops on later unmounting the exported filesystem.

Note also delaying the fi_had_conflict check we're able to return a
better error (hence give 4.1 clients a better idea why the delegation
failed; though note CONFLICT isn't an exact match here, as that's
supposed to indicate a current conflict, but all we know here is that
there was one recently).

Reported-by: Toralf Förster <toralf.foerster@gmx.de>
Tested-by: Toralf Förster <toralf.foerster@gmx.de>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4state.c |   31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index eb9cf81..0874998 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -368,11 +368,8 @@ static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh)
 {
 	struct nfs4_delegation *dp;
-	struct nfs4_file *fp = stp->st_file;
 
 	dprintk("NFSD alloc_init_deleg\n");
-	if (fp->fi_had_conflict)
-		return NULL;
 	if (num_delegations > max_delegations)
 		return NULL;
 	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
@@ -389,8 +386,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	INIT_LIST_HEAD(&dp->dl_perfile);
 	INIT_LIST_HEAD(&dp->dl_perclnt);
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
-	get_nfs4_file(fp);
-	dp->dl_file = fp;
+	dp->dl_file = NULL;
 	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
 	dp->dl_time = 0;
@@ -3044,22 +3040,35 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	return 0;
 }
 
-static int nfs4_set_delegation(struct nfs4_delegation *dp)
+static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 {
-	struct nfs4_file *fp = dp->dl_file;
+	int status;
 
-	if (!fp->fi_lease)
-		return nfs4_setlease(dp);
+	if (fp->fi_had_conflict)
+		return -EAGAIN;
+	get_nfs4_file(fp);
+	dp->dl_file = fp;
+	if (!fp->fi_lease) {
+		status = nfs4_setlease(dp);
+		if (status)
+			goto out_free;
+		return 0;
+	}
 	spin_lock(&recall_lock);
 	if (fp->fi_had_conflict) {
 		spin_unlock(&recall_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_free;
 	}
 	atomic_inc(&fp->fi_delegees);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
 	spin_unlock(&recall_lock);
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 	return 0;
+out_free:
+	put_nfs4_file(fp);
+	dp->dl_file = fp;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
@@ -3134,7 +3143,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 	dp = alloc_init_deleg(oo->oo_owner.so_client, stp, fh);
 	if (dp == NULL)
 		goto out_no_deleg;
-	status = nfs4_set_delegation(dp);
+	status = nfs4_set_delegation(dp, stp->st_file);
 	if (status)
 		goto out_free;
 
-- 
1.7.9.5

Re: [PATCH] nfsd4: fix leak of inode reference on delegation failure

[J. Bruce Fields]

Whoops, apologies, this requires a little fixing up to backport.  Ignore
for now.

--b.

On Tue, Oct 01, 2013 at 10:42:50AM -0400, bfields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> commit bf7bd3e98be5c74813bee6ad496139fb0a011b3b upstream, for >= 3.10.
> 
> This fixes a regression from 68a3396178e6688ad7367202cdf0af8ed03c8727
> "nfsd4: shut down more of delegation earlier".
> 
> After that commit, nfs4_set_delegation() failures result in
> nfs4_put_delegation being called, but nfs4_put_delegation doesn't free
> the nfs4_file that has already been set by alloc_init_deleg().
> 
> This can result in an oops on later unmounting the exported filesystem.
> 
> Note also delaying the fi_had_conflict check we're able to return a
> better error (hence give 4.1 clients a better idea why the delegation
> failed; though note CONFLICT isn't an exact match here, as that's
> supposed to indicate a current conflict, but all we know here is that
> there was one recently).
> 
> Reported-by: Toralf Förster <toralf.foerster@gmx.de>
> Tested-by: Toralf Förster <toralf.foerster@gmx.de>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfs4state.c |   31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index eb9cf81..0874998 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -368,11 +368,8 @@ static struct nfs4_delegation *
>  alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh)
>  {
>  	struct nfs4_delegation *dp;
> -	struct nfs4_file *fp = stp->st_file;
>  
>  	dprintk("NFSD alloc_init_deleg\n");
> -	if (fp->fi_had_conflict)
> -		return NULL;
>  	if (num_delegations > max_delegations)
>  		return NULL;
>  	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
> @@ -389,8 +386,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
>  	INIT_LIST_HEAD(&dp->dl_perfile);
>  	INIT_LIST_HEAD(&dp->dl_perclnt);
>  	INIT_LIST_HEAD(&dp->dl_recall_lru);
> -	get_nfs4_file(fp);
> -	dp->dl_file = fp;
> +	dp->dl_file = NULL;
>  	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>  	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
>  	dp->dl_time = 0;
> @@ -3044,22 +3040,35 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
>  	return 0;
>  }
>  
> -static int nfs4_set_delegation(struct nfs4_delegation *dp)
> +static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
>  {
> -	struct nfs4_file *fp = dp->dl_file;
> +	int status;
>  
> -	if (!fp->fi_lease)
> -		return nfs4_setlease(dp);
> +	if (fp->fi_had_conflict)
> +		return -EAGAIN;
> +	get_nfs4_file(fp);
> +	dp->dl_file = fp;
> +	if (!fp->fi_lease) {
> +		status = nfs4_setlease(dp);
> +		if (status)
> +			goto out_free;
> +		return 0;
> +	}
>  	spin_lock(&recall_lock);
>  	if (fp->fi_had_conflict) {
>  		spin_unlock(&recall_lock);
> -		return -EAGAIN;
> +		status = -EAGAIN;
> +		goto out_free;
>  	}
>  	atomic_inc(&fp->fi_delegees);
>  	list_add(&dp->dl_perfile, &fp->fi_delegations);
>  	spin_unlock(&recall_lock);
>  	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
>  	return 0;
> +out_free:
> +	put_nfs4_file(fp);
> +	dp->dl_file = fp;
> +	return status;
>  }
>  
>  static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
> @@ -3134,7 +3143,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
>  	dp = alloc_init_deleg(oo->oo_owner.so_client, stp, fh);
>  	if (dp == NULL)
>  		goto out_no_deleg;
> -	status = nfs4_set_delegation(dp);
> +	status = nfs4_set_delegation(dp, stp->st_file);
>  	if (status)
>  		goto out_free;
>  
> -- 
> 1.7.9.5
> 

Re: [PATCH] nfsd4: fix leak of inode reference on delegation failure

[Greg KH]

On Tue, Oct 01, 2013 at 10:42:50AM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> commit bf7bd3e98be5c74813bee6ad496139fb0a011b3b upstream, for >= 3.10.

This doesn't apply at all to 3.10, but does to 3.11.  If you want it in
3.10, can you provide a backport that works?

thanks,

greg k-h

Re: [PATCH] nfsd4: fix leak of inode reference on delegation failure

[Greg KH]

On Wed, Oct 02, 2013 at 10:03:32AM -0400, J. Bruce Fields wrote:
> Whoops, apologies, this requires a little fixing up to backport.  Ignore
> for now.

Even for 3.11?  It seems to build there just fine.

thanks,

greg k-h

Re: [PATCH] nfsd4: fix leak of inode reference on delegation failure

[J. Bruce Fields]

On Sat, Oct 05, 2013 at 04:46:12PM -0700, Greg KH wrote:
> On Wed, Oct 02, 2013 at 10:03:32AM -0400, J. Bruce Fields wrote:
> > Whoops, apologies, this requires a little fixing up to backport.  Ignore
> > for now.
> 
> Even for 3.11?  It seems to build there just fine.

Please do apply for 3.11, thanks for noticing that.

Then if/when this gets backported to 3.10 we'll send that separately.

Apologies for the confusion.

--b.

[PATCH] nfsd4: fix leak of inode reference on delegation failure

[=?UTF-8?q?Tuomas=20R=C3=A4s=C3=A4nen?=]

From: J. Bruce Fields <bfields@redhat.com>

commit bf7bd3e98be5c74813bee6ad496139fb0a011b3b upstream.

This fixes a regression from 68a3396178e6688ad7367202cdf0af8ed03c8727
"nfsd4: shut down more of delegation earlier".

After that commit, nfs4_set_delegation() failures result in
nfs4_put_delegation being called, but nfs4_put_delegation doesn't free
the nfs4_file that has already been set by alloc_init_deleg().

This can result in an oops on later unmounting the exported filesystem.

Note also delaying the fi_had_conflict check we're able to return a
better error (hence give 4.1 clients a better idea why the delegation
failed; though note CONFLICT isn't an exact match here, as that's
supposed to indicate a current conflict, but all we know here is that
there was one recently).

Reported-by: Toralf Förster <toralf.foerster@gmx.de>
Tested-by: Toralf Förster <toralf.foerster@gmx.de>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
[tuomasjjrasanen: backported to 3.10
   Conflicts fs/nfsd/nfs4state.c:
     Delegation type flags have been removed from upstream code. In 3.10-series,
     they still exists and therefore the commit caused few conflicts in function
     signatures.
]
Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@opinsys.fi>
---
 fs/nfsd/nfs4state.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index bdff771..836307a 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -367,7 +367,6 @@ static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh, u32 type)
 {
 	struct nfs4_delegation *dp;
-	struct nfs4_file *fp = stp->st_file;
 
 	dprintk("NFSD alloc_init_deleg\n");
 	/*
@@ -377,8 +376,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	 */
 	if (type != NFS4_OPEN_DELEGATE_READ)
 		return NULL;
-	if (fp->fi_had_conflict)
-		return NULL;
 	if (num_delegations > max_delegations)
 		return NULL;
 	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
@@ -395,8 +392,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
 	INIT_LIST_HEAD(&dp->dl_perfile);
 	INIT_LIST_HEAD(&dp->dl_perclnt);
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
-	get_nfs4_file(fp);
-	dp->dl_file = fp;
+	dp->dl_file = NULL;
 	dp->dl_type = type;
 	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
 	dp->dl_time = 0;
@@ -2965,22 +2961,35 @@ static int nfs4_setlease(struct nfs4_delegation *dp, int flag)
 	return 0;
 }
 
-static int nfs4_set_delegation(struct nfs4_delegation *dp, int flag)
+static int nfs4_set_delegation(struct nfs4_delegation *dp, int flag, struct nfs4_file *fp)
 {
-	struct nfs4_file *fp = dp->dl_file;
+	int status;
 
-	if (!fp->fi_lease)
-		return nfs4_setlease(dp, flag);
+	if (fp->fi_had_conflict)
+		return -EAGAIN;
+	get_nfs4_file(fp);
+	dp->dl_file = fp;
+	if (!fp->fi_lease) {
+		status = nfs4_setlease(dp, flag);
+		if (status)
+			goto out_free;
+		return 0;
+	}
 	spin_lock(&recall_lock);
 	if (fp->fi_had_conflict) {
 		spin_unlock(&recall_lock);
-		return -EAGAIN;
+		status = -EAGAIN;
+		goto out_free;
 	}
 	atomic_inc(&fp->fi_delegees);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
 	spin_unlock(&recall_lock);
 	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 	return 0;
+out_free:
+	put_nfs4_file(fp);
+	dp->dl_file = fp;
+	return status;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
@@ -3046,7 +3055,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
 	dp = alloc_init_deleg(oo->oo_owner.so_client, stp, fh, flag);
 	if (dp == NULL)
 		goto out_no_deleg;
-	status = nfs4_set_delegation(dp, flag);
+	status = nfs4_set_delegation(dp, flag, stp->st_file);
 	if (status)
 		goto out_free;
 
-- 
2.1.3

Re: [PATCH] nfsd4: fix leak of inode reference on delegation failure

[J. Bruce Fields]

Acked-by: J. Bruce Fields <bfields@redhat.com>

On Tue, Nov 25, 2014 at 11:05:21AM +0200, =?UTF-8?q?Tuomas=20R=C3=A4s=C3=A4nen?= wrote:
> From: J. Bruce Fields <bfields@redhat.com>
> 
> commit bf7bd3e98be5c74813bee6ad496139fb0a011b3b upstream.
> 
> This fixes a regression from 68a3396178e6688ad7367202cdf0af8ed03c8727
> "nfsd4: shut down more of delegation earlier".
> 
> After that commit, nfs4_set_delegation() failures result in
> nfs4_put_delegation being called, but nfs4_put_delegation doesn't free
> the nfs4_file that has already been set by alloc_init_deleg().
> 
> This can result in an oops on later unmounting the exported filesystem.
> 
> Note also delaying the fi_had_conflict check we're able to return a
> better error (hence give 4.1 clients a better idea why the delegation
> failed; though note CONFLICT isn't an exact match here, as that's
> supposed to indicate a current conflict, but all we know here is that
> there was one recently).
> 
> Reported-by: Toralf Förster <toralf.foerster@gmx.de>
> Tested-by: Toralf Förster <toralf.foerster@gmx.de>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> [tuomasjjrasanen: backported to 3.10
>    Conflicts fs/nfsd/nfs4state.c:
>      Delegation type flags have been removed from upstream code. In 3.10-series,
>      they still exists and therefore the commit caused few conflicts in function
>      signatures.
> ]
> Signed-off-by: Tuomas Räsänen <tuomasjjrasanen@opinsys.fi>
> ---
>  fs/nfsd/nfs4state.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index bdff771..836307a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -367,7 +367,6 @@ static struct nfs4_delegation *
>  alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct svc_fh *current_fh, u32 type)
>  {
>  	struct nfs4_delegation *dp;
> -	struct nfs4_file *fp = stp->st_file;
>  
>  	dprintk("NFSD alloc_init_deleg\n");
>  	/*
> @@ -377,8 +376,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
>  	 */
>  	if (type != NFS4_OPEN_DELEGATE_READ)
>  		return NULL;
> -	if (fp->fi_had_conflict)
> -		return NULL;
>  	if (num_delegations > max_delegations)
>  		return NULL;
>  	dp = delegstateid(nfs4_alloc_stid(clp, deleg_slab));
> @@ -395,8 +392,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_ol_stateid *stp, struct sv
>  	INIT_LIST_HEAD(&dp->dl_perfile);
>  	INIT_LIST_HEAD(&dp->dl_perclnt);
>  	INIT_LIST_HEAD(&dp->dl_recall_lru);
> -	get_nfs4_file(fp);
> -	dp->dl_file = fp;
> +	dp->dl_file = NULL;
>  	dp->dl_type = type;
>  	fh_copy_shallow(&dp->dl_fh, &current_fh->fh_handle);
>  	dp->dl_time = 0;
> @@ -2965,22 +2961,35 @@ static int nfs4_setlease(struct nfs4_delegation *dp, int flag)
>  	return 0;
>  }
>  
> -static int nfs4_set_delegation(struct nfs4_delegation *dp, int flag)
> +static int nfs4_set_delegation(struct nfs4_delegation *dp, int flag, struct nfs4_file *fp)
>  {
> -	struct nfs4_file *fp = dp->dl_file;
> +	int status;
>  
> -	if (!fp->fi_lease)
> -		return nfs4_setlease(dp, flag);
> +	if (fp->fi_had_conflict)
> +		return -EAGAIN;
> +	get_nfs4_file(fp);
> +	dp->dl_file = fp;
> +	if (!fp->fi_lease) {
> +		status = nfs4_setlease(dp, flag);
> +		if (status)
> +			goto out_free;
> +		return 0;
> +	}
>  	spin_lock(&recall_lock);
>  	if (fp->fi_had_conflict) {
>  		spin_unlock(&recall_lock);
> -		return -EAGAIN;
> +		status = -EAGAIN;
> +		goto out_free;
>  	}
>  	atomic_inc(&fp->fi_delegees);
>  	list_add(&dp->dl_perfile, &fp->fi_delegations);
>  	spin_unlock(&recall_lock);
>  	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
>  	return 0;
> +out_free:
> +	put_nfs4_file(fp);
> +	dp->dl_file = fp;
> +	return status;
>  }
>  
>  static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
> @@ -3046,7 +3055,7 @@ nfs4_open_delegation(struct net *net, struct svc_fh *fh,
>  	dp = alloc_init_deleg(oo->oo_owner.so_client, stp, fh, flag);
>  	if (dp == NULL)
>  		goto out_no_deleg;
> -	status = nfs4_set_delegation(dp, flag);
> +	status = nfs4_set_delegation(dp, flag, stp->st_file);
>  	if (status)
>  		goto out_free;
>  
> -- 
> 2.1.3
>