| Message ID | 20130201201304.GE30668@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */ -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */