[PATCH 1/5] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[PATCH 1/5] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[Kinglong Mee]

If nfsd4_layout_setlease fail, nfsd will not put ls->ls_file.

Fix commit c5c707f96f "nfsd: implement pNFS layout recalls".

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4layouts.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 6904213..367a65a 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -212,8 +212,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
 	BUG_ON(!ls->ls_file);
 
 	if (nfsd4_layout_setlease(ls)) {
-		put_nfs4_file(fp);
-		kmem_cache_free(nfs4_layout_stateid_cache, ls);
+		nfs4_put_stid(stp);
 		return NULL;
 	}
 
-- 
2.4.3

[PATCH 2/5] nfsd: Drop including client's header file nfs_fs.h

[Kinglong Mee]

nfs_fs.h is a client's header file.

# ll fs/nfsd/nfs4acl.o fs/nfsd/nfsd.ko
-rw-r--r--. 1 root root 328248 Jul  3 19:26 fs/nfsd/nfs4acl.o
-rw-r--r--. 1 root root 7452016 Jul  3 19:26 fs/nfsd/nfsd.ko

After this patch,
# ll fs/nfsd/nfs4acl.o fs/nfsd/nfsd.ko
-rw-r--r--. 1 root root 150872 Jul  3 19:15 fs/nfsd/nfs4acl.o
-rw-r--r--. 1 root root 7273792 Jul  3 19:23 fs/nfsd/nfsd.ko

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4acl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4acl.c b/fs/nfsd/nfs4acl.c
index eb5accf..4b939b0 100644
--- a/fs/nfsd/nfs4acl.c
+++ b/fs/nfsd/nfs4acl.c
@@ -34,8 +34,10 @@
  *  SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <linux/fs.h>
 #include <linux/slab.h>
-#include <linux/nfs_fs.h>
+#include <linux/posix_acl.h>
+
 #include "nfsfh.h"
 #include "nfsd.h"
 #include "acl.h"
-- 
2.4.3

[PATCH 3/5] nfsd: Remove duplicate define of IDMAP_NAMESZ/IDMAP_TYPE_xx

[Kinglong Mee]

Just using the macro defined in nfs_idmap.h.

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/idmap.h     | 4 +---
 fs/nfsd/nfs4idmap.c | 3 ---
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/nfsd/idmap.h b/fs/nfsd/idmap.h
index a3f3490..23cc85d 100644
--- a/fs/nfsd/idmap.h
+++ b/fs/nfsd/idmap.h
@@ -37,9 +37,7 @@
 
 #include <linux/in.h>
 #include <linux/sunrpc/svc.h>
-
-/* XXX from linux/nfs_idmap.h */
-#define IDMAP_NAMESZ 128
+#include <linux/nfs_idmap.h>
 
 #ifdef CONFIG_NFSD_V4
 int nfsd_idmap_init(struct net *);
diff --git a/fs/nfsd/nfs4idmap.c b/fs/nfsd/nfs4idmap.c
index e1b3d3d..5b20577 100644
--- a/fs/nfsd/nfs4idmap.c
+++ b/fs/nfsd/nfs4idmap.c
@@ -59,9 +59,6 @@ MODULE_PARM_DESC(nfs4_disable_idmapping,
  * that.
  */
 
-#define IDMAP_TYPE_USER  0
-#define IDMAP_TYPE_GROUP 1
-
 struct ent {
 	struct cache_head h;
 	int               type;		       /* User / Group */
-- 
2.4.3

[PATCH 4/5] nfsd: Check stateid generation in nfsd4_lookup_stateid()

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4layouts.c |  2 --
 fs/nfsd/nfs4state.c   | 30 ++++++++++++++----------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 367a65a..ef63244 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -264,8 +264,6 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
 		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
 
 		status = nfserr_bad_stateid;
-		if (stateid->si_generation > stid->sc_stateid.si_generation)
-			goto out_put_stid;
 		if (layout_type != ls->ls_layout_type)
 			goto out_put_stid;
 	}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 61dfb33..53248cd 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4555,6 +4555,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		     stateid_t *stateid, unsigned char typemask,
 		     struct nfs4_stid **s, struct nfsd_net *nn)
 {
+	struct nfs4_stid *stid;
 	__be32 status;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
@@ -4567,10 +4568,18 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 	}
 	if (status)
 		return status;
