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

Commit Message

Jeff Layton April 17, 2009, 3:45 p.m. UTC

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(-)

Comments

Christoph Hellwig May 11, 2009, 8:55 a.m. UTC | #1

> +	/* Try unlinking the target dentry if it's not negative */
> +	if (target_dentry->d_inode && (rc == -EACCES || rc == -EEXIST)) {

That comment is rather confusing to the reader.  A negative dentry
is a Linux implementation detail.  What is really mean here is "delete
the target if it exists".

Also cifs_rename for that case seems like it's not atomic as required
by posix, as cifs_do_rename might fail after the target has been removed
already.

Mail servers won't be very happy on cifs :)

Jeff Layton May 11, 2009, 11:03 a.m. UTC | #2

On Mon, 11 May 2009 04:55:04 -0400
Christoph Hellwig <[e-mail]> wrote:

> > +	/* Try unlinking the target dentry if it's not negative */
> > +	if (target_dentry->d_inode && (rc == -EACCES || rc == -EEXIST)) {
> 
> That comment is rather confusing to the reader.  A negative dentry
> is a Linux implementation detail.  What is really mean here is "delete
> the target if it exists".
> 
> Also cifs_rename for that case seems like it's not atomic as required
> by posix, as cifs_do_rename might fail after the target has been removed
> already.
> 
> Mail servers won't be very happy on cifs :)
> 

Yes, the cifs_rename codepaths are not atomic. There's unfortunately
not much we can do about that given the limitations of the protocol and
servers. When I initially did this code, I attempted to at least make
them back out changes when somethng failed, but it became a real mess.
Unfortunately, a lot of apps rely on being able to delete and rename
open files on top of one another so the alternative is that these apps
just fail.

One possibility is to consider changing this so that these operations
just fail on these servers, and add a mount option to allow "relaxed"
posix standards that enables the non-atomic renames (and maybe other
places where we bend the posix rules a bit).

Regardless though, the mount.cifs manpage should probably make mention
of this fact. Eventually I'd like to see a cifs(5) manpage, similar to
the nfs(5) one.

> 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.

Agreed. Steve's already committed a patch to make it "safe" to call
cifs_unlink on a negative dentry, but I'd prefer to see that reverted
and the BUG_ON in its place.

Patch

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;