[PATCH] smb: client: fix filename matching of deferred files

[PATCH] smb: client: fix filename matching of deferred files

[Paulo Alcantara]

Fix the following case where the client would end up closing both
deferred files (foo.tmp & foo) after unlink(foo) due to strstr() call
in cifs_close_deferred_file_under_dentry():

  fd1 = openat(AT_FDCWD, "foo", O_WRONLY|O_CREAT|O_TRUNC, 0666);
  fd2 = openat(AT_FDCWD, "foo.tmp", O_WRONLY|O_CREAT|O_TRUNC, 0666);
  close(fd1);
  close(fd2);
  unlink("foo");

Fixes: e3fc065682eb ("cifs: Deferred close performance improvements")
Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
Cc: Frank Sorenson <sorenson@redhat.com>
Cc: David Howells <dhowells@redhat.com>
Cc: linux-cifs@vger.kernel.org
---
 fs/smb/client/misc.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
index da23cc12a52c..09d5fa3638c9 100644
--- a/fs/smb/client/misc.c
+++ b/fs/smb/client/misc.c
@@ -845,20 +845,19 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
 	spin_lock(&tcon->open_file_lock);
 	list_for_each_entry(cfile, &tcon->openFileList, tlist) {
 		full_path = build_path_from_dentry(cfile->dentry, page);
-		if (strstr(full_path, path)) {
-			if (delayed_work_pending(&cfile->deferred)) {
-				if (cancel_delayed_work(&cfile->deferred)) {
-					spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
-					cifs_del_deferred_close(cfile);
-					spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
+		if (IS_ERR(full_path) || strcmp(full_path, path))
+			continue;
+		if (delayed_work_pending(&cfile->deferred) &&
+		    cancel_delayed_work(&cfile->deferred)) {
+			spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
+			cifs_del_deferred_close(cfile);
+			spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
 
-					tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
-					if (tmp_list == NULL)
-						break;
-					tmp_list->cfile = cfile;
-					list_add_tail(&tmp_list->list, &file_head);
-				}
-			}
+			tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+			if (tmp_list == NULL)
+				break;
+			tmp_list->cfile = cfile;
+			list_add_tail(&tmp_list->list, &file_head);
 		}
 	}
 	spin_unlock(&tcon->open_file_lock);
-- 
2.51.0

Re: [PATCH] smb: client: fix filename matching of deferred files

[Enzo Matsumiya]

On 09/16, Paulo Alcantara wrote:
>Fix the following case where the client would end up closing both
>deferred files (foo.tmp & foo) after unlink(foo) due to strstr() call
>in cifs_close_deferred_file_under_dentry():
>
>  fd1 = openat(AT_FDCWD, "foo", O_WRONLY|O_CREAT|O_TRUNC, 0666);
>  fd2 = openat(AT_FDCWD, "foo.tmp", O_WRONLY|O_CREAT|O_TRUNC, 0666);
>  close(fd1);
>  close(fd2);
>  unlink("foo");

Good catch, but since you're at it, wouldn't it be more elegant to
replace @path with @dentry and just compare cfile->dentry == dentry?

Would also improve performance by saving calls to
build_path_from_dentry().


Cheers,

Enzo

>Fixes: e3fc065682eb ("cifs: Deferred close performance improvements")
>Signed-off-by: Paulo Alcantara (Red Hat) <pc@manguebit.org>
>Cc: Frank Sorenson <sorenson@redhat.com>
>Cc: David Howells <dhowells@redhat.com>
>Cc: linux-cifs@vger.kernel.org
>---
> fs/smb/client/misc.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
>diff --git a/fs/smb/client/misc.c b/fs/smb/client/misc.c
>index da23cc12a52c..09d5fa3638c9 100644
>--- a/fs/smb/client/misc.c
>+++ b/fs/smb/client/misc.c
>@@ -845,20 +845,19 @@ cifs_close_deferred_file_under_dentry(struct cifs_tcon *tcon, const char *path)
> 	spin_lock(&tcon->open_file_lock);
> 	list_for_each_entry(cfile, &tcon->openFileList, tlist) {
> 		full_path = build_path_from_dentry(cfile->dentry, page);
>-		if (strstr(full_path, path)) {
>-			if (delayed_work_pending(&cfile->deferred)) {
>-				if (cancel_delayed_work(&cfile->deferred)) {
>-					spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
>-					cifs_del_deferred_close(cfile);
>-					spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
>+		if (IS_ERR(full_path) || strcmp(full_path, path))
>+			continue;
>+		if (delayed_work_pending(&cfile->deferred) &&
>+		    cancel_delayed_work(&cfile->deferred)) {
>+			spin_lock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
>+			cifs_del_deferred_close(cfile);
>+			spin_unlock(&CIFS_I(d_inode(cfile->dentry))->deferred_lock);
>
>-					tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
>-					if (tmp_list == NULL)
>-						break;
>-					tmp_list->cfile = cfile;
>-					list_add_tail(&tmp_list->list, &file_head);
>-				}
>-			}
>+			tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
>+			if (tmp_list == NULL)
>+				break;
>+			tmp_list->cfile = cfile;
>+			list_add_tail(&tmp_list->list, &file_head);
> 		}
> 	}
> 	spin_unlock(&tcon->open_file_lock);
>-- 
>2.51.0
>
>

Re: [PATCH] smb: client: fix filename matching of deferred files

[Paulo Alcantara]

Enzo Matsumiya <ematsumiya@suse.de> writes:

> On 09/16, Paulo Alcantara wrote:
>>Fix the following case where the client would end up closing both
>>deferred files (foo.tmp & foo) after unlink(foo) due to strstr() call
>>in cifs_close_deferred_file_under_dentry():
>>
>>  fd1 = openat(AT_FDCWD, "foo", O_WRONLY|O_CREAT|O_TRUNC, 0666);
>>  fd2 = openat(AT_FDCWD, "foo.tmp", O_WRONLY|O_CREAT|O_TRUNC, 0666);
>>  close(fd1);
>>  close(fd2);
>>  unlink("foo");
>
> Good catch, but since you're at it, wouldn't it be more elegant to
> replace @path with @dentry and just compare cfile->dentry == dentry?
>
> Would also improve performance by saving calls to
> build_path_from_dentry().

Sounds good, thanks.