-	*s = find_stateid_by_type(cstate->clp, stateid, typemask);
-	if (!*s)
+	stid = find_stateid_by_type(cstate->clp, stateid, typemask);
+	if (!stid)
 		return nfserr_bad_stateid;
-	return nfs_ok;
+
+	status = check_stateid_generation(stateid, &stid->sc_stateid,
+			nfsd4_has_session(cstate));
+	if (status)
+		nfs4_put_stid(stid);
+	else
+		*s = stid;
+
+	return status;
 }
 
 static struct file *
@@ -4673,10 +4682,6 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 				&s, nn);
 	if (status)
 		return status;
-	status = check_stateid_generation(stateid, &s->sc_stateid,
-			nfsd4_has_session(cstate));
-	if (status)
-		goto out;
 
 	switch (s->sc_type) {
 	case NFS4_DELEG_STID:
@@ -4694,7 +4699,6 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
 done:
 	if (!status && filpp)
 		status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
-out:
 	if (s)
 		nfs4_put_stid(s);
 	return status;
@@ -5021,7 +5025,6 @@ __be32
 nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		  struct nfsd4_delegreturn *dr)
 {
-	struct nfs4_delegation *dp;
 	stateid_t *stateid = &dr->dr_stateid;
 	struct nfs4_stid *s;
 	__be32 status;
@@ -5033,14 +5036,9 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
 	if (status)
 		goto out;
-	dp = delegstateid(s);
-	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
-	if (status)
-		goto put_stateid;
 
-	destroy_delegation(dp);
-put_stateid:
-	nfs4_put_stid(&dp->dl_stid);
+	destroy_delegation(delegstateid(s));
+	nfs4_put_stid(s);
 out:
 	return status;
 }
-- 
2.4.3

[PATCH 5/5] nfsd: Add macro NFS_ACL_MASK for ACL

[Kinglong Mee]

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs2acl.c           | 10 ++++------
 fs/nfsd/nfs3acl.c           |  4 ++--
 include/uapi/linux/nfsacl.h |  1 +
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index d54701f..1580ea6 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -44,13 +44,13 @@ static __be32 nfsacld_proc_getacl(struct svc_rqst * rqstp,
 
 	inode = d_inode(fh->fh_dentry);
 
-	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT))
+	if (argp->mask & ~NFS_ACL_MASK)
 		RETURN_STATUS(nfserr_inval);
 	resp->mask = argp->mask;
 
 	nfserr = fh_getattr(fh, &resp->stat);
 	if (nfserr)
-		goto fail;
+		RETURN_STATUS(nfserr);
 
 	if (resp->mask & (NFS_ACL|NFS_ACLCNT)) {
 		acl = get_acl(inode, ACL_TYPE_ACCESS);
@@ -202,7 +202,7 @@ static int nfsaclsvc_decode_setaclargs(struct svc_rqst *rqstp, __be32 *p,
 	if (!p)
 		return 0;
 	argp->mask = ntohl(*p++);
-	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT) ||
+	if (argp->mask & ~NFS_ACL_MASK ||
 	    !xdr_argsize_check(rqstp, p))
 		return 0;
 
@@ -293,9 +293,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
 				  resp->acl_default,
 				  resp->mask & NFS_DFACL,
 				  NFS_ACL_DEFAULT);
-	if (n <= 0)
-		return 0;
-	return 1;
+	return (n > 0);
 }
 
 static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
index 882b1a1..01df4cd 100644
--- a/fs/nfsd/nfs3acl.c
+++ b/fs/nfsd/nfs3acl.c
@@ -41,7 +41,7 @@ static __be32 nfsd3_proc_getacl(struct svc_rqst * rqstp,
 
 	inode = d_inode(fh->fh_dentry);
 
-	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT))
+	if (argp->mask & ~NFS_ACL_MASK)
 		RETURN_STATUS(nfserr_inval);
 	resp->mask = argp->mask;
 
