Re: incorrect EXDEV error occasionally returned for rename over NFS

[Joe Habermann <joe.habermann@gmail.com>]

I did test out the fix and it did avoid the EXDEV problem.  But I see your
point about it opening up a small security hole.

The other thought I had was to use checking similar to what's in
svc_export_match() to see if the two exports were really for the same
namespace/client domain:

--- vfs.c.orig  2011-02-10 18:39:30.000000000 -0600
+++ vfs.c       2011-02-10 18:39:46.000000000 -0600
@@ -1744,8 +1744,12 @@ nfsd_rename(struct svc_rqst *rqstp, stru
        tdir = tdentry->d_inode;

        err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev;
-       if (ffhp->fh_export != tfhp->fh_export)
+       if (ffhp->fh_export != tfhp->fh_export) {
+           if (ffhp->fh_export->ex_client      != tfhp->fh_export->ex_client ||
+                ffhp->fh_export->ex_path.dentry !=
tfhp->fh_export->ex_path.dentry ||
+                ffhp->fh_export->ex_path.mnt    !=
tfhp->fh_export->ex_path.mnt)
                goto out;
+       }

        err = nfserr_perm;
        if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))

I don't think this is perfect because in the race window, the old export
could be removed and a new one could enter the cache with a different
ex_client pointer.

More noodling.

Thanks, -Joe

On Tue, Nov 16, 2010 at 12:55 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Nov 15, 2010 at 03:46:44PM -0600, Joe Habermann wrote:
>> When running SVN checkouts over NFSv3, they occasionally fail with an
>> EXDEV error:
>>
>> (etc.)
>> U    snfs/build/Makefile.hpux
>> svn: In directory 'snfs/build'
>> svn: Can't move 'snfs/build/tempfile.tmp' to
>> 'snfs/build/Makefile.sol': Invalid cross-device link
>>
>> I've noticed that other people have reported this problem but it remains
>> unresolved.  Through some debugging, I believe I have found root cause.
>> nfsd_rename() contains the following code:
>>
>> 1707 /*
>> 1708  * Rename a file
>> 1709  * N.B. After this call _both_ ffhp and tfhp need an fh_put 1710  */
>> 1711 __be32
>> 1712 nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char
>> *fname, int flen,
>> 1713                             struct svc_fh *tfhp, char *tname, int tlen)
>> 1714 {
>> 1715         struct dentry   *fdentry, *tdentry, *odentry, *ndentry, *trap;
>> 1716         struct inode    *fdir, *tdir;
>> 1717         __be32          err;
>> 1718         int             host_err;
>> 1719
>> 1720         err = fh_verify(rqstp, ffhp, S_IFDIR, NFSD_MAY_REMOVE);
>> 1721         if (err)
>> 1722                 goto out;
>> 1723         err = fh_verify(rqstp, tfhp, S_IFDIR, NFSD_MAY_CREATE);
>> 1724         if (err)
>> 1725                 goto out;
>> 1726
>> 1727         fdentry = ffhp->fh_dentry;
>> 1728         fdir = fdentry->d_inode;
>> 1729
>> 1730         tdentry = tfhp->fh_dentry;
>> 1731         tdir = tdentry->d_inode;
>> 1732
>> 1733         err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev;
>> 1734         if (ffhp->fh_export != tfhp->fh_export)
>> 1735                 goto out;
>>
>> Line 1734 compares the fh_export pointers in the file handles for
>> the "from" and "to" directories.  However, this is a bad check since
>> it's possible these pointers differ yet refer to the same underlying
>> file system.  Based on code inspection, encached export pointers are
>> only valid for 30 minutes before they expire and are reinserted through
>> calls made between the kernel and rpc.mountd.  So a race exists where the
>> ffhp->fh_export is populated on line 1720, the export cache is updated,
>> and tfhp->fh_export is set on line 1723 to a different value.
>>
>> This window can be cranked wide open by putting an "msleep(10);"
>> between lines 1722 and 1723 above, recompiling and reinstalling nfsd.ko
>> etc., and then running SVN checkouts in a loop from an NFS client.
>> Within 30 minutes, the problem is seen.
>
> Good detective work, thanks.  Have you confirmed that applying your
> change below eliminates the problem?
>
>> I'm still very new to the NFS code, but I'm wonder whether the check
>> needs to be something like this instead:
>>
>> --- vfs.c.orig  2010-11-15 14:35:18.000000000 -0600
>> +++ vfs.c       2010-11-15 14:43:52.000000000 -0600
>> @@ -1731,7 +1731,7 @@ nfsd_rename(struct svc_rqst *rqstp, stru
>>    tdir = tdentry->d_inode;
>>
>>    err = (rqstp->rq_vers == 2) ? nfserr_acces : nfserr_xdev;
>> -       if (ffhp->fh_export != tfhp->fh_export)
>> +       if (fdir->i_sb != tdir->i_sb)
>>        goto out;
>>
>>    err = nfserr_perm;
>>
>> Thoughts?  I think it's either that or some locking needs to be put in
>> to guarantee that the file handles for the "to" and "from" directory
>> always wind up with the same export pointer when they belong to the
>> same file system.  I guess the other question is why there is an
>> EXDEV check at the top of nfsd_rename() instead of putting it in
>> vfs_rename().
>
> The fh_verify call is also responsible for setting the identity of the
> user, and that may be affected by export options like root_squash.  In
> the above case if ffhp was on a root_squash export, and tfhp on a
> no_root_squash export, and we get a request from uid 0, we'd end up
> with an incoming uid-0 user running as root (since the fh_verify for
> tfhp was performed last), and might allow renaming from a directory that
> only root had write permission to.
>
> It's a minor hole (especially since it's already easy for a client to
> circumvent protections that vary from export to export in the default
> no_subtree_check case).  But we should be able to do better.
>
> I'm not sure what to suggest.
>
> --b.
>
>>
>> For full disclosure, I'm actually running the SLES11 SP1 kernel,
>> linux-2.6.32.12-0.7, but I've looked at the latest upstream kernels and
>> they appear to have the same code and I assume the same issue.
>