nfsd: Fix a file leak of ls_file if nfsd4_layout_setlease fail

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4layouts.c |  2 --
 fs/nfsd/nfs4state.c   | 30 ++++++++++++++----------------
 2 files changed, 14 insertions(+), 18 deletions(-)

fs/nfsd/nfs4layouts.c +0 -2

index 367a65a..ef63244 100644
@@ -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;
 	}

fs/nfsd/nfs4state.c +14 -16

index 61dfb33..53248cd 100644
@@ -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;
 }

Signature · 1 line

2.4.3

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

--b.

Quoted 99 lines · Kinglong Mee

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs4layouts.c |  2 --
 fs/nfsd/nfs4state.c   | 30 ++++++++++++++----------------
 2 files changed, 14 insertions(+), 18 deletions(-)

fs/nfsd/nfs4layouts.c +0 -2

index 367a65a..ef63244 100644
@@ -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;
 	}

fs/nfsd/nfs4state.c +14 -16

index 61dfb33..53248cd 100644
@@ -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;
 }

Signature · 1 line

2.4.3

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/nfsd/nfs2acl.c           | 10 ++++------
 fs/nfsd/nfs3acl.c           |  4 ++--
 include/uapi/linux/nfsacl.h |  1 +
 3 files changed, 7 insertions(+), 8 deletions(-)

fs/nfsd/nfs2acl.c +4 -6

index d54701f..1580ea6 100644
@@ -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,

fs/nfsd/nfs3acl.c +2 -2

index 882b1a1..01df4cd 100644
@@ -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;
 

include/uapi/linux/nfsacl.h +1 -0

index 9bb9771..5527266 100644
@@ -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

Signature · 1 line

2.4.3

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 <[email protected]>

---
 fs/nfsd/nfs4layouts.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/nfsd/nfs4layouts.c +0 -0

index 6904213..367a65a 100644

fs/nfsd/nfs4layouts.c +0 -0

 		return NULL;
 	}
 

Signature · 1 line

2.4.3

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

Quoted 1 line · J. Bruce Fields

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.

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 <[email protected]>

---
 fs/nfsd/nfs4layouts.c | 1 +
 1 file changed, 1 insertion(+)

fs/nfsd/nfs4layouts.c +1 -0

index 6904213..ebf90e4 100644
@@ -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;

Signature · 1 line

2.4.3