[PATCH 0/3] Improvements to cifs_rename_pending_delete()

[PATCH 0/3] Improvements to cifs_rename_pending_delete()

[Sachin Prabhu]

The patch series consists of a bug fix and a couple of enhancements
to cifs_rename_pending_delete()

Sachin Prabhu (3):
  cifs: Fix bug when checking error condition in
    cifs_rename_pending_delete()
  cifs: Check server capability before attempting silly rename
  cifs: Return EBUSY instead of ETXTBSY in cifs_rename_pending_delete()

 fs/cifs/inode.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
1.7.11.7

[PATCH 1/3] cifs: Fix bug when checking error condition in cifs_rename_pending_delete()

[Sachin Prabhu]

Fix check for error condition after setting attributes with
CIFSSMBSetFileInfo().

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index ed6208f..a183b9d 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1014,7 +1014,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 					current->tgid);
 		/* although we would like to mark the file hidden
  		   if that fails we will still try to rename it */
-		if (rc != 0)
+		if (!rc)
 			cifsInode->cifsAttrs = dosattr;
 		else
 			dosattr = origattr; /* since not able to change them */
-- 
1.7.11.7

Re: [PATCH 1/3] cifs: Fix bug when checking error condition in cifs_rename_pending_delete()

[Jeff Layton]

On Tue,  5 Mar 2013 19:25:55 +0000
Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Fix check for error condition after setting attributes with
> CIFSSMBSetFileInfo().
> 
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index ed6208f..a183b9d 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1014,7 +1014,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>  					current->tgid);
>  		/* although we would like to mark the file hidden
>   		   if that fails we will still try to rename it */
> -		if (rc != 0)
> +		if (!rc)
>  			cifsInode->cifsAttrs = dosattr;
>  		else
>  			dosattr = origattr; /* since not able to change them */

Nice catch

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 1/3] cifs: Fix bug when checking error condition in cifs_rename_pending_delete()

[Pavel Shilovsky]

2013/3/6 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Tue,  5 Mar 2013 19:25:55 +0000
> Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Fix check for error condition after setting attributes with
>> CIFSSMBSetFileInfo().
>>
>> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/inode.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index ed6208f..a183b9d 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -1014,7 +1014,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>>                                       current->tgid);
>>               /* although we would like to mark the file hidden
>>                  if that fails we will still try to rename it */
>> -             if (rc != 0)
>> +             if (!rc)
>>                       cifsInode->cifsAttrs = dosattr;
>>               else
>>                       dosattr = origattr; /* since not able to change them */
>
> Nice catch
>
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reviewed-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>

-- 
Best regards,
Pavel Shilovsky.

[PATCH 2/3] cifs: Check server capability before attempting silly rename

[Sachin Prabhu]

cifs_rename_pending_delete() attempts to silly rename file using
CIFSSMBRenameOpenFile(). This uses the SET_FILE_INFORMATION TRANS2
command with information level set to the passthru info-level
SMB_SET_FILE_RENAME_INFORMATION.

We need to check to make sure that the server support passthru
info-levels before attempting the silly rename or else we will fail to
rename the file.

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/inode.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index a183b9d..fee2d40 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -986,6 +986,15 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 		return PTR_ERR(tlink);
 	tcon = tlink_tcon(tlink);
 
+	/*
+	 * We cannot rename the file if the server doesn't support
+	 * CAP_INFOLEVEL_PASSTHRU
+	 */
+	if (!(tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)) {
+		rc = -EBUSY;
+		goto out;
+	}
+
 	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
 			 DELETE|FILE_WRITE_ATTRIBUTES, CREATE_NOT_DIR,
 			 &netfid, &oplock, NULL, cifs_sb->local_nls,
-- 
1.7.11.7

Re: [PATCH 2/3] cifs: Check server capability before attempting silly rename

[Jeff Layton]

