On Mon, 14 Jul 2014 09:00:28 -0400 Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
wrote:
> On Mon, 14 Jul 2014 22:35:13 +1000
> NeilBrown <neilb@xxxxxxx> wrote:
>
> > On Mon, 14 Jul 2014 08:14:55 -0400 Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
> > wrote:
> >
> > > On Mon, 14 Jul 2014 15:14:05 +1000
> > > NeilBrown <neilb@xxxxxxx> wrote:
> > >
> > > >
> > > > If an 'open' of a file in an NFSv4 filesystem finds that the dentry is
> > > > in cache, but the inode is stale (on the server), the dentry will not
> > > > be re-validated immediately and may cause ESTALE to be returned to
> > > > user-space.
> > > >
> > > > For a non-create 'open', do_last() calls lookup_fast() and on success
> > > > will eventually call may_open() which calls into nfs_permission().
> > > > If nfs_permission() makes the ACCESS call to the server it will get
> > > > NFS4ERR_STALE, resulting in ESTALE from may_open() and thence from
> > > > do_last().
> > > > The retry-on-ESTALE in filename_lookup() will repeat exactly the same
> > > > process because nothing in this path will invalidate the dentry due to
> > > > the inode being stale, so the ESTALE will be returned.
> > > >
> > > > lookup_fast() calls ->d_revalidate(), but for an OPEN on an NFSv4
> > > > filesystem, that will succeed for regular files:
> > > > /* Let f_op->open() actually open (and revalidate) the file */
> > > >
> > > > Unfortunately in the case of a STALE inode, f_op->open() never gets
> > > > called. If we teach nfs4_lookup_revalidate() to report a failure on
> > > > NFS_STALE() inodes, then the dentry will be invalidated and a full
> > > > lookup will be attempted. The ESTALE errors go away.
> > > >
> > > >
> > > > While I think this fix is correct, I'm not convinced that it is
> > > > sufficient, particularly if lookupcache=none.
> > > > The current code will fail an "open" is nfs_permission() fails,
> > > > without having performed a LOOKUP. i.e. it will use the cache.
> > > > nfs_lookup_revalidate will force a lookup before the permission check
> > > > if NFS_MOUNT_LOOKUP_CACHE_NONE, but nfs4_lookup_revalidate will not.
> > > >
> > >
> > > This patch should make the code fall through to nfs_lookup_revalidate,
> > > which would then force the lookup, right?
> >
> > Yes ... though maybe that's not what I really want to do. I really wanted to
> > just return '0', though I would need to check that is right in all cases.
> >
> > >
> > > Also, I'm a little unclear...
> > >
> > > Why would may_open fail with ESTALE after the v4 OPEN succeeds? The
> > > OPEN should be returning a filehandle and attributes for the inode
> > > actually opened. It seems like we ought to be doing any permission
> > > checks vs. that inode, not anything we had in cache. Presumably the
> > > server is then holding it open so it shouldn't be stale.
> >
> > may_open is called *before* and v4 OPEN.
> >
> > In do_last, if the inode is already in cache, then
> > lookup_fast is called, which calls d_revalidate
> > then may_open (calls ->permission)
> > then finish_open which calls f_op->open
> >
> > Yes, we should be doing permission checking against whatever 'open' finds.
> > But the VFS is structured to the the permission check after d_revalidate and
> > before ->open. So maybe d_revalidate needs to do the NFS open??
> >
>
> Ok, I see. Ugh, having the revalidate do the open sounds...messy.
Having the VFS call into the file system in dribs and drabs, rather than just
asking the filesystem to "open" and letting it call back to VFS libraries
for name lookup etc it what is really messy (IMO).
So yes - definite mess. Not entirely sure where the mess is.
>
> A simpler fix might be to fix it so that an -ESTALE return from
> may_open triggers a retry. Something like this maybe (probably
> whitespace damaged, so just for discussion purposes):
Nice idea but doesn't work.
We get back to retry_lookup and call lookup_open().
lookup_dcache calls d_revalidate which reports that everything is fine, so it
tells lookup_open which jumps to out_no_open and does nothing useful.
So we end up in may_open() again which returns ESTALE again but now we've
used up all our extra lives...
One thing I noticed while exploring this is that do_last calls "may_open"
*before* finish_open() while atomic_open() calls "may_open" *after*
finish_open() (which it calls by virtual of the fact that all ->atomic_open
methods call finish_open()).
I was very tempted to just move the 'may_open' call in 'do_last' to after the
'finish_open' call. That fixed the problem, but I'm not sure it is "right".
I think the real core messiness here is that permission checking should be
neither before nor after finish_open, but should be an integral part of
finish_open with the filesystem doing the permission check in f_op->open().
I'm currently thinking this is the best patch for now:
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 4f7414afca27..5c40cfd3ae29 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1563,9 +1563,10 @@ static int nfs4_lookup_revalidate(struct dentry *dentry, unsigned int flags)
/* We cannot do exclusive creation on a positive dentry */
if (flags & LOOKUP_EXCL)
goto no_open_dput;
- /* Let f_op->open() actually open (and revalidate) the file */
- ret = 1;
+ if (!NFS_STALE(inode))
+ /* Let f_op->open() actually open (and revalidate) the file */
+ ret = 1;
out:
dput(parent);
Thanks,
NeilBrown
>
> diff --git a/fs/namei.c b/fs/namei.c
> index 985c6f368485..c1657deea52c 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3045,8 +3045,13 @@ finish_open:
> }
> finish_open_created:
> error = may_open(&nd->path, acc_mode, open_flag);
> - if (error)
> + if (error) {
> + if (error == -ESTALE)
> + goto stale_open;
> goto out;
> + }
> file->f_path.mnt = nd->path.mnt;
> error = finish_open(file, nd->path.dentry, NULL, opened);
> if (error) {
>
>
> ...though might need to convert the ESTALE to EOPENSTALE there too, not
> sure...
>
> >
> > >
> > > Are we not properly updating the dcache (and attrcache) after the
> > > OPEN reply?
> >
> > I think so, yes. But in the problem case, we don't even send an OPEN
> > request.
> >
> >
> > >
> > > >
> > > > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > > >
> > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > > index 4a3d4ef76127..4f7414afca27 100644
> > > > --- a/fs/nfs/dir.c
> > > > +++ b/fs/nfs/dir.c
> > > > @@ -1563,6 +1563,8 @@ static int nfs4_lookup_revalidate(struct
> > > > dentry *dentry, unsigned int flags) /* We cannot do exclusive
> > > > creation on a positive dentry */ if (flags & LOOKUP_EXCL)
> > > > goto no_open_dput;
> > > > + if (NFS_STALE(inode))
> > > > + goto no_open_dput;
> > > >
> > > > /* Let f_op->open() actually open (and revalidate) the
> > > > file */ ret = 1;
> > >
> > > Looks legit to me too, but it seems like the inode could go stale
> > > w/o us knowing after this point.
> > >
> > > Acked-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
> >
> > Thanks,
> > NeilBrown
>
>
Attachment:
signature.asc
Description: PGP signature
