Return-Path: linux-nfs-owner@vger.kernel.org Received: from fieldses.org ([174.143.236.118]:48026 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756826Ab3BVVqf (ORCPT ); Fri, 22 Feb 2013 16:46:35 -0500 Date: Fri, 22 Feb 2013 16:46:34 -0500 From: "J. Bruce Fields" To: Al Viro Cc: linux-nfs@vger.kernel.org Subject: Re: [PATCH] nfsd: handle vfs_getattr errors in acl protocol Message-ID: <20130222214634.GB30856@fieldses.org> References: <20130201131537.GC30668@fieldses.org> <20130201185721.GP4503@ZenIV.linux.org.uk> <20130201201304.GE30668@fieldses.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20130201201304.GE30668@fieldses.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: 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" > > > > > > 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 > 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 > Signed-off-by: J. Bruce Fields > > 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 */