On Wed, 2013-07-31 at 15:26 -0500, Quentin Barnes wrote:
> On Wed, Jul 31, 2013 at 1:46 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Wed, 31 Jul 2013 13:36:21 -0500
> > Quentin Barnes <qbarnes@xxxxxxxxx> wrote:
> >
> >> I was reviewing patches I was integrating into my NFS source base
> >> and this one popped up (commit f6488c9b). In reviewing it, it would
> >> fix the problem, however, I think it's not the best fix for it.
> >> Catching it in nfs_find_actor() is after the horse has left the
> >> barn so to speak.
> >>
> >> I think the problem is in nfs_fhget() when constructing the new
> >> inode (inside the if (inode->i_state & I_NEW) {...}) and should be
> >> addressed there.
> >>
> >> The nfs module already has checks to ensure that the expression
> >> "((S_IFMT & inode->i_mode) == (S_IFMT & fattr->mode))" is true
> >> in nfs_update_inode() and nfs_check_inode_attributes(). I think
> >> problem Benny hit was that the equivalent check is missing in
> >> nfs_fhget() when constructing a new inode. The same check that's
> >> in those other two functions needs to be added to nfs_fhget()'s "if
> >> (inode->i_state & I_NEW) {...}" conditional block to prevent the
> >> problem of an inconsistent fattr/inode state from occurring in the
> >> first place.
> >>
> >> I can come up with a patch if you'd like, but I wanted to make
> >> sure my assertions were valid first. Is removing a check from
> >> nfs_find_actor() and adding the check to nfs_fhget() to prevent the
> >> inconsistency in the first place a better approach?
> >>
> >> Quentin
> >
> > I don't think I agree. The whole problem was that we were allowing
> > iget5_locked to match an existing inode even though (S_IFMT & mode) was
> > different.
>
> I assert that when an inconsistent state happens you don't want
> nfs_find_actor() to be the one responsible for noticing it
> (actually, ignoring the problem). You want the existing consistency
> and error recovery paths to handle it instead.
>
> So one or two things are going on. The existing error recovery
> paths are inadequate or that there's a hole in the fattr/inode
> consistency checking (or possibly both). Your patch covers over
> this underlying bug rather than addressing it and which also lets
> the old inode not get flagged as invalid. I would rather see the
> real problem fixed and the old inode get flagged rather than just
> ignoring it and creating another new inode.
>
> Before your patch, if because of a server bug reusing a file
> handle, nfs_find_actor(), would match and iget5_locked() would
> return an inode that matched on its file handle but that didn't
> necessarily match the fattr info.
>
> If that inode returned by iget5_locked() was not I_NEW (the else
> case), nfs_fhget() would invoke nfs_refresh_inode(), which would
> call nfs_refresh_inode_locked(). It calls either nfs_update_inode()
> or nfs_check_inode_attributes(). Both of those functions already
> check for fattr/inode S_IFMT consistency catching bad states.
>
> > Why would it be preferable to use the same struct inode even though
> > it's clearly a different one on the server? inodes generally don't
> > change their type after they've been created.
>
> It's not preferable to have the same inode in a different state.
> What you don't want is nfs_find_actor() as a side-effect to let the
> problem go unnoticed. You want let the normal, existing error
> handling paths deal with it, or if they're not fully dealing with
> it, fix them.
>
> Since the I_NEW conditional false path (the else case) in nfs_fhget()
> should catch the problem, I had thought that the problem must lay
> with the I_NEW conditional true path. Now I don't think so.
>
> I see now that nfs_check_inode_attributes() is just returning an
> error when the fattr and inode states are found to be inconsistent,
> but is taking no action on the bad inode's state (i.e. flagging it
> as invalid). And upon return, nfs_fhget() is not doing anything with
> the bad return state passed up to nfs_refresh_inode(). Now I'm
> thinking that this is problematic hole because it is ignoring the
> error state and that let Benny's panic happen.
>
> It doesn't seem to me that it should be nfs_check_inode_attributes()
> job to flag the inode as invalid. That seems outside its role. I
> would think it should be nfs_refresh_inode_locked()'s job, but I'm
> still reviewing the code.
>
> Thoughts?
Quite frankly, all I care about in a situation like this is that the
client doesn't Oops.
If a server is really this utterly broken, then there is no way we can
fix it on the client, and we're not even going to try.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
