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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxx>
>
> 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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
