On Fri, 2008-02-08 at 16:03 -0500, J. Bruce Fields wrote:
> On Fri, Feb 08, 2008 at 03:07:57PM -0500, Peter Staubach wrote:
> > J. Bruce Fields wrote:
> >> From: Frank Filz <[EMAIL PROTECTED]>
> >>
> >> This patch addresses a compatibility issue with a Linux NFS server and
> >> AIX NFS client.
> >>
> >> I have exported /export as fsid=0 with sec=krb5:krb5i
> >> I have mount --bind /home onto /export/home
> >> I have exported /export/home with sec=krb5i
> >>
> >> The AIX client mounts / -o sec=krb5:krb5i onto /mnt
> >>
> >> If I do an ls /mnt, the AIX client gets a permission error. Looking at
> >> the network traceIwe see a READDIR looking for attributes
> >> FATTR4_RDATTR_ERROR and FATTR4_MOUNTED_ON_FILEID. The response gives a
> >> NFS4ERR_WRONGSEC which the AIX client is not expecting.
> >>
> >> Since the AIX client is only asking for an attribute that is an
> >> attribute of the parent file system (pseudo root in my example), it
> >> seems reasonable that there should not be an error.
> >>
> >>
> >
> > Can we go through this explanation one more time, a little more
> > slowly, please? I am not following it.
> >
> > It is my understanding that FATTR4_RDATTR_ERROR simply says
> > to return a magic value to indicate that the requested attributes
> > could not be retrieved during a READDIR operation. It is also
> > my understanding that the FATTR4_MOUNTED_ON_FILEID returns either
> > the fileid of the entry in the directory or the fileid of the
> > directory which is the root of the mount point which is mounted
> > on top of the entry in the directory.
>
> No, it's the fileid of the directory underneath (the mounted-on
> directory), not of the directory that's mounted on top of it--that would
> be just the regular fileid. Does that clear up the confusion?
READDIR with cross-mounts is definitely confusing. Remember READDIR is
reading some directory .../a/ that is part of file system #1. Within
that directory is a mountpoint, .../a/fs2 for a new file system (#2). A
cross mount. The client is currently using a security flavor that is
valid for file system #1 but not valid for file system #2.
Since .../a/fs2 is the root of a new file system, it probably has a
FILEID like 2 or something. However, there is .../a/fs2 which is a file
in file system #1 (perhaps with a FILEID like 356).
FATTR4_MOUNTED_ON_FILEID will return 356 instead of 2. Since the client
is only asking for the file names in .../a/ and their
FATTR4_MOUNTED_ON_FILEID, the READDIR is clearly asking for information
that does not at all depend on access to (and permission to use) file
system #2 (in fact, the exports might even deny this client access to
file system #2 no matter what security flavor it uses). Since the client
is not asking for anything that it doesn't have permission to ask for
(if file system #2 were not currently mounted, this READDIR would
respond with exactly the same results, and the security of file system
#2 would not come into play at all), there is no reason to report an
error (and not supply the attribute).
> > So, given all of this, why is the right thing to do to return
> > the fileid of the entry in the directory, even though it is
> > mounted on top of? Why isn't the right thing to do to return
> > NFS4ERR_WRONGSEC per page 206 in rfc3530?
> >
> > Perhaps I am not following the bind mount properly?
> >
> > A little more below --
> >
> >
> >> In discussing this issue with Bruce Fields, I initially proposed
> >> ignoring the error in nfsd4_encode_dirent_fattr() if all that was being
> >> asked for was FATTR4_RDATTR_ERROR and FATTR4_MOUNTED_ON_FILEID, however,
> >> Bruce suggested that we avoid calling cross_mnt() if only these
> >> attributes are requested.
> >>
> >> The following patch implements bypassing cross_mnt() if only
> >> FATTR4_RDATTR_ERROR and FATTR4_MOUNTED_ON_FILEID are called. Since there
> >> is some complexity in the code in nfsd4_encode_fattr(), I didn't want to
> >> duplicate code (and introduce a maintenance nightmare), so I added a
> >> parameter to nfsd4_encode_fattr() that indicates whether it should
> >> ignore cross mounts and simply fill in the attribute using the passed in
> >> dentry as opposed to it's parent.
> >>
> >> Signed-off-by: Frank Filz <[EMAIL PROTECTED]>
> >> Signed-off-by: J. Bruce Fields <[EMAIL PROTECTED]>
> >> ---
> >> fs/nfsd/nfs4proc.c | 2 +-
> >> fs/nfsd/nfs4xdr.c | 27 ++++++++++++++++++++++-----
> >> include/linux/nfsd/xdr4.h | 2 +-
> >> 3 files changed, 24 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >> index 18ead17..c593db0 100644
> >> --- a/fs/nfsd/nfs4proc.c
> >> +++ b/fs/nfsd/nfs4proc.c
> >> @@ -750,7 +750,7 @@ _nfsd4_verify(struct svc_rqst *rqstp, struct
> >> nfsd4_compound_state *cstate,
> >> cstate->current_fh.fh_export,
> >> cstate->current_fh.fh_dentry, buf,
> >> &count, verify->ve_bmval,
> >> - rqstp);
> >> + rqstp, 0);
> >> /* this means that nfsd4_encode_fattr() ran out of space */
> >> if (status == nfserr_resource && count == 0)
> >> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> >> index 25c7ae2..2d94b9b 100644
> >> --- a/fs/nfsd/nfs4xdr.c
> >> +++ b/fs/nfsd/nfs4xdr.c
> >> @@ -1453,7 +1453,7 @@ static __be32 fattr_handle_absent_fs(u32 *bmval0,
> >> u32 *bmval1, u32 *rdattr_err)
> >> __be32
> >> nfsd4_encode_fattr(struct svc_fh *fhp, struct svc_export *exp,
> >> struct dentry *dentry, __be32 *buffer, int *countp, u32 *bmval,
> >> - struct svc_rqst *rqstp)
> >> + struct svc_rqst *rqstp, int ignore_crossmnt)
> >> {
> >> u32 bmval0 = bmval[0];
> >> u32 bmval1 = bmval[1];
> >> @@ -1833,7 +1833,12 @@ out_acl:
> >> if (bmval1 & FATTR4_WORD1_MOUNTED_ON_FILEID) {
> >> if ((buflen -= 8) < 0)
> >> goto out_resource;
> >> - if (exp->ex_mnt->mnt_root->d_inode == dentry->d_inode) {
> >> + /*
> >> + * Get parent's attributes if not ignoring crossmount
> >> + * and this is the root of a cross-mounted filesystem.
> >> + */
> >> + if (ignore_crossmnt == 0 &&
> >> + exp->ex_mnt->mnt_root->d_inode == dentry->d_inode) {
> >> err = vfs_getattr(exp->ex_mnt->mnt_parent,
> >> exp->ex_mnt->mnt_mountpoint, &stat);
> >> if (err)
> >> @@ -1869,13 +1874,25 @@ nfsd4_encode_dirent_fattr(struct nfsd4_readdir *cd,
> >> struct svc_export *exp = cd->rd_fhp->fh_export;
> >> struct dentry *dentry;
> >> __be32 nfserr;
> >> + int ignore_crossmnt = 0;
> >> dentry = lookup_one_len(name, cd->rd_fhp->fh_dentry, namlen);
> >> if (IS_ERR(dentry))
> >> return nfserrno(PTR_ERR(dentry));
> >> exp_get(exp);
> >> - if (d_mountpoint(dentry)) {
> >> + /*
> >> + * In the case of a mountpoint, the client may be asking for
> >> + * attributes that are only properties of the underlying filesystem
> >> + * as opposed to the cross-mounted file system. In such a case,
> >> + * we will not follow the cross mount and will fill the attribtutes
> >> + * directly from the mountpoint dentry.
> >> + */
> >> + if (d_mountpoint(dentry) &&
> >> + (cd->rd_bmval[0] & ~FATTR4_WORD0_RDATTR_ERROR) == 0 &&
> >> + (cd->rd_bmval[1] & ~FATTR4_WORD1_MOUNTED_ON_FILEID) == 0)
> >>
> >
> > These are some odd looking tests. What is the real intention
> > for these tests? They don't test just for requests with just
> > RDATTR_ERROR and MOUNTED_ON_FILEID set. They will trigger
> > whether or not either or neither is set as well.
>
> Right. The test is meant to fail iff someone requests an attribute
> other than those two. (Note the different rd_bmval[] array indices.)
>
> (Actually, I suppose we could also allow requests for lease_time. Patch
> welcomed....)
Yep, intention here is to bypass the crossmount if the only attributes
requested are properties of file system #1 (per above discussion).
Frank Filz
-
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
