Re: [PATCH] cifs: when renaming don't try to unlink negative dentry
Jeff Layton <[email protected]>
| Newsgroups | gmane.linux.file-systems.cifs |
|---|---|
| Message-ID | <[email protected]> |
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? -- Jeff Layton <[email protected]> _______________________________________________ linux-cifs-client mailing list [email protected] https://lists.samba.org/mailman/listinfo/linux-cifs-client
0001-cifs-don-t-allow-cifs_unlink-to-be-called-on-negati.patch
(text/x-patch, 1.9 KB)
>From 7364188d2637bf58dca7c4ecdf6e73b5c281b4ad Mon Sep 17 00:00:00 2001 From: Jeff Layton <[email protected]> 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 <[email protected]> --- 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