2015-07-03 11:32:19

by Kinglong Mee

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

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(-)

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



2015-07-03 11:35:03

by Kinglong Mee

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

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


2015-07-03 11:36:47

by Kinglong Mee

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

Just using the macro defined in nfs_idmap.h.

Signed-off-by: Kinglong Mee <[email protected]>
---
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


2015-07-03 11:38:46

by Kinglong Mee

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

Signed-off-by: Kinglong Mee <[email protected]>
---
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


2015-07-03 11:39:12

by Kinglong Mee

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

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(-)

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


2015-07-08 21:30:15

by J. Bruce Fields

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

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

2015-07-08 21:42:37

by J. Bruce Fields

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

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

2015-07-08 21:45:35

by J. Bruce Fields

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

On Fri, Jul 03, 2015 at 07:39:02PM +0800, Kinglong Mee wrote:
> Signed-off-by: Kinglong Mee <[email protected]>

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

2015-07-09 08:12:40

by Christoph Hellwig

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

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.

2015-07-09 09:31:29

by Kinglong Mee

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

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

2015-07-09 09:38:34

by Kinglong Mee

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

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(+)

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


2015-07-09 10:52:06

by Kinglong Mee

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

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

2015-07-09 16:19:24

by J. Bruce Fields

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

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