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

Commit Message

Steve French April 17, 2009, 9:16 p.m. UTC

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:


On Fri, Apr 17, 2009 at 10:45 AM, Jeff Layton <[e-mail]> wrote:
> When attempting to rename a file on a read-only share, the kernel can
> call cifs_unlink on a negative dentry, which causes an oops. Only try
> to unlink the file if it's a positive dentry.
>
> Signed-off-by: Jeff Layton <[e-mail]>
> Tested-by: Shirish Pargaonkar <[e-mail]>
> ---
>  fs/cifs/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 09082ac..f36b4e4 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1453,7 +1453,8 @@ int cifs_rename(struct inode *source_dir, struct dentry *source_dentry,
>                     checking the UniqueId via FILE_INTERNAL_INFO */
>
>  unlink_target:
> -       if ((rc == -EACCES) || (rc == -EEXIST)) {
> +       /* Try unlinking the target dentry if it's not negative */
> +       if (target_dentry->d_inode && (rc == -EACCES || rc == -EEXIST)) {
>                tmprc = cifs_unlink(target_dir, target_dentry);
>                if (tmprc)
>                        goto cifs_rename_exit;
> --
> 1.6.0.6
>
>

Comments

Jeff Layton April 17, 2009, 11:02 p.m. UTC | #1

On Fri, 17 Apr 2009 16:16:23 -0500
Steve French <[e-mail]> 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.

Steve French April 18, 2009, 2:31 a.m. UTC | #2

On Fri, Apr 17, 2009 at 6:02 PM, Jeff Layton <[e-mail]> wrote:
> On Fri, 17 Apr 2009 16:16:23 -0500
> Steve French <[e-mail]> 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

Patch

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 09082ac..42572b7 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -968,7 +968,7 @@  int cifs_unlink(struct inode *dir, struct dentry *dentry)
 	int xid;
 	char *full_path = NULL;
 	struct inode *inode = dentry->d_inode;
-	struct cifsInodeInfo *cifsInode = CIFS_I(inode);
+	struct cifsInodeInfo *cifs_inode;
 	struct super_block *sb = dir->i_sb;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
 	struct cifsTconInfo *tcon = cifs_sb->tcon;
@@ -1012,7 +1012,7 @@  psx_del_no_retry:
 		rc = cifs_rename_pending_delete(full_path, dentry, xid);
 		if (rc == 0)
 			drop_nlink(inode);
-	} else if (rc == -EACCES && dosattr == 0) {
+	} else if ((rc == -EACCES) && (dosattr == 0) && inode) {
 		attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
 		if (attrs == NULL) {
 			rc = -ENOMEM;
@@ -1020,7 +1020,8 @@  psx_del_no_retry:
 		}

 		/* try to reset dos attributes */
-		origattr = cifsInode->cifsAttrs;
+		cifs_inode = CIFS_I(inode);
+		origattr = cifs_inode->cifsAttrs;
 		if (origattr == 0)
 			origattr |= ATTR_NORMAL;
 		dosattr = origattr & ~ATTR_READONLY;
@@ -1041,13 +1042,13 @@  psx_del_no_retry:

 out_reval:
 	if (inode) {
-		cifsInode = CIFS_I(inode);
-		cifsInode->time = 0;	/* will force revalidate to get info
+		cifs_inode = CIFS_I(inode);
+		cifs_inode->time = 0;	/* will force revalidate to get info
 					   when needed */
 		inode->i_ctime = current_fs_time(sb);
 	}
 	dir->i_ctime = dir->i_mtime = current_fs_time(sb);
-	cifsInode = CIFS_I(dir);
+	cifs_inode = CIFS_I(dir);
 	CIFS_I(dir)->time = 0;	/* force revalidate of dir as well */

 	kfree(full_path);