On Tue,  5 Mar 2013 19:25:56 +0000
Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> cifs_rename_pending_delete() attempts to silly rename file using
> CIFSSMBRenameOpenFile(). This uses the SET_FILE_INFORMATION TRANS2
> command with information level set to the passthru info-level
> SMB_SET_FILE_RENAME_INFORMATION.
> 
> We need to check to make sure that the server support passthru
> info-levels before attempting the silly rename or else we will fail to
> rename the file.
> 
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/inode.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index a183b9d..fee2d40 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -986,6 +986,15 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>  		return PTR_ERR(tlink);
>  	tcon = tlink_tcon(tlink);
>  
> +	/*
> +	 * We cannot rename the file if the server doesn't support
> +	 * CAP_INFOLEVEL_PASSTHRU
> +	 */
> +	if (!(tcon->ses->capabilities & CAP_INFOLEVEL_PASSTHRU)) {
> +		rc = -EBUSY;
> +		goto out;
> +	}
> +
>  	rc = CIFSSMBOpen(xid, tcon, full_path, FILE_OPEN,
>  			 DELETE|FILE_WRITE_ATTRIBUTES, CREATE_NOT_DIR,
>  			 &netfid, &oplock, NULL, cifs_sb->local_nls,

We almost certainly need to do the same in other places too. The cifs
code does too much "try and fall back" when it should first check for
capabilities. Keep your eyes open for others!

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH 3/3] cifs: Return EBUSY instead of ETXTBSY in cifs_rename_pending_delete()

[Sachin Prabhu]

Instead of remapping ETXTBSY errors to EBUSY in cifs_unlink, replace all
cases of ETXTBSY in cifs_rename_pending_delete() with EBUSY.

Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/inode.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index fee2d40..0ab0328 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1034,7 +1034,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 				   cifs_sb->mnt_cifs_flags &
 					    CIFS_MOUNT_MAP_SPECIAL_CHR);
 	if (rc != 0) {
-		rc = -ETXTBSY;
+		rc = -EBUSY;
 		goto undo_setattr;
 	}
 
@@ -1053,7 +1053,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 		if (rc == -ENOENT)
 			rc = 0;
 		else if (rc != 0) {
-			rc = -ETXTBSY;
+			rc = -EBUSY;
 			goto undo_rename;
 		}
 		cifsInode->delete_pending = true;
@@ -1167,8 +1167,6 @@ psx_del_no_retry:
 			if (rc == 0)
 				cifs_drop_nlink(inode);
 		}
-		if (rc == -ETXTBSY)
-			rc = -EBUSY;
 	} else if ((rc == -EACCES) && (dosattr == 0) && inode) {
 		attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
 		if (attrs == NULL) {
-- 
1.7.11.7

Re: [PATCH 3/3] cifs: Return EBUSY instead of ETXTBSY in cifs_rename_pending_delete()

[Jeff Layton]

On Tue,  5 Mar 2013 19:25:57 +0000
Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Instead of remapping ETXTBSY errors to EBUSY in cifs_unlink, replace all
> cases of ETXTBSY in cifs_rename_pending_delete() with EBUSY.
> 
> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/inode.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index fee2d40..0ab0328 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -1034,7 +1034,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>  				   cifs_sb->mnt_cifs_flags &
>  					    CIFS_MOUNT_MAP_SPECIAL_CHR);
>  	if (rc != 0) {
> -		rc = -ETXTBSY;
> +		rc = -EBUSY;
>  		goto undo_setattr;
>  	}
>  
> @@ -1053,7 +1053,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>  		if (rc == -ENOENT)
>  			rc = 0;
>  		else if (rc != 0) {
> -			rc = -ETXTBSY;
> +			rc = -EBUSY;
>  			goto undo_rename;
>  		}
>  		cifsInode->delete_pending = true;
> @@ -1167,8 +1167,6 @@ psx_del_no_retry:
>  			if (rc == 0)
>  				cifs_drop_nlink(inode);
>  		}
> -		if (rc == -ETXTBSY)
> -			rc = -EBUSY;