@@ -148,7 +148,7 @@ static int nfs3svc_decode_setaclargs(struct svc_rqst *rqstp, __be32 *p,
 	if (!p)
 		return 0;
 	args->mask = ntohl(*p++);
-	if (args->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT) ||
+	if (args->mask & ~NFS_ACL_MASK ||
 	    !xdr_argsize_check(rqstp, p))
 		return 0;
 
diff --git a/include/uapi/linux/nfsacl.h b/include/uapi/linux/nfsacl.h
index 9bb9771..5527266 100644
--- a/include/uapi/linux/nfsacl.h
+++ b/include/uapi/linux/nfsacl.h
@@ -22,6 +22,7 @@
 #define NFS_ACLCNT		0x0002
 #define NFS_DFACL		0x0004
 #define NFS_DFACLCNT		0x0008
+#define NFS_ACL_MASK		0x000f
 
 /* Flag for Default ACL entries */
 #define NFS_ACL_DEFAULT		0x1000
-- 
2.4.3

Re: [PATCH 1/5] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[J. Bruce Fields]

On Fri, Jul 03, 2015 at 07:32:07PM +0800, Kinglong Mee wrote:
> If nfsd4_layout_setlease fail, nfsd will not put ls->ls_file.
> 
> Fix commit c5c707f96f "nfsd: implement pNFS layout recalls".
> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4layouts.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 6904213..367a65a 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -212,8 +212,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
>  	BUG_ON(!ls->ls_file);
>  
>  	if (nfsd4_layout_setlease(ls)) {
> -		put_nfs4_file(fp);
> -		kmem_cache_free(nfs4_layout_stateid_cache, ls);
> +		nfs4_put_stid(stp);

Hm, is the stateid really completely enough set up that this is safe?

Looking at nfsd4_free_layout_stateid.... OK, the unnecessary lease
unlock and tracepoint are a bit ugly bug I guess we can live with those.

--b.

>  		return NULL;
>  	}
>  
> -- 
> 2.4.3

Re: [PATCH 4/5] nfsd: Check stateid generation in nfsd4_lookup_stateid()

[J. Bruce Fields]

I think you overlooked preprocesse_seqid_op?  Does the reordering of the
stateid generation checking matter there?  I'm not sure.

--b.

On Fri, Jul 03, 2015 at 07:38:36PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4layouts.c |  2 --
>  fs/nfsd/nfs4state.c   | 30 ++++++++++++++----------------
>  2 files changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 367a65a..ef63244 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -264,8 +264,6 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>  		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
>  
>  		status = nfserr_bad_stateid;
> -		if (stateid->si_generation > stid->sc_stateid.si_generation)
> -			goto out_put_stid;
>  		if (layout_type != ls->ls_layout_type)
>  			goto out_put_stid;
>  	}
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 61dfb33..53248cd 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4555,6 +4555,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		     stateid_t *stateid, unsigned char typemask,
>  		     struct nfs4_stid **s, struct nfsd_net *nn)
>  {
> +	struct nfs4_stid *stid;
>  	__be32 status;
>  
>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
> @@ -4567,10 +4568,18 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  	}
>  	if (status)
>  		return status;
> -	*s = find_stateid_by_type(cstate->clp, stateid, typemask);
> -	if (!*s)
> +	stid = find_stateid_by_type(cstate->clp, stateid, typemask);
> +	if (!stid)
>  		return nfserr_bad_stateid;
> -	return nfs_ok;
> +
> +	status = check_stateid_generation(stateid, &stid->sc_stateid,
> +			nfsd4_has_session(cstate));
> +	if (status)
> +		nfs4_put_stid(stid);
> +	else
> +		*s = stid;
> +
> +	return status;
>  }
>  
>  static struct file *
> @@ -4673,10 +4682,6 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  				&s, nn);
>  	if (status)
>  		return status;
> -	status = check_stateid_generation(stateid, &s->sc_stateid,
> -			nfsd4_has_session(cstate));
> -	if (status)
> -		goto out;
>  
>  	switch (s->sc_type) {
>  	case NFS4_DELEG_STID:
> @@ -4694,7 +4699,6 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  done:
>  	if (!status && filpp)
>  		status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
> -out:
>  	if (s)
>  		nfs4_put_stid(s);
>  	return status;
> @@ -5021,7 +5025,6 @@ __be32
>  nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		  struct nfsd4_delegreturn *dr)
>  {
> -	struct nfs4_delegation *dp;
>  	stateid_t *stateid = &dr->dr_stateid;
>  	struct nfs4_stid *s;
>  	__be32 status;
> @@ -5033,14 +5036,9 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
>  	if (status)
>  		goto out;
> -	dp = delegstateid(s);
> -	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
> -	if (status)
> -		goto put_stateid;
>  
> -	destroy_delegation(dp);
> -put_stateid:
> -	nfs4_put_stid(&dp->dl_stid);
> +	destroy_delegation(delegstateid(s));
> +	nfs4_put_stid(s);
>  out:
>  	return status;
>  }
> -- 
> 2.4.3

