| Message ID | 20090418060558.016e3ef0@tleilax.poochiereds.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 18, 2009 at 5:05 AM, Jeff Layton <jlayton@redhat.com> wrote: > On Fri, 17 Apr 2009 21:31:50 -0500 > Steve French <smfrench@gmail.com> wrote: > >> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <jlayton@redhat.com> wrote: >> > On Fri, 17 Apr 2009 16:16:23 -0500 >> > Steve French <smfrench@gmail.com> 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.
On Sat, 18 Apr 2009 09:43:18 -0500 Steve French <smfrench@gmail.com> wrote: > On Sat, Apr 18, 2009 at 5:05 AM, Jeff Layton <jlayton@redhat.com> wrote: > > On Fri, 17 Apr 2009 21:31:50 -0500 > > Steve French <smfrench@gmail.com> wrote: > > > >> On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <jlayton@redhat.com> wrote: > >> > On Fri, 17 Apr 2009 16:16:23 -0500 > >> > Steve French <smfrench@gmail.com> 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.
On Sat, Apr 18, 2009 at 03:32:02PM -0400, Jeff Layton wrote: > 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. Please put the BUG_ON in. The current code is typical cargo-cult programming mess, and the newly added comment ontop of cifs_unlink is highly confusing.
>From 7364188d2637bf58dca7c4ecdf6e73b5c281b4ad Mon Sep 17 00:00:00 2001 From: Jeff Layton <jlayton@redhat.com> Date: Sat, 18 Apr 2009 05:56:11 -0400 Subject: [PATCH] cifs: don't allow cifs_unlink to be called on negative dentry External callers (the VFS and knfsd) will never call the unlink inode op on a negative dentry. Internal callers must not do so either. Check for it and BUG() if it occurs. Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/inode.c | 22 +++++++++++++--------- 1 files changed, 13 insertions(+), 9 deletions(-) diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index f36b4e4..5476e0f 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -977,6 +977,11 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry) cFYI(1, ("cifs_unlink, dir=0x%p, dentry=0x%p", dir, dentry)); + if (!inode) { + cERROR(1, ("cifs_unlink called on negative dentry!")); + BUG(); + } + xid = GetXid(); /* Unlink can be called from rename so we can not take the @@ -1004,8 +1009,7 @@ retry_std_delete: psx_del_no_retry: if (!rc) { - if (inode) - drop_nlink(inode); + drop_nlink(inode); } else if (rc == -ENOENT) { d_drop(dentry); } else if (rc == -ETXTBSY) { @@ -1040,15 +1044,15 @@ psx_del_no_retry: cifs_set_file_info(inode, attrs, xid, full_path, origattr); out_reval: - if (inode) { - cifsInode = CIFS_I(inode); - cifsInode->time = 0; /* will force revalidate to get info - when needed */ - inode->i_ctime = current_fs_time(sb); - } + /* forces a revalidate when next needed */ + cifsInode = CIFS_I(inode); + cifsInode->time = 0; + + /* force revalidate of dir as well */ + inode->i_ctime = current_fs_time(sb); dir->i_ctime = dir->i_mtime = current_fs_time(sb); cifsInode = CIFS_I(dir); - CIFS_I(dir)->time = 0; /* force revalidate of dir as well */ + CIFS_I(dir)->time = 0; kfree(full_path); kfree(attrs); -- 1.6.0.6