This doesn't look quite right. If ->rename_pending_delete isn't set,
then rc will still stay -ETXTBSY. I think this is a good cleanup, but
we need to consider changing the mapping for
NT_STATUS_SHARING_VIOLATION to -EBUSY as well, and fix all of the
places that look for -ETXTBSY in the cifs code.


>  	} else if ((rc == -EACCES) && (dosattr == 0) && inode) {
>  		attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
>  		if (attrs == NULL) {


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 3/3] cifs: Return EBUSY instead of ETXTBSY in cifs_rename_pending_delete()

[Steve French]

Does someone have a good explanation of the difference between text
file busy and device busy?  almost seems like we should be returning
EXTBUSY more based on the description

On Wed, Mar 6, 2013 at 6:39 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue,  5 Mar 2013 19:25:57 +0000
> Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> Instead of remapping ETXTBSY errors to EBUSY in cifs_unlink, replace all
>> cases of ETXTBSY in cifs_rename_pending_delete() with EBUSY.
>>
>> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  fs/cifs/inode.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
>> index fee2d40..0ab0328 100644
>> --- a/fs/cifs/inode.c
>> +++ b/fs/cifs/inode.c
>> @@ -1034,7 +1034,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>>                                  cifs_sb->mnt_cifs_flags &
>>                                           CIFS_MOUNT_MAP_SPECIAL_CHR);
>>       if (rc != 0) {
>> -             rc = -ETXTBSY;
>> +             rc = -EBUSY;
>>               goto undo_setattr;
>>       }
>>
>> @@ -1053,7 +1053,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
>>               if (rc == -ENOENT)
>>                       rc = 0;
>>               else if (rc != 0) {
>> -                     rc = -ETXTBSY;
>> +                     rc = -EBUSY;
>>                       goto undo_rename;
>>               }
>>               cifsInode->delete_pending = true;
>> @@ -1167,8 +1167,6 @@ psx_del_no_retry:
>>                       if (rc == 0)
>>                               cifs_drop_nlink(inode);
>>               }
>> -             if (rc == -ETXTBSY)
>> -                     rc = -EBUSY;
>
>
>
> This doesn't look quite right. If ->rename_pending_delete isn't set,
> then rc will still stay -ETXTBSY. I think this is a good cleanup, but
> we need to consider changing the mapping for
> NT_STATUS_SHARING_VIOLATION to -EBUSY as well, and fix all of the
> places that look for -ETXTBSY in the cifs code.
>
>
>>       } else if ((rc == -EACCES) && (dosattr == 0) && inode) {
>>               attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
>>               if (attrs == NULL) {
>
>
> --
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

Re: [PATCH 3/3] cifs: Return EBUSY instead of ETXTBSY in cifs_rename_pending_delete()

[Jeff Layton]

On Wed, 6 Mar 2013 18:32:46 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Does someone have a good explanation of the difference between text
> file busy and device busy?  almost seems like we should be returning
> EXTBUSY more based on the description
> 

ETXTBSY has a more specific meaning on Linux. Mostly, it's used when
you try to alter a binary that's being executed (though there are other
uses -- see mmap(2) for instance).

EBUSY is more general: "Device or Resource busy". That error seems more
appropriate to me in this case. It's also listed as a well-recognized
error in the unlink(2) manpage and here:

    http://pubs.opengroup.org/onlinepubs/009604599/functions/unlink.html

The latter page also lists ETXTBSY as a valid return code, but it has a
somewhat more obscure meaning there.

> On Wed, Mar 6, 2013 at 6:39 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue,  5 Mar 2013 19:25:57 +0000
> > Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> Instead of remapping ETXTBSY errors to EBUSY in cifs_unlink, replace all
> >> cases of ETXTBSY in cifs_rename_pending_delete() with EBUSY.
> >>
> >> Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  fs/cifs/inode.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> >> index fee2d40..0ab0328 100644
> >> --- a/fs/cifs/inode.c
> >> +++ b/fs/cifs/inode.c
> >> @@ -1034,7 +1034,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
> >>                                  cifs_sb->mnt_cifs_flags &
> >>                                           CIFS_MOUNT_MAP_SPECIAL_CHR);
> >>       if (rc != 0) {
> >> -             rc = -ETXTBSY;
> >> +             rc = -EBUSY;
> >>               goto undo_setattr;
> >>       }
> >>
> >> @@ -1053,7 +1053,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
> >>               if (rc == -ENOENT)
> >>                       rc = 0;
> >>               else if (rc != 0) {
> >> -                     rc = -ETXTBSY;
> >> +                     rc = -EBUSY;
> >>                       goto undo_rename;
> >>               }
> >>               cifsInode->delete_pending = true;
> >> @@ -1167,8 +1167,6 @@ psx_del_no_retry:
> >>                       if (rc == 0)
> >>                               cifs_drop_nlink(inode);
> >>               }
> >> -             if (rc == -ETXTBSY)
> >> -                     rc = -EBUSY;
> >
> >
> >
> > This doesn't look quite right. If ->rename_pending_delete isn't set,
> > then rc will still stay -ETXTBSY. I think this is a good cleanup, but
> > we need to consider changing the mapping for
> > NT_STATUS_SHARING_VIOLATION to -EBUSY as well, and fix all of the
> > places that look for -ETXTBSY in the cifs code.
> >
> >
> >>       } else if ((rc == -EACCES) && (dosattr == 0) && inode) {
> >>               attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> >>               if (attrs == NULL) {
> >
> >
> > --
> > Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 3/3] cifs: Return EBUSY instead of ETXTBSY in cifs_rename_pending_delete()

[Sachin Prabhu]

On Wed, 2013-03-06 at 07:39 -0500, Jeff Layton wrote:
> On Tue,  5 Mar 2013 19:25:57 +0000
> Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > Instead of remapping ETXTBSY errors to EBUSY in cifs_unlink, replace all
> > cases of ETXTBSY in cifs_rename_pending_delete() with EBUSY.
> > 
> > Signed-off-by: Sachin Prabhu <sprabhu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/inode.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> > index fee2d40..0ab0328 100644
> > --- a/fs/cifs/inode.c
> > +++ b/fs/cifs/inode.c
> > @@ -1034,7 +1034,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
> >  				   cifs_sb->mnt_cifs_flags &
> >  					    CIFS_MOUNT_MAP_SPECIAL_CHR);
> >  	if (rc != 0) {
> > -		rc = -ETXTBSY;
> > +		rc = -EBUSY;
> >  		goto undo_setattr;
> >  	}
> >  
> > @@ -1053,7 +1053,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
> >  		if (rc == -ENOENT)
> >  			rc = 0;
> >  		else if (rc != 0) {
> > -			rc = -ETXTBSY;
> > +			rc = -EBUSY;
> >  			goto undo_rename;
> >  		}
> >  		cifsInode->delete_pending = true;
> > @@ -1167,8 +1167,6 @@ psx_del_no_retry:
> >  			if (rc == 0)
> >  				cifs_drop_nlink(inode);
> >  		}
> > -		if (rc == -ETXTBSY)
> > -			rc = -EBUSY;
> 
> 
> 
> This doesn't look quite right. If ->rename_pending_delete isn't set,
> then rc will still stay -ETXTBSY. I think this is a good cleanup, but
> we need to consider changing the mapping for
> NT_STATUS_SHARING_VIOLATION to -EBUSY as well, and fix all of the
> places that look for -ETXTBSY in the cifs code.

Hello Jeff, 

NT_STATUS_SHARING_VIOLATION for smb2 is already set to return -EBUSY in 
smb2_error_map_table.

I looked into changing NT_STATUS_SHARING_VIOLATION for smb1 to use
-EBUSY instead. A quick check shows that NT_STATUS_SHARING_VIOLATION is
the only error condition linked to ETXTBSY which in turn is only checked
for in cifs_unlink() and in cifs_do_rename(). 
I then checked all possible error messages returned for smb1 and checked
for EBUSY mapped to these error messages. None of those appear to be
mapped to EBUSY either.

So making this change should be easy. I'll post the new patch soon.

Sachin Prabhu