[PATCH] nfsd: handle vfs_getattr errors in acl protocol

[PATCH] nfsd: handle vfs_getattr errors in acl protocol

[J. Bruce Fields]

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

We're currently ignoring errors from vfs_getattr.

The correct thing to do is to do the stat in the main service procedure
not in the response encoding.

Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs2acl.c |   31 +++++++++++++++++++++++++++----
 fs/nfsd/nfsxdr.c  |    6 ++----
 fs/nfsd/xdr.h     |    2 +-
 fs/nfsd/xdr3.h    |    2 ++
 4 files changed, 32 insertions(+), 9 deletions(-)

For 3.9.

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index 9170861..b99864b 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -24,6 +24,14 @@ nfsacld_proc_null(struct svc_rqst *rqstp, void *argp, void *resp)
 	return nfs_ok;
 }
 
+static __be32 nfsd_getattr(struct svc_fh *fh, struct kstat *stat)
+{
+	int err;
+
+	err = vfs_getattr(fh->fh_export->ex_path.mnt, fh->fh_dentry, stat);
+	return nfserrno(err);
+}
+
 /*
  * Get the Access and/or Default ACL of a file.
  */
@@ -45,6 +53,10 @@ static __be32 nfsacld_proc_getacl(struct svc_rqst * rqstp,
 		RETURN_STATUS(nfserr_inval);
 	resp->mask = argp->mask;
 
+	nfserr = nfsd_getattr(fh, &resp->stat);
+	if (nfserr)
+		goto fail;
+
 	if (resp->mask & (NFS_ACL|NFS_ACLCNT)) {
 		acl = nfsd_get_posix_acl(fh, ACL_TYPE_ACCESS);
 		if (IS_ERR(acl)) {
@@ -115,6 +127,9 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
 		nfserr = nfserrno( nfsd_set_posix_acl(
 			fh, ACL_TYPE_DEFAULT, argp->acl_default) );
 	}
+	if (!nfserr) {
+		nfserr = nfsd_getattr(fh, &resp->stat);
+	}
 
 	/* argp->acl_{access,default} may have been allocated in
 	   nfssvc_decode_setaclargs. */
@@ -129,10 +144,15 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
 static __be32 nfsacld_proc_getattr(struct svc_rqst * rqstp,
 		struct nfsd_fhandle *argp, struct nfsd_attrstat *resp)
 {
+	__be32 nfserr;
 	dprintk("nfsd: GETATTR  %s\n", SVCFH_fmt(&argp->fh));
 
 	fh_copy(&resp->fh, &argp->fh);
-	return fh_verify(rqstp, &resp->fh, 0, NFSD_MAY_NOP);
+	nfserr = fh_verify(rqstp, &resp->fh, 0, NFSD_MAY_NOP);
+	if (nfserr)
+		return nfserr;
+	nfserr = nfsd_getattr(&resp->fh, &resp->stat);
+	return nfserr;
 }
 
 /*
@@ -150,6 +170,9 @@ static __be32 nfsacld_proc_access(struct svc_rqst *rqstp, struct nfsd3_accessarg
 	fh_copy(&resp->fh, &argp->fh);
 	resp->access = argp->access;
 	nfserr = nfsd_access(rqstp, &resp->fh, &resp->access, NULL);
+	if (nfserr)
+		return nfserr;
+	nfserr = nfsd_getattr(&resp->fh, &resp->stat);
 	return nfserr;
 }
 
@@ -243,7 +266,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
 		return 0;
 	inode = dentry->d_inode;
 
-	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
+	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
 	*p++ = htonl(resp->mask);
 	if (!xdr_ressize_check(rqstp, p))
 		return 0;
@@ -274,7 +297,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
 static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
 		struct nfsd_attrstat *resp)
 {
-	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
+	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
 	return xdr_ressize_check(rqstp, p);
 }
 
@@ -282,7 +305,7 @@ static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
 static int nfsaclsvc_encode_accessres(struct svc_rqst *rqstp, __be32 *p,
 		struct nfsd3_accessres *resp)
 {
-	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
+	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
 	*p++ = htonl(resp->access);
 	return xdr_ressize_check(rqstp, p);
 }
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 979b421..90f47fe 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -194,11 +194,9 @@ encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp,
 }
 
 /* Helper function for NFSv2 ACL code */
-__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
+__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp, struct kstat *stat)
 {
-	struct kstat stat;
-	vfs_getattr(fhp->fh_export->ex_path.mnt, fhp->fh_dentry, &stat);
-	return encode_fattr(rqstp, p, fhp, &stat);
+	return encode_fattr(rqstp, p, fhp, stat);
 }
 
 /*
diff --git a/fs/nfsd/xdr.h b/fs/nfsd/xdr.h
index 53b1863..4f0481d 100644
--- a/fs/nfsd/xdr.h
+++ b/fs/nfsd/xdr.h
@@ -167,7 +167,7 @@ int nfssvc_encode_entry(void *, const char *name,
 int nfssvc_release_fhandle(struct svc_rqst *, __be32 *, struct nfsd_fhandle *);
 
 /* Helper functions for NFSv2 ACL code */
-__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp);
+__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp, struct kstat *stat);
 __be32 *nfs2svc_decode_fh(__be32 *p, struct svc_fh *fhp);
 
 #endif /* LINUX_NFSD_H */
diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
index 7df980e..b6d5542 100644
--- a/fs/nfsd/xdr3.h
+++ b/fs/nfsd/xdr3.h
@@ -136,6 +136,7 @@ struct nfsd3_accessres {
 	__be32			status;
 	struct svc_fh		fh;
 	__u32			access;
+	struct kstat		stat;
 };
 
 struct nfsd3_readlinkres {
@@ -225,6 +226,7 @@ struct nfsd3_getaclres {
 	int			mask;
 	struct posix_acl	*acl_access;
 	struct posix_acl	*acl_default;
+	struct kstat		stat;
 };
 
 /* dummy type for release */
-- 
1.7.9.5

Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol

[Al Viro]

On Fri, Feb 01, 2013 at 08:15:37AM -0500, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> We're currently ignoring errors from vfs_getattr.
> 
> The correct thing to do is to do the stat in the main service procedure
> not in the response encoding.

FWIW, I'd combine that with parts of commit I've got in my tree; I think
nfsd_getattr() in your variant isn't the best API here.  All callers
that want nfserrno want vfsmount/dentry coming from some fhandle.  Diff
below is introducing such helper and switching to its use (warning: edited
patch; only obvious editing done there, but still).  It does *not* address
the issue your patch deals with (see /* BUG */ in there), but I really
think it's a better interface than your variant.  FWIW, the rest of the
commit in question is switching vfs_getattr() to struct path *; I'd
edited those parts out of the diff below.  Comments?

new helper: fh_getattr

A bunch of places in nfsd want vfs_getattr() done on object an fhandle
refers to, with nfs-encoded error (__be32).  fh_getattr(fh, stat) does
just that; open-coded instances switched to it.
 
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index 1fc02df..4012899 100644
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -43,7 +43,6 @@ static __be32
 nfsd3_proc_getattr(struct svc_rqst *rqstp, struct nfsd_fhandle  *argp,
 					   struct nfsd3_attrstat *resp)
 {
-	int	err;
 	__be32	nfserr;
 
 	dprintk("nfsd: GETATTR(3)  %s\n",
@@ -55,9 +54,7 @@ nfsd3_proc_getattr(struct svc_rqst *rqstp, struct nfsd_fhandle  *argp,
 	if (nfserr)
 		RETURN_STATUS(nfserr);
 
-	err = vfs_getattr(resp->fh.fh_export->ex_path.mnt,
-			  resp->fh.fh_dentry, &resp->stat);
-	nfserr = nfserrno(err);
+	nfserr = fh_getattr(&resp->fh, &resp->stat);
 
 	RETURN_STATUS(nfserr);
 }
diff --git a/fs/nfsd/nfs3xdr.c b/fs/nfsd/nfs3xdr.c
index 324c0ba..7af9417b 100644
--- a/fs/nfsd/nfs3xdr.c
+++ b/fs/nfsd/nfs3xdr.c
@@ -11,6 +11,7 @@
 #include "xdr3.h"
 #include "auth.h"
 #include "netns.h"
+#include "vfs.h"
 
 #define NFSDDBG_FACILITY		NFSDDBG_XDR
 
@@ -204,10 +205,10 @@ encode_post_op_attr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
 {
 	struct dentry *dentry = fhp->fh_dentry;
 	if (dentry && dentry->d_inode) {
-	        int err;
+	        __be32 err;
 		struct kstat stat;
 
-		err = vfs_getattr(fhp->fh_export->ex_path.mnt, dentry, &stat);
+		err = fh_getattr(fhp, &stat);
 		if (!err) {
 			*p++ = xdr_one;		/* attributes follow */
 			lease_get_mtime(dentry->d_inode, &stat.mtime);
@@ -254,13 +255,12 @@ encode_wcc_data(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
  */
 void fill_post_wcc(struct svc_fh *fhp)
 {
-	int err;
+	__be32 err;
 
 	if (fhp->fh_post_saved)
 		printk("nfsd: inode locked twice during operation.\n");
 
-	err = vfs_getattr(fhp->fh_export->ex_path.mnt, fhp->fh_dentry,
-			&fhp->fh_post_attr);
+	err = fh_getattr(fhp, &fhp->fh_post_attr);
 	fhp->fh_post_change = fhp->fh_dentry->d_inode->i_version;
 	if (err) {
 		fhp->fh_post_saved = 0;
diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index aad6d45..54c6b3d 100644
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -26,17 +26,13 @@ static __be32
 nfsd_return_attrs(__be32 err, struct nfsd_attrstat *resp)
 {
 	if (err) return err;
-	return nfserrno(vfs_getattr(resp->fh.fh_export->ex_path.mnt,
-				    resp->fh.fh_dentry,
-				    &resp->stat));
+	return fh_getattr(&resp->fh, &resp->stat);
 }
 static __be32
 nfsd_return_dirop(__be32 err, struct nfsd_diropres *resp)
 {
 	if (err) return err;
-	return nfserrno(vfs_getattr(resp->fh.fh_export->ex_path.mnt,
-				    resp->fh.fh_dentry,
-				    &resp->stat));
+	return fh_getattr(&resp->fh, &resp->stat);
 }
 /*
  * Get a file's attributes
@@ -150,9 +146,7 @@ nfsd_proc_read(struct svc_rqst *rqstp, struct nfsd_readargs *argp,
 				  &resp->count);
 
 	if (nfserr) return nfserr;
-	return nfserrno(vfs_getattr(resp->fh.fh_export->ex_path.mnt,
-				    resp->fh.fh_dentry,
-				    &resp->stat));
+	return fh_getattr(&resp->fh, &resp->stat);
 }
 
 /*
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index 979b421..bf6d3bc 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -4,6 +4,7 @@
  * Copyright (C) 1995, 1996 Olaf Kirch <okir@monad.swb.de>
  */
 
+#include "vfs.h"
 #include "xdr.h"
 #include "auth.h"
 
@@ -197,7 +198,7 @@ encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp,
 __be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
 {
 	struct kstat stat;
-	vfs_getattr(fhp->fh_export->ex_path.mnt, fhp->fh_dentry, &stat);
+	fh_getattr(fhp, &stat);	/* BUG */
 	return encode_fattr(rqstp, p, fhp, &stat);
 }
 
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index 359594c..5b58941 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -6,6 +6,7 @@
 #define LINUX_NFSD_VFS_H
 
 #include "nfsfh.h"
+#include "nfsd.h"
 
 /*
  * Flags for nfsd_permission
@@ -125,4 +126,11 @@ static inline void fh_drop_write(struct svc_fh *fh)
 	}
 }
 
+static inline __be32 fh_getattr(struct svc_fh *fh, struct kstat *stat)
+{
+	return nfserrno(vfs_getattr(fh->fh_export->ex_path.mnt,
+				    fh->fh_dentry,
+				    stat));
+}
+
 #endif /* LINUX_NFSD_VFS_H */

Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol

[J. Bruce Fields]

On Fri, Feb 01, 2013 at 06:57:22PM +0000, Al Viro wrote:
> On Fri, Feb 01, 2013 at 08:15:37AM -0500, J. Bruce Fields wrote:
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > We're currently ignoring errors from vfs_getattr.
> > 
> > The correct thing to do is to do the stat in the main service procedure
> > not in the response encoding.
> 
> FWIW, I'd combine that with parts of commit I've got in my tree; I think
> nfsd_getattr() in your variant isn't the best API here.  All callers
> that want nfserrno want vfsmount/dentry coming from some fhandle.  Diff
> below is introducing such helper and switching to its use (warning: edited
> patch; only obvious editing done there, but still).  It does *not* address
> the issue your patch deals with (see /* BUG */ in there), but I really
> think it's a better interface than your variant.

Actually, we have precisely the same interface except for the name:

	__be32 fh_getattr(struct svc_fh *fh, struct kstat *stat)
vs.
	__be32 nfsd_getattr(struct svc_fh *fh, struct kstat *stat)

but I'm fine with your name.

Do you want to take these patches, or should I?

Assuming the latter: this is a version of my bugfix rebased on top of
what you just sent me.  I did a quick test and verified it doesn't crash
when I asked for an acl....

--b.

commit 7afea2b2cd951c3a566356206d84a0f5b8aa0e1a
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Wed Jan 30 16:10:19 2013 -0500

    nfsd: handle vfs_getattr errors in acl protocol
    
    We're currently ignoring errors from vfs_getattr.
    
    The correct thing to do is to do the stat in the main service procedure
    not in the response encoding.
    
    Reported-by: Al Viro <viro@zeniv.linux.org.uk>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index 9170861..95d76dc 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -45,6 +45,10 @@ static __be32 nfsacld_proc_getacl(struct svc_rqst * rqstp,
 		RETURN_STATUS(nfserr_inval);
 	resp->mask = argp->mask;
 
+	nfserr = fh_getattr(fh, &resp->stat);
+	if (nfserr)
+		goto fail;
+
 	if (resp->mask & (NFS_ACL|NFS_ACLCNT)) {
 		acl = nfsd_get_posix_acl(fh, ACL_TYPE_ACCESS);
 		if (IS_ERR(acl)) {
@@ -115,6 +119,9 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
 		nfserr = nfserrno( nfsd_set_posix_acl(
 			fh, ACL_TYPE_DEFAULT, argp->acl_default) );
 	}
+	if (!nfserr) {
+		nfserr = fh_getattr(fh, &resp->stat);
+	}
 
 	/* argp->acl_{access,default} may have been allocated in
 	   nfssvc_decode_setaclargs. */
@@ -129,10 +136,15 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
 static __be32 nfsacld_proc_getattr(struct svc_rqst * rqstp,
 		struct nfsd_fhandle *argp, struct nfsd_attrstat *resp)
 {
+	__be32 nfserr;
 	dprintk("nfsd: GETATTR  %s\n", SVCFH_fmt(&argp->fh));
 
 	fh_copy(&resp->fh, &argp->fh);
-	return fh_verify(rqstp, &resp->fh, 0, NFSD_MAY_NOP);
+	nfserr = fh_verify(rqstp, &resp->fh, 0, NFSD_MAY_NOP);
+	if (nfserr)
+		return nfserr;
+	nfserr = fh_getattr(&resp->fh, &resp->stat);
+	return nfserr;
 }
 
 /*
@@ -150,6 +162,9 @@ static __be32 nfsacld_proc_access(struct svc_rqst *rqstp, struct nfsd3_accessarg
 	fh_copy(&resp->fh, &argp->fh);
 	resp->access = argp->access;
 	nfserr = nfsd_access(rqstp, &resp->fh, &resp->access, NULL);
+	if (nfserr)
+		return nfserr;
+	nfserr = fh_getattr(&resp->fh, &resp->stat);
 	return nfserr;
 }
 
@@ -243,7 +258,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
 		return 0;
 	inode = dentry->d_inode;
 
-	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
+	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
 	*p++ = htonl(resp->mask);
 	if (!xdr_ressize_check(rqstp, p))
 		return 0;
@@ -274,7 +289,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
 static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
 		struct nfsd_attrstat *resp)
 {
-	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
+	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
 	return xdr_ressize_check(rqstp, p);
 }
 
@@ -282,7 +297,7 @@ static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
 static int nfsaclsvc_encode_accessres(struct svc_rqst *rqstp, __be32 *p,
 		struct nfsd3_accessres *resp)
 {
-	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
+	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
 	*p++ = htonl(resp->access);
 	return xdr_ressize_check(rqstp, p);
 }
diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
index bf6d3bc..96e5619 100644
--- a/fs/nfsd/nfsxdr.c
+++ b/fs/nfsd/nfsxdr.c
@@ -195,11 +195,9 @@ encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp,
 }
 
 /* Helper function for NFSv2 ACL code */
-__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
+__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp, struct kstat *stat)
 {
-	struct kstat stat;
-	fh_getattr(fhp, &stat);	/* BUG */
-	return encode_fattr(rqstp, p, fhp, &stat);
+	return encode_fattr(rqstp, p, fhp, stat);
 }
 
 /*
diff --git a/fs/nfsd/xdr.h b/fs/nfsd/xdr.h
index 53b1863..4f0481d 100644
--- a/fs/nfsd/xdr.h
+++ b/fs/nfsd/xdr.h
@@ -167,7 +167,7 @@ int nfssvc_encode_entry(void *, const char *name,
 int nfssvc_release_fhandle(struct svc_rqst *, __be32 *, struct nfsd_fhandle *);
 
 /* Helper functions for NFSv2 ACL code */
-__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp);
+__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp, struct kstat *stat);
 __be32 *nfs2svc_decode_fh(__be32 *p, struct svc_fh *fhp);
 
 #endif /* LINUX_NFSD_H */
diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
index 7df980e..b6d5542 100644
--- a/fs/nfsd/xdr3.h
+++ b/fs/nfsd/xdr3.h
@@ -136,6 +136,7 @@ struct nfsd3_accessres {
 	__be32			status;
 	struct svc_fh		fh;
 	__u32			access;
+	struct kstat		stat;
 };
 
 struct nfsd3_readlinkres {
@@ -225,6 +226,7 @@ struct nfsd3_getaclres {
 	int			mask;
 	struct posix_acl	*acl_access;
 	struct posix_acl	*acl_default;
+	struct kstat		stat;
 };
 
 /* dummy type for release */

Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol

[J. Bruce Fields]

On Fri, Feb 01, 2013 at 03:13:04PM -0500, J. Bruce Fields wrote:
> On Fri, Feb 01, 2013 at 06:57:22PM +0000, Al Viro wrote:
> > On Fri, Feb 01, 2013 at 08:15:37AM -0500, J. Bruce Fields wrote:
> > > From: "J. Bruce Fields" <bfields@redhat.com>
> > > 
> > > We're currently ignoring errors from vfs_getattr.
> > > 
> > > The correct thing to do is to do the stat in the main service procedure
> > > not in the response encoding.
> > 
> > FWIW, I'd combine that with parts of commit I've got in my tree; I think
> > nfsd_getattr() in your variant isn't the best API here.  All callers
> > that want nfserrno want vfsmount/dentry coming from some fhandle.  Diff
> > below is introducing such helper and switching to its use (warning: edited
> > patch; only obvious editing done there, but still).  It does *not* address
> > the issue your patch deals with (see /* BUG */ in there), but I really
> > think it's a better interface than your variant.
> 
> Actually, we have precisely the same interface except for the name:
> 
> 	__be32 fh_getattr(struct svc_fh *fh, struct kstat *stat)
> vs.
> 	__be32 nfsd_getattr(struct svc_fh *fh, struct kstat *stat)
> 
> but I'm fine with your name.
> 
> Do you want to take these patches, or should I?

I guess what I'll do unless I hear otherwise is apply both your patch
and mine to my tree for 3.9.

--b.

> 
> Assuming the latter: this is a version of my bugfix rebased on top of
> what you just sent me.  I did a quick test and verified it doesn't crash
> when I asked for an acl....
> 
> --b.
> 
> commit 7afea2b2cd951c3a566356206d84a0f5b8aa0e1a
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Wed Jan 30 16:10:19 2013 -0500
> 
>     nfsd: handle vfs_getattr errors in acl protocol
>     
>     We're currently ignoring errors from vfs_getattr.
>     
>     The correct thing to do is to do the stat in the main service procedure
>     not in the response encoding.
>     
>     Reported-by: Al Viro <viro@zeniv.linux.org.uk>
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
> index 9170861..95d76dc 100644
> --- a/fs/nfsd/nfs2acl.c
> +++ b/fs/nfsd/nfs2acl.c
> @@ -45,6 +45,10 @@ static __be32 nfsacld_proc_getacl(struct svc_rqst * rqstp,
>  		RETURN_STATUS(nfserr_inval);
>  	resp->mask = argp->mask;
>  
> +	nfserr = fh_getattr(fh, &resp->stat);
> +	if (nfserr)
> +		goto fail;
> +
>  	if (resp->mask & (NFS_ACL|NFS_ACLCNT)) {
>  		acl = nfsd_get_posix_acl(fh, ACL_TYPE_ACCESS);
>  		if (IS_ERR(acl)) {
> @@ -115,6 +119,9 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
>  		nfserr = nfserrno( nfsd_set_posix_acl(
>  			fh, ACL_TYPE_DEFAULT, argp->acl_default) );
>  	}
> +	if (!nfserr) {
> +		nfserr = fh_getattr(fh, &resp->stat);
> +	}
>  
>  	/* argp->acl_{access,default} may have been allocated in
>  	   nfssvc_decode_setaclargs. */
> @@ -129,10 +136,15 @@ static __be32 nfsacld_proc_setacl(struct svc_rqst * rqstp,
>  static __be32 nfsacld_proc_getattr(struct svc_rqst * rqstp,
>  		struct nfsd_fhandle *argp, struct nfsd_attrstat *resp)
>  {
> +	__be32 nfserr;
>  	dprintk("nfsd: GETATTR  %s\n", SVCFH_fmt(&argp->fh));
>  
>  	fh_copy(&resp->fh, &argp->fh);
> -	return fh_verify(rqstp, &resp->fh, 0, NFSD_MAY_NOP);
> +	nfserr = fh_verify(rqstp, &resp->fh, 0, NFSD_MAY_NOP);
> +	if (nfserr)
> +		return nfserr;
> +	nfserr = fh_getattr(&resp->fh, &resp->stat);
> +	return nfserr;
>  }
>  
>  /*
> @@ -150,6 +162,9 @@ static __be32 nfsacld_proc_access(struct svc_rqst *rqstp, struct nfsd3_accessarg
>  	fh_copy(&resp->fh, &argp->fh);
>  	resp->access = argp->access;
>  	nfserr = nfsd_access(rqstp, &resp->fh, &resp->access, NULL);
> +	if (nfserr)
> +		return nfserr;
> +	nfserr = fh_getattr(&resp->fh, &resp->stat);
>  	return nfserr;
>  }
>  
> @@ -243,7 +258,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
>  		return 0;
>  	inode = dentry->d_inode;
>  
> -	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
> +	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
>  	*p++ = htonl(resp->mask);
>  	if (!xdr_ressize_check(rqstp, p))
>  		return 0;
> @@ -274,7 +289,7 @@ static int nfsaclsvc_encode_getaclres(struct svc_rqst *rqstp, __be32 *p,
>  static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
>  		struct nfsd_attrstat *resp)
>  {
> -	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
> +	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
>  	return xdr_ressize_check(rqstp, p);
>  }
>  
> @@ -282,7 +297,7 @@ static int nfsaclsvc_encode_attrstatres(struct svc_rqst *rqstp, __be32 *p,
>  static int nfsaclsvc_encode_accessres(struct svc_rqst *rqstp, __be32 *p,
>  		struct nfsd3_accessres *resp)
>  {
> -	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh);
> +	p = nfs2svc_encode_fattr(rqstp, p, &resp->fh, &resp->stat);
>  	*p++ = htonl(resp->access);
>  	return xdr_ressize_check(rqstp, p);
>  }
> diff --git a/fs/nfsd/nfsxdr.c b/fs/nfsd/nfsxdr.c
> index bf6d3bc..96e5619 100644
> --- a/fs/nfsd/nfsxdr.c
> +++ b/fs/nfsd/nfsxdr.c
> @@ -195,11 +195,9 @@ encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp,
>  }
>  
>  /* Helper function for NFSv2 ACL code */
> -__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp)
> +__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp, struct kstat *stat)
>  {
> -	struct kstat stat;
> -	fh_getattr(fhp, &stat);	/* BUG */
> -	return encode_fattr(rqstp, p, fhp, &stat);
> +	return encode_fattr(rqstp, p, fhp, stat);
>  }
>  
>  /*
> diff --git a/fs/nfsd/xdr.h b/fs/nfsd/xdr.h
> index 53b1863..4f0481d 100644
> --- a/fs/nfsd/xdr.h
> +++ b/fs/nfsd/xdr.h
> @@ -167,7 +167,7 @@ int nfssvc_encode_entry(void *, const char *name,
>  int nfssvc_release_fhandle(struct svc_rqst *, __be32 *, struct nfsd_fhandle *);
>  
>  /* Helper functions for NFSv2 ACL code */
> -__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp);
> +__be32 *nfs2svc_encode_fattr(struct svc_rqst *rqstp, __be32 *p, struct svc_fh *fhp, struct kstat *stat);
>  __be32 *nfs2svc_decode_fh(__be32 *p, struct svc_fh *fhp);
>  
>  #endif /* LINUX_NFSD_H */
> diff --git a/fs/nfsd/xdr3.h b/fs/nfsd/xdr3.h
> index 7df980e..b6d5542 100644
> --- a/fs/nfsd/xdr3.h
> +++ b/fs/nfsd/xdr3.h
> @@ -136,6 +136,7 @@ struct nfsd3_accessres {
>  	__be32			status;
>  	struct svc_fh		fh;
>  	__u32			access;
> +	struct kstat		stat;
>  };
>  
>  struct nfsd3_readlinkres {
> @@ -225,6 +226,7 @@ struct nfsd3_getaclres {
>  	int			mask;
>  	struct posix_acl	*acl_access;
>  	struct posix_acl	*acl_default;
> +	struct kstat		stat;
>  };
>  
>  /* dummy type for release */

Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol

[Al Viro]

On Fri, Feb 22, 2013 at 04:46:34PM -0500, J. Bruce Fields wrote:

> > Actually, we have precisely the same interface except for the name:
> > 
> > 	__be32 fh_getattr(struct svc_fh *fh, struct kstat *stat)
> > vs.
> > 	__be32 nfsd_getattr(struct svc_fh *fh, struct kstat *stat)
> > 
> > but I'm fine with your name.
> > 
> > Do you want to take these patches, or should I?

*blink*

I blame being very low on caffeine; that, or being a blind idiot...
My apologies ;-/

> I guess what I'll do unless I hear otherwise is apply both your patch
> and mine to my tree for 3.9.

FWIW, I'm going to push the first part of VFS queue later tonight...

Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol

[J. Bruce Fields]

On Fri, Feb 22, 2013 at 10:15:06PM +0000, Al Viro wrote:
> On Fri, Feb 22, 2013 at 04:46:34PM -0500, J. Bruce Fields wrote:
> 
> > > Actually, we have precisely the same interface except for the name:
> > > 
> > > 	__be32 fh_getattr(struct svc_fh *fh, struct kstat *stat)
> > > vs.
> > > 	__be32 nfsd_getattr(struct svc_fh *fh, struct kstat *stat)
> > > 
> > > but I'm fine with your name.
> > > 
> > > Do you want to take these patches, or should I?
> 
> *blink*
> 
> I blame being very low on caffeine; that, or being a blind idiot...
> My apologies ;-/
>
> > I guess what I'll do unless I hear otherwise is apply both your patch
> > and mine to my tree for 3.9.
> 
> FWIW, I'm going to push the first part of VFS queue later tonight...

OK, I'll wait and see what's in that.

(And what should I do with the delegation patches?)

--b.

Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol

[Al Viro]

On Fri, Feb 22, 2013 at 05:18:35PM -0500, J. Bruce Fields wrote:

> > FWIW, I'm going to push the first part of VFS queue later tonight...
> 
> OK, I'll wait and see what's in that.

See for-next...

> (And what should I do with the delegation patches?)

Looking through that stuff...