Re: [PATCH] cifs: when renaming don't try to unlink negative dentry

On Sat, 18 Apr 2009 09:43:18 -0500
Steve French <[email protected]> wrote:

> On Sat, Apr 18, 2009 at 5:05 AM, Jeff Layton <[email protected]> wrote:
> > On Fri, 17 Apr 2009 21:31:50 -0500
> > Steve French <[email protected]> wrote:
> >
> >> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <[email protected]> wrote:
> >> > On Fri, 17 Apr 2009 16:16:23 -0500
> >> > Steve French <[email protected]> wrote:
> >> >
> >> >> I merged this, adding the CC: stable, but think we need to also
> >> >> consistently check for inode == NULL in cifs_unlink (we only check in
> >> >> two branches now)
> >> >>
> >> >> Any objections if I also add the following check:
> >> >>
> >> >
> >> > I think it would be preferable to just check once for inode==NULL in
> >> > cifs_unlink near the top and BUG() if it is. Note that vfs_unlink takes
> >> > the i_mutex on this before calling the .unlink inode op, so we're
> >> > guaranteed that the d_inode won't be NULL from that codepath.
> >> >
> >> > I think if we get an unlink on a negative dentry then we should
> >> > probably consider that a BUG().
> >> >
> >> > Other .unlink ops also seem to assume that you can't call .unlink with
> >> > a negative dentry.
> >>
> >> I am more worried about internal calls to unlink from cifs slipping
> >> through with inode null.  If we check in one branch we should check in
> >> the other, or as you suggest move it to the top
> >>
> >
> > Yep, internal callers are my worry too. That's why I think a BUG() is
> > appropriate to help catch this when it occurs. Maybe something like
> > this patch?
> 
> Even if in some strange error path (the kind of cleanup code you added in
> various rename paths for example) an internal caller of course has to
> have a path (dentry) and should have an inode but I am not convinced
> that we should force having a cached inode in all error/cleanup paths -
> why is having a local cached copy of the inode fundamentally
> required to do unlink (the server state, not the client state is what
> is current).   For example, doing this check prevents doing
> unlink when there is a negative dentry - but the file still may exist
> on the server
> (a negative dentry is just a "cached" directory entry - but the file may have
> been recently created on the server).  I don't really see a reason why
> why we should assume that a negative dentry or dentry without an inode
>  (for example, perhaps there are other error cases) should always
> prevent us from trying unlink.
> 
> I don't mind throwing the cERROR on this case, but it seems odd to require
> (in all potential internal unlink uses) an inode if all we are doing
> with the inode is
> updating cached inode information (if we don't have cached inode information,
> we can still send the SMB and do the unlink).
> 
> I do agree that today we do have a cached inode in all paths
> that call into cifs_unlink in current cifs (presumably that was not
> always the case)
> (except the negative dentry case which your patch now forbids), but
> the existence (or not) of the dentry->d_inode does not really have
> anything to do with whether SMB Delete File would work or not.

Why would we want the kernel to attempt to delete a file that it
doesn't believe exists? It seems reasonable to expect that the caller
have an up to date dcache for this.

I don't think we can reasonably have cifs_unlink handle the case where
we have a negative dentry. That will add a lot of complexity to this
codepath (and it's already very complex as it is). We have to recognize
that cifs_unlink does more than just SMB_Delete_File. It'll try to do a
busy file rename in some cases, the delete on close bit, etc.

If the caller just wants a simple SMB_Delete_File attempt then it
should just do that call itself.

That said, I don't feel terribly strongly about the BUG() call. If you
think it's better to just pop a printk and have cifs_unlink immediately
return an error then I can live with that.

-- 
Jeff Layton <[email protected]>