Re: [PATCH 5/5] nfsd: Add macro NFS_ACL_MASK for ACL

[J. Bruce Fields]

On Fri, Jul 03, 2015 at 07:39:02PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>

OK, thanks.--b.

> ---
>  fs/nfsd/nfs2acl.c           | 10 ++++------
>  fs/nfsd/nfs3acl.c           |  4 ++--
>  include/uapi/linux/nfsacl.h |  1 +
>  3 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> index d54701f..1580ea6 100644
> --- a/fs/nfsd/nfs2acl.c
> +++ b/fs/nfsd/nfs2acl.c
> @@ -44,13 +44,13 @@ static __be32 nfsacld_proc_getacl(struct svc_rqst * rqstp,
>  
>  	inode = d_inode(fh->fh_dentry);
>  
> -	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT))
> +	if (argp->mask & ~NFS_ACL_MASK)
>  		RETURN_STATUS(nfserr_inval);
>  	resp->mask = argp->mask;
>  
>  	nfserr = fh_getattr(fh, &resp->stat);
>  	if (nfserr)
> -		goto fail;
> +		RETURN_STATUS(nfserr);
>  
>  	if (resp->mask & (NFS_ACL|NFS_ACLCNT)) {
>  		acl = get_acl(inode, ACL_TYPE_ACCESS);
> @@ -202,7 +202,7 @@ static int nfsaclsvc_decode_setaclargs(struct svc_rqst *rqstp, __be32 *p,
>  	if (!p)
>  		return 0;
>  	argp->mask = ntohl(*p++);
> -	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT) ||
> +	if (argp->mask & ~NFS_ACL_MASK ||
>  	    !xdr_argsize_check(rqstp, p))
>  		return 0;
>  
> @@ -293,9 +293,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
>  				  resp->acl_default,
>  				  resp->mask & NFS_DFACL,
>  				  NFS_ACL_DEFAULT);
> -	if (n <= 0)
> -		return 0;
> -	return 1;
> +	return (n > 0);
>  }
>  
>  static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
> diff --git a/fs/nfsd/nfs3acl.c b/fs/nfsd/nfs3acl.c
> index 882b1a1..01df4cd 100644
> --- a/fs/nfsd/nfs3acl.c
> +++ b/fs/nfsd/nfs3acl.c
> @@ -41,7 +41,7 @@ static __be32 nfsd3_proc_getacl(struct svc_rqst * rqstp,
>  
>  	inode = d_inode(fh->fh_dentry);
>  
> -	if (argp->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT))
> +	if (argp->mask & ~NFS_ACL_MASK)
>  		RETURN_STATUS(nfserr_inval);
>  	resp->mask = argp->mask;
>  
> @@ -148,7 +148,7 @@ static int nfs3svc_decode_setaclargs(struct svc_rqst *rqstp, __be32 *p,
>  	if (!p)
>  		return 0;
>  	args->mask = ntohl(*p++);
> -	if (args->mask & ~(NFS_ACL|NFS_ACLCNT|NFS_DFACL|NFS_DFACLCNT) ||
> +	if (args->mask & ~NFS_ACL_MASK ||
>  	    !xdr_argsize_check(rqstp, p))
>  		return 0;
>  
> diff --git a/include/uapi/linux/nfsacl.h b/include/uapi/linux/nfsacl.h
> index 9bb9771..5527266 100644
> --- a/include/uapi/linux/nfsacl.h
> +++ b/include/uapi/linux/nfsacl.h
> @@ -22,6 +22,7 @@
>  #define NFS_ACLCNT		0x0002
>  #define NFS_DFACL		0x0004
>  #define NFS_DFACLCNT		0x0008
> +#define NFS_ACL_MASK		0x000f
>  
>  /* Flag for Default ACL entries */
>  #define NFS_ACL_DEFAULT		0x1000
> -- 
> 2.4.3

Re: [PATCH 1/5] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[Christoph Hellwig]

On Wed, Jul 08, 2015 at 05:30:15PM -0400, J. Bruce Fields wrote:
> Hm, is the stateid really completely enough set up that this is safe?

It's not.  nfsd4_free_layout_stateid unconditionally deletes
from the per-client and per-file lists which are empty at this
point.  Just adding an explicit fput would be the better fix.

Re: [PATCH 1/5] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[Kinglong Mee]

On 7/9/2015 16:12, Christoph Hellwig wrote:
> On Wed, Jul 08, 2015 at 05:30:15PM -0400, J. Bruce Fields wrote:
>> Hm, is the stateid really completely enough set up that this is safe?
> 
> It's not.  nfsd4_free_layout_stateid unconditionally deletes
> from the per-client and per-file lists which are empty at this
> point.  Just adding an explicit fput would be the better fix.
> 

Got it.

thanks,
Kinglong Mee

[PATCH v2] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[Kinglong Mee]

If nfsd4_layout_setlease fail, nfsd will not put ls->ls_file.

Fix commit c5c707f96f "nfsd: implement pNFS layout recalls".

v2, just put file using fput

Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
---
 fs/nfsd/nfs4layouts.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 6904213..ebf90e4 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -212,6 +212,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
 	BUG_ON(!ls->ls_file);
 
 	if (nfsd4_layout_setlease(ls)) {
+		fput(ls->ls_file);
 		put_nfs4_file(fp);
 		kmem_cache_free(nfs4_layout_stateid_cache, ls);
 		return NULL;
-- 
2.4.3

Re: [PATCH 4/5] nfsd: Check stateid generation in nfsd4_lookup_stateid()

[Kinglong Mee]

On 7/9/2015 05:42, J. Bruce Fields wrote:
> I think you overlooked preprocesse_seqid_op?  Does the reordering of the
> stateid generation checking matter there?  I'm not sure.

Yes, I overlooked it. Sorry for my fault. 
There is an bug exist after reordering the stateid generation checking, 
because the replay request's generation is less than the record always, 
client should receive the old reply but receive nfserr_old_stateid.

So, please ignore this patch. Sorry for the noise.

thanks,
Kinglong Mee

> 
> --b.
> 
> On Fri, Jul 03, 2015 at 07:38:36PM +0800, Kinglong Mee wrote:
>> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
>> ---
>>  fs/nfsd/nfs4layouts.c |  2 --
>>  fs/nfsd/nfs4state.c   | 30 ++++++++++++++----------------
>>  2 files changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
>> index 367a65a..ef63244 100644
>> --- a/fs/nfsd/nfs4layouts.c
>> +++ b/fs/nfsd/nfs4layouts.c
>> @@ -264,8 +264,6 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>>  		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
>>  
>>  		status = nfserr_bad_stateid;
>> -		if (stateid->si_generation > stid->sc_stateid.si_generation)
>> -			goto out_put_stid;
>>  		if (layout_type != ls->ls_layout_type)
>>  			goto out_put_stid;
>>  	}
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 61dfb33..53248cd 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -4555,6 +4555,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>>  		     stateid_t *stateid, unsigned char typemask,
>>  		     struct nfs4_stid **s, struct nfsd_net *nn)
>>  {
>> +	struct nfs4_stid *stid;
>>  	__be32 status;
>>  
>>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid))
>> @@ -4567,10 +4568,18 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>>  	}
>>  	if (status)
>>  		return status;
>> -	*s = find_stateid_by_type(cstate->clp, stateid, typemask);
>> -	if (!*s)
>> +	stid = find_stateid_by_type(cstate->clp, stateid, typemask);
>> +	if (!stid)
>>  		return nfserr_bad_stateid;
>> -	return nfs_ok;
>> +
>> +	status = check_stateid_generation(stateid, &stid->sc_stateid,
>> +			nfsd4_has_session(cstate));
>> +	if (status)
>> +		nfs4_put_stid(stid);
>> +	else
>> +		*s = stid;
>> +
>> +	return status;
>>  }
>>  
>>  static struct file *
>> @@ -4673,10 +4682,6 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>>  				&s, nn);
>>  	if (status)
>>  		return status;
>> -	status = check_stateid_generation(stateid, &s->sc_stateid,
>> -			nfsd4_has_session(cstate));
>> -	if (status)
>> -		goto out;
>>  
>>  	switch (s->sc_type) {
>>  	case NFS4_DELEG_STID:
>> @@ -4694,7 +4699,6 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>>  done:
>>  	if (!status && filpp)
>>  		status = nfs4_check_file(rqstp, fhp, s, filpp, tmp_file, flags);
>> -out:
>>  	if (s)
>>  		nfs4_put_stid(s);
>>  	return status;
>> @@ -5021,7 +5025,6 @@ __be32
>>  nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  		  struct nfsd4_delegreturn *dr)
>>  {
>> -	struct nfs4_delegation *dp;
>>  	stateid_t *stateid = &dr->dr_stateid;
>>  	struct nfs4_stid *s;
>>  	__be32 status;
>> @@ -5033,14 +5036,9 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>  	status = nfsd4_lookup_stateid(cstate, stateid, NFS4_DELEG_STID, &s, nn);
>>  	if (status)
>>  		goto out;
>> -	dp = delegstateid(s);
>> -	status = check_stateid_generation(stateid, &dp->dl_stid.sc_stateid, nfsd4_has_session(cstate));
>> -	if (status)
>> -		goto put_stateid;
>>  
>> -	destroy_delegation(dp);
>> -put_stateid:
>> -	nfs4_put_stid(&dp->dl_stid);
>> +	destroy_delegation(delegstateid(s));
>> +	nfs4_put_stid(s);
>>  out:
>>  	return status;
>>  }
>> -- 
>> 2.4.3
> 

Re: [PATCH v2] nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

[J. Bruce Fields]

On Thu, Jul 09, 2015 at 05:38:26PM +0800, Kinglong Mee wrote:
> If nfsd4_layout_setlease fail, nfsd will not put ls->ls_file.
> 
> Fix commit c5c707f96f "nfsd: implement pNFS layout recalls".
> 
> v2, just put file using fput

OK, thanks.--b.

> 
> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> ---
>  fs/nfsd/nfs4layouts.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 6904213..ebf90e4 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -212,6 +212,7 @@ nfsd4_alloc_layout_stateid(struct nfsd4_compound_state *cstate,
>  	BUG_ON(!ls->ls_file);
>  
>  	if (nfsd4_layout_setlease(ls)) {
> +		fput(ls->ls_file);
>  		put_nfs4_file(fp);
>  		kmem_cache_free(nfs4_layout_stateid_cache, ls);
>  		return NULL;
> -- 
> 2.4.3