linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
@ 2021-08-09  9:58 Rohith Surabattula
  2021-08-10 11:47 ` Shyam Prasad N
  0 siblings, 1 reply; 8+ messages in thread
From: Rohith Surabattula @ 2021-08-09  9:58 UTC (permalink / raw)
  To: linux-cifs, Steve French, Shyam Prasad N, ronnie sahlberg

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

Hi Steve/All,

During unlink/rename/lease break, deferred work for close is
scheduled immediately but in asynchronous manner which might
lead to race with actual(unlink/rename) commands.

This change will schedule close synchronously which will avoid
the race conditions with other commands.

Regards,
Rohith

[-- Attachment #2: cifs-Call-close-synchronously-during-unlink-rename-l.patch --]
[-- Type: application/octet-stream, Size: 5699 bytes --]

From 60a14e1a98c97bcd047a776afbcfdb883e269007 Mon Sep 17 00:00:00 2001
From: Rohith Surabattula
 <rohiths@microsoft.com>
Date: Mon, 9 Aug 2021 09:32:46 +0000
Subject: [PATCH] cifs: Call close synchronously during unlink/rename/lease
 break.

During unlink/rename/lease break, deferred work for close is
scheduled immediately but in asynchronous manner which might
lead to race with actual(unlink/rename) commands.

This change will schedule close synchronously which will avoid
the race conditions with other commands.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
---
 fs/cifs/cifsglob.h |  5 +++++
 fs/cifs/file.c     | 35 +++++++++++++++++------------------
 fs/cifs/misc.c     | 42 ++++++++++++++++++++++++++++++------------
 3 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index c0bfc2f01030..c6a9542ca281 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1611,6 +1611,11 @@ struct dfs_info3_param {
 	int ttl;
 };
 
+struct file_list {
+	struct list_head list;
+	struct cifsFileInfo *cfile;
+};
+
 /*
  * common struct for holding inode info when searching for or updating an
  * inode with new info
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 0a72840a88f1..bb98fbdd22a9 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4847,17 +4847,6 @@ void cifs_oplock_break(struct work_struct *work)
 		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
 
 oplock_break_ack:
-	/*
-	 * releasing stale oplock after recent reconnect of smb session using
-	 * a now incorrect file handle is not a data integrity issue but do
-	 * not bother sending an oplock release if session to server still is
-	 * disconnected since oplock already released by the server
-	 */
-	if (!cfile->oplock_break_cancelled) {
-		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
-							     cinode);
-		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
-	}
 	/*
 	 * When oplock break is received and there are no active
 	 * file handles but cached, then schedule deferred close immediately.
@@ -4865,17 +4854,27 @@ void cifs_oplock_break(struct work_struct *work)
 	 */
 	spin_lock(&CIFS_I(inode)->deferred_lock);
 	is_deferred = cifs_is_deferred_close(cfile, &dclose);
+	spin_unlock(&CIFS_I(inode)->deferred_lock);
 	if (is_deferred &&
 	    cfile->deferred_close_scheduled &&
 	    delayed_work_pending(&cfile->deferred)) {
-		/*
-		 * If there is no pending work, mod_delayed_work queues new work.
-		 * So, Increase the ref count to avoid use-after-free.
-		 */
-		if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-			cifsFileInfo_get(cfile);
+		if (cancel_delayed_work(&cfile->deferred)) {
+			_cifsFileInfo_put(cfile, false, false);
+			goto oplock_break_done;
+		}
 	}
-	spin_unlock(&CIFS_I(inode)->deferred_lock);
+	/*
+	 * releasing stale oplock after recent reconnect of smb session using
+	 * a now incorrect file handle is not a data integrity issue but do
+	 * not bother sending an oplock release if session to server still is
+	 * disconnected since oplock already released by the server
+	 */
+	if (!cfile->oplock_break_cancelled) {
+		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
+							     cinode);
+		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
+	}
+oplock_break_done:
 	_cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
 	cifs_done_oplock_break(cinode);
 }
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index cdb1ec1461de..6df4fb92d14a 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -723,20 +723,30 @@ void
 cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
 {
 	struct cifsFileInfo *cfile = NULL;
+	struct file_list *tmp_list;
+	struct list_head file_head;
 
 	if (cifs_inode == NULL)
 		return;
 
+	INIT_LIST_HEAD(&file_head);
+	spin_lock(&cifs_inode->open_file_lock);
 	list_for_each_entry(cfile, &cifs_inode->openFileList, flist) {
 		if (delayed_work_pending(&cfile->deferred)) {
-			/*
-			 * If there is no pending work, mod_delayed_work queues new work.
-			 * So, Increase the ref count to avoid use-after-free.
-			 */
-			if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-				cifsFileInfo_get(cfile);
+			if (cancel_delayed_work(&cfile->deferred)) {
+				tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+				if (tmp_list == NULL)
+					continue;
+				tmp_list->cfile = cfile;
+				list_add_tail(&tmp_list->list, &file_head);
+			}
 		}
 	}
+	spin_unlock(&cifs_inode->open_file_lock);
+
+	list_for_each_entry(tmp_list, &file_head, list) {
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
+	}
 }
 
 void
@@ -744,20 +754,28 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
 {
 	struct cifsFileInfo *cfile;
 	struct list_head *tmp;
+	struct file_list *tmp_list;
+	struct list_head file_head;
 
+	INIT_LIST_HEAD(&file_head);
 	spin_lock(&tcon->open_file_lock);
 	list_for_each(tmp, &tcon->openFileList) {
 		cfile = list_entry(tmp, struct cifsFileInfo, tlist);
 		if (delayed_work_pending(&cfile->deferred)) {
-			/*
-			 * If there is no pending work, mod_delayed_work queues new work.
-			 * So, Increase the ref count to avoid use-after-free.
-			 */
-			if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-				cifsFileInfo_get(cfile);
+			if (cancel_delayed_work(&cfile->deferred)) {
+				tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+				if (tmp_list == NULL)
+					continue;
+				tmp_list->cfile = cfile;
+				list_add_tail(&tmp_list->list, &file_head);
+			}
 		}
 	}
 	spin_unlock(&tcon->open_file_lock);
+
+	list_for_each_entry(tmp_list, &file_head, list) {
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
+	}
 }
 
 /* parses DFS refferal V3 structure
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-09  9:58 [PATCH][CIFS]Call close synchronously during unlink/rename/lease break Rohith Surabattula
@ 2021-08-10 11:47 ` Shyam Prasad N
  2021-08-10 17:29   ` Rohith Surabattula
  0 siblings, 1 reply; 8+ messages in thread
From: Shyam Prasad N @ 2021-08-10 11:47 UTC (permalink / raw)
  To: Rohith Surabattula; +Cc: linux-cifs, Steve French, ronnie sahlberg

Already gave my review comments along with this one to the other email.

Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>

On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
<rohiths.msft@gmail.com> wrote:
>
> Hi Steve/All,
>
> During unlink/rename/lease break, deferred work for close is
> scheduled immediately but in asynchronous manner which might
> lead to race with actual(unlink/rename) commands.
>
> This change will schedule close synchronously which will avoid
> the race conditions with other commands.
>
> Regards,
> Rohith



-- 
Regards,
Shyam

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-10 11:47 ` Shyam Prasad N
@ 2021-08-10 17:29   ` Rohith Surabattula
  2021-08-10 19:28     ` Steve French
  0 siblings, 1 reply; 8+ messages in thread
From: Rohith Surabattula @ 2021-08-10 17:29 UTC (permalink / raw)
  To: Shyam Prasad N; +Cc: linux-cifs, Steve French, ronnie sahlberg

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Hi All,

Have updated the patch to fix the memleak.

Regards,
Rohith

On Tue, Aug 10, 2021 at 5:18 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> Already gave my review comments along with this one to the other email.
>
> Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
>
> On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
> <rohiths.msft@gmail.com> wrote:
> >
> > Hi Steve/All,
> >
> > During unlink/rename/lease break, deferred work for close is
> > scheduled immediately but in asynchronous manner which might
> > lead to race with actual(unlink/rename) commands.
> >
> > This change will schedule close synchronously which will avoid
> > the race conditions with other commands.
> >
> > Regards,
> > Rohith
>
>
>
> --
> Regards,
> Shyam

[-- Attachment #2: cifs-Call-close-synchronously-during-unlink-rename-l.patch --]
[-- Type: application/octet-stream, Size: 5802 bytes --]

From 348cd891cf5246628e5de9cbf419cd81a32ce855 Mon Sep 17 00:00:00 2001
From: Rohith Surabattula <rohiths@microsoft.com>
Date: Mon, 9 Aug 2021 09:32:46 +0000
Subject: [PATCH] cifs: Call close synchronously during unlink/rename/lease
 break.

During unlink/rename/lease break, deferred work for close is
scheduled immediately but in asynchronous manner which might
lead to race with actual(unlink/rename) commands.

This change will schedule close synchronously which will avoid
the race conditions with other commands.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
---
 fs/cifs/cifsglob.h |  5 +++++
 fs/cifs/file.c     | 35 +++++++++++++++++------------------
 fs/cifs/misc.c     | 46 ++++++++++++++++++++++++++++++++++------------
 3 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index c0bfc2f01030..c6a9542ca281 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1611,6 +1611,11 @@ struct dfs_info3_param {
 	int ttl;
 };
 
+struct file_list {
+	struct list_head list;
+	struct cifsFileInfo *cfile;
+};
+
 /*
  * common struct for holding inode info when searching for or updating an
  * inode with new info
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 0a72840a88f1..bb98fbdd22a9 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4847,17 +4847,6 @@ void cifs_oplock_break(struct work_struct *work)
 		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
 
 oplock_break_ack:
-	/*
-	 * releasing stale oplock after recent reconnect of smb session using
-	 * a now incorrect file handle is not a data integrity issue but do
-	 * not bother sending an oplock release if session to server still is
-	 * disconnected since oplock already released by the server
-	 */
-	if (!cfile->oplock_break_cancelled) {
-		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
-							     cinode);
-		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
-	}
 	/*
 	 * When oplock break is received and there are no active
 	 * file handles but cached, then schedule deferred close immediately.
@@ -4865,17 +4854,27 @@ void cifs_oplock_break(struct work_struct *work)
 	 */
 	spin_lock(&CIFS_I(inode)->deferred_lock);
 	is_deferred = cifs_is_deferred_close(cfile, &dclose);
+	spin_unlock(&CIFS_I(inode)->deferred_lock);
 	if (is_deferred &&
 	    cfile->deferred_close_scheduled &&
 	    delayed_work_pending(&cfile->deferred)) {
-		/*
-		 * If there is no pending work, mod_delayed_work queues new work.
-		 * So, Increase the ref count to avoid use-after-free.
-		 */
-		if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-			cifsFileInfo_get(cfile);
+		if (cancel_delayed_work(&cfile->deferred)) {
+			_cifsFileInfo_put(cfile, false, false);
+			goto oplock_break_done;
+		}
 	}
-	spin_unlock(&CIFS_I(inode)->deferred_lock);
+	/*
+	 * releasing stale oplock after recent reconnect of smb session using
+	 * a now incorrect file handle is not a data integrity issue but do
+	 * not bother sending an oplock release if session to server still is
+	 * disconnected since oplock already released by the server
+	 */
+	if (!cfile->oplock_break_cancelled) {
+		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
+							     cinode);
+		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
+	}
+oplock_break_done:
 	_cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
 	cifs_done_oplock_break(cinode);
 }
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index cdb1ec1461de..f235a8d42e37 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -723,20 +723,32 @@ void
 cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
 {
 	struct cifsFileInfo *cfile = NULL;
+	struct file_list *tmp_list;
+	struct list_head file_head;
 
 	if (cifs_inode == NULL)
 		return;
 
+	INIT_LIST_HEAD(&file_head);
+	spin_lock(&cifs_inode->open_file_lock);
 	list_for_each_entry(cfile, &cifs_inode->openFileList, flist) {
 		if (delayed_work_pending(&cfile->deferred)) {
-			/*
-			 * If there is no pending work, mod_delayed_work queues new work.
-			 * So, Increase the ref count to avoid use-after-free.
-			 */
-			if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-				cifsFileInfo_get(cfile);
+			if (cancel_delayed_work(&cfile->deferred)) {
+				tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+				if (tmp_list == NULL)
+					continue;
+				tmp_list->cfile = cfile;
+				list_add_tail(&tmp_list->list, &file_head);
+			}
 		}
 	}
+	spin_unlock(&cifs_inode->open_file_lock);
+
+	list_for_each_entry(tmp_list, &file_head, list) {
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
+		list_del(&tmp_list->list);
+		kfree(tmp_list);
+	}
 }
 
 void
@@ -744,20 +756,30 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
 {
 	struct cifsFileInfo *cfile;
 	struct list_head *tmp;
+	struct file_list *tmp_list;
+	struct list_head file_head;
 
+	INIT_LIST_HEAD(&file_head);
 	spin_lock(&tcon->open_file_lock);
 	list_for_each(tmp, &tcon->openFileList) {
 		cfile = list_entry(tmp, struct cifsFileInfo, tlist);
 		if (delayed_work_pending(&cfile->deferred)) {
-			/*
-			 * If there is no pending work, mod_delayed_work queues new work.
-			 * So, Increase the ref count to avoid use-after-free.
-			 */
-			if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-				cifsFileInfo_get(cfile);
+			if (cancel_delayed_work(&cfile->deferred)) {
+				tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+				if (tmp_list == NULL)
+					continue;
+				tmp_list->cfile = cfile;
+				list_add_tail(&tmp_list->list, &file_head);
+			}
 		}
 	}
 	spin_unlock(&tcon->open_file_lock);
+
+	list_for_each_entry(tmp_list, &file_head, list) {
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
+		list_del(&tmp_list->list);
+		kfree(tmp_list);
+	}
 }
 
 /* parses DFS refferal V3 structure
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-10 17:29   ` Rohith Surabattula
@ 2021-08-10 19:28     ` Steve French
  2021-08-10 19:31       ` Steve French
  0 siblings, 1 reply; 8+ messages in thread
From: Steve French @ 2021-08-10 19:28 UTC (permalink / raw)
  To: Rohith Surabattula; +Cc: Shyam Prasad N, linux-cifs, ronnie sahlberg

I saw some test failures in

and looking in /var/log/messages I see e.g.

Aug 10 14:00:22 fedora29 systemd[1]: Started /usr/bin/bash -c exit 77.
Aug 10 14:00:25 fedora29 kernel: kmemleak: 10 new suspected memory
leaks (see /sys/kernel/debug/kmemleak)
Aug 10 14:00:28 fedora29 kernel: kmemleak: 1 new suspected memory
leaks (see /sys/kernel/debug/kmemleak)
Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
\\win16.vm.test\Scratch
Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
\\win16.vm.test\Scratch
Aug 10 14:00:29 fedora29 root[6460]: run xfstest cifs/100
Aug 10 14:00:29 fedora29 journal: run fstests cifs/100 at 2021-08-10 14:00:29
Aug 10 14:00:29 fedora29 systemd[1]: Started /usr/bin/bash -c test -w
/proc/self/oom_score_adj && echo 250 > /proc/self/oom_score_adj; exec
./tests/cifs/100.
Aug 10 14:00:30 fedora29 kernel: general protection fault, probably
for non-canonical address 0xdead000000000110: 0000 [#4] SMP PTI
Aug 10 14:00:30 fedora29 kernel: CPU: 0 PID: 6624 Comm: rm Tainted: G
    D           5.14.0-rc4 #1
Aug 10 14:00:30 fedora29 kernel: Hardware name: Red Hat KVM, BIOS
0.5.1 01/01/2011
Aug 10 14:00:30 fedora29 kernel: RIP:
0010:cifs_close_deferred_file+0x77/0x140 [cifs]
Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
fe ff 48 89 df e8 61 46
Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb121806dbdd0 EFLAGS: 00010287
Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc55b8ca01 RBX:
dead000000000100 RCX: 00000000000034d9
Aug 10 14:00:30 fedora29 kernel: RDX: 00000000000034d8 RSI:
67739b6f43694caa RDI: 0000000000032080

and

Aug 10 14:00:30 fedora29 kernel: ---[ end trace b062b12a095f4dd5 ]---
Aug 10 14:00:30 fedora29 kernel: RIP:
0010:cifs_close_deferred_file+0x77/0x140 [cifs]
Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
fe ff 48 89 df e8 61 46
Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb12180f0fc80 EFLAGS: 00010283
Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc5b2ca201 RBX:
dead000000000100 RCX: 0000000000002367
Aug 10 14:00:30 fedora29 kernel: RDX: 0000000000002366 RSI:
67100f614dfd246a RDI: 0000000000032080
Aug 10 14:00:30 fedora29 kernel: RBP: dead000000000122 R08:
0000000000000001 R09: 0000000000000001
Aug 10 14:00:30 fedora29 kernel: R10: ffffb12180f0fba0 R11:
0000000000000001 R12: ffffb12180f0fc80

On Tue, Aug 10, 2021 at 12:29 PM Rohith Surabattula
<rohiths.msft@gmail.com> wrote:
>
> Hi All,
>
> Have updated the patch to fix the memleak.
>
> Regards,
> Rohith
>
> On Tue, Aug 10, 2021 at 5:18 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> >
> > Already gave my review comments along with this one to the other email.
> >
> > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> >
> > On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
> > <rohiths.msft@gmail.com> wrote:
> > >
> > > Hi Steve/All,
> > >
> > > During unlink/rename/lease break, deferred work for close is
> > > scheduled immediately but in asynchronous manner which might
> > > lead to race with actual(unlink/rename) commands.
> > >
> > > This change will schedule close synchronously which will avoid
> > > the race conditions with other commands.
> > >
> > > Regards,
> > > Rohith
> >
> >
> >
> > --
> > Regards,
> > Shyam



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-10 19:28     ` Steve French
@ 2021-08-10 19:31       ` Steve French
  2021-08-10 19:48         ` Rohith Surabattula
  0 siblings, 1 reply; 8+ messages in thread
From: Steve French @ 2021-08-10 19:31 UTC (permalink / raw)
  To: Rohith Surabattula; +Cc: Shyam Prasad N, linux-cifs, ronnie sahlberg

This is in http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/766

On Tue, Aug 10, 2021 at 2:28 PM Steve French <smfrench@gmail.com> wrote:
>
> I saw some test failures in
>
> and looking in /var/log/messages I see e.g.
>
> Aug 10 14:00:22 fedora29 systemd[1]: Started /usr/bin/bash -c exit 77.
> Aug 10 14:00:25 fedora29 kernel: kmemleak: 10 new suspected memory
> leaks (see /sys/kernel/debug/kmemleak)
> Aug 10 14:00:28 fedora29 kernel: kmemleak: 1 new suspected memory
> leaks (see /sys/kernel/debug/kmemleak)
> Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> \\win16.vm.test\Scratch
> Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> \\win16.vm.test\Scratch
> Aug 10 14:00:29 fedora29 root[6460]: run xfstest cifs/100
> Aug 10 14:00:29 fedora29 journal: run fstests cifs/100 at 2021-08-10 14:00:29
> Aug 10 14:00:29 fedora29 systemd[1]: Started /usr/bin/bash -c test -w
> /proc/self/oom_score_adj && echo 250 > /proc/self/oom_score_adj; exec
> ./tests/cifs/100.
> Aug 10 14:00:30 fedora29 kernel: general protection fault, probably
> for non-canonical address 0xdead000000000110: 0000 [#4] SMP PTI
> Aug 10 14:00:30 fedora29 kernel: CPU: 0 PID: 6624 Comm: rm Tainted: G
>     D           5.14.0-rc4 #1
> Aug 10 14:00:30 fedora29 kernel: Hardware name: Red Hat KVM, BIOS
> 0.5.1 01/01/2011
> Aug 10 14:00:30 fedora29 kernel: RIP:
> 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> fe ff 48 89 df e8 61 46
> Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb121806dbdd0 EFLAGS: 00010287
> Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc55b8ca01 RBX:
> dead000000000100 RCX: 00000000000034d9
> Aug 10 14:00:30 fedora29 kernel: RDX: 00000000000034d8 RSI:
> 67739b6f43694caa RDI: 0000000000032080
>
> and
>
> Aug 10 14:00:30 fedora29 kernel: ---[ end trace b062b12a095f4dd5 ]---
> Aug 10 14:00:30 fedora29 kernel: RIP:
> 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> fe ff 48 89 df e8 61 46
> Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb12180f0fc80 EFLAGS: 00010283
> Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc5b2ca201 RBX:
> dead000000000100 RCX: 0000000000002367
> Aug 10 14:00:30 fedora29 kernel: RDX: 0000000000002366 RSI:
> 67100f614dfd246a RDI: 0000000000032080
> Aug 10 14:00:30 fedora29 kernel: RBP: dead000000000122 R08:
> 0000000000000001 R09: 0000000000000001
> Aug 10 14:00:30 fedora29 kernel: R10: ffffb12180f0fba0 R11:
> 0000000000000001 R12: ffffb12180f0fc80
>
> On Tue, Aug 10, 2021 at 12:29 PM Rohith Surabattula
> <rohiths.msft@gmail.com> wrote:
> >
> > Hi All,
> >
> > Have updated the patch to fix the memleak.
> >
> > Regards,
> > Rohith
> >
> > On Tue, Aug 10, 2021 at 5:18 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> > >
> > > Already gave my review comments along with this one to the other email.
> > >
> > > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> > >
> > > On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
> > > <rohiths.msft@gmail.com> wrote:
> > > >
> > > > Hi Steve/All,
> > > >
> > > > During unlink/rename/lease break, deferred work for close is
> > > > scheduled immediately but in asynchronous manner which might
> > > > lead to race with actual(unlink/rename) commands.
> > > >
> > > > This change will schedule close synchronously which will avoid
> > > > the race conditions with other commands.
> > > >
> > > > Regards,
> > > > Rohith
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Shyam
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-10 19:31       ` Steve French
@ 2021-08-10 19:48         ` Rohith Surabattula
  2021-08-11  5:28           ` Steve French
  0 siblings, 1 reply; 8+ messages in thread
From: Rohith Surabattula @ 2021-08-10 19:48 UTC (permalink / raw)
  To: Steve French; +Cc: Shyam Prasad N, linux-cifs, ronnie sahlberg

[-- Attachment #1: Type: text/plain, Size: 4319 bytes --]

Hi All,

I have attached an old version of patch by mistake.
Have attached the correct one.

Sorry for the confusion.

Regards.
Rohith

On Wed, Aug 11, 2021 at 1:02 AM Steve French <smfrench@gmail.com> wrote:
>
> This is in http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/766
>
> On Tue, Aug 10, 2021 at 2:28 PM Steve French <smfrench@gmail.com> wrote:
> >
> > I saw some test failures in
> >
> > and looking in /var/log/messages I see e.g.
> >
> > Aug 10 14:00:22 fedora29 systemd[1]: Started /usr/bin/bash -c exit 77.
> > Aug 10 14:00:25 fedora29 kernel: kmemleak: 10 new suspected memory
> > leaks (see /sys/kernel/debug/kmemleak)
> > Aug 10 14:00:28 fedora29 kernel: kmemleak: 1 new suspected memory
> > leaks (see /sys/kernel/debug/kmemleak)
> > Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> > \\win16.vm.test\Scratch
> > Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> > \\win16.vm.test\Scratch
> > Aug 10 14:00:29 fedora29 root[6460]: run xfstest cifs/100
> > Aug 10 14:00:29 fedora29 journal: run fstests cifs/100 at 2021-08-10 14:00:29
> > Aug 10 14:00:29 fedora29 systemd[1]: Started /usr/bin/bash -c test -w
> > /proc/self/oom_score_adj && echo 250 > /proc/self/oom_score_adj; exec
> > ./tests/cifs/100.
> > Aug 10 14:00:30 fedora29 kernel: general protection fault, probably
> > for non-canonical address 0xdead000000000110: 0000 [#4] SMP PTI
> > Aug 10 14:00:30 fedora29 kernel: CPU: 0 PID: 6624 Comm: rm Tainted: G
> >     D           5.14.0-rc4 #1
> > Aug 10 14:00:30 fedora29 kernel: Hardware name: Red Hat KVM, BIOS
> > 0.5.1 01/01/2011
> > Aug 10 14:00:30 fedora29 kernel: RIP:
> > 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> > Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> > 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> > bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> > fe ff 48 89 df e8 61 46
> > Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb121806dbdd0 EFLAGS: 00010287
> > Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc55b8ca01 RBX:
> > dead000000000100 RCX: 00000000000034d9
> > Aug 10 14:00:30 fedora29 kernel: RDX: 00000000000034d8 RSI:
> > 67739b6f43694caa RDI: 0000000000032080
> >
> > and
> >
> > Aug 10 14:00:30 fedora29 kernel: ---[ end trace b062b12a095f4dd5 ]---
> > Aug 10 14:00:30 fedora29 kernel: RIP:
> > 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> > Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> > 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> > bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> > fe ff 48 89 df e8 61 46
> > Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb12180f0fc80 EFLAGS: 00010283
> > Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc5b2ca201 RBX:
> > dead000000000100 RCX: 0000000000002367
> > Aug 10 14:00:30 fedora29 kernel: RDX: 0000000000002366 RSI:
> > 67100f614dfd246a RDI: 0000000000032080
> > Aug 10 14:00:30 fedora29 kernel: RBP: dead000000000122 R08:
> > 0000000000000001 R09: 0000000000000001
> > Aug 10 14:00:30 fedora29 kernel: R10: ffffb12180f0fba0 R11:
> > 0000000000000001 R12: ffffb12180f0fc80
> >
> > On Tue, Aug 10, 2021 at 12:29 PM Rohith Surabattula
> > <rohiths.msft@gmail.com> wrote:
> > >
> > > Hi All,
> > >
> > > Have updated the patch to fix the memleak.
> > >
> > > Regards,
> > > Rohith
> > >
> > > On Tue, Aug 10, 2021 at 5:18 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> > > >
> > > > Already gave my review comments along with this one to the other email.
> > > >
> > > > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> > > >
> > > > On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
> > > > <rohiths.msft@gmail.com> wrote:
> > > > >
> > > > > Hi Steve/All,
> > > > >
> > > > > During unlink/rename/lease break, deferred work for close is
> > > > > scheduled immediately but in asynchronous manner which might
> > > > > lead to race with actual(unlink/rename) commands.
> > > > >
> > > > > This change will schedule close synchronously which will avoid
> > > > > the race conditions with other commands.
> > > > >
> > > > > Regards,
> > > > > Rohith
> > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > > Shyam
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

[-- Attachment #2: cifs-Call-close-synchronously-during-unlink-rename-l.patch --]
[-- Type: application/octet-stream, Size: 5882 bytes --]

From 22b464f8f7fff4d366e057bf5e5a51b12ce48bda Mon Sep 17 00:00:00 2001
From: Rohith Surabattula <rohiths@microsoft.com>
Date: Mon, 9 Aug 2021 09:32:46 +0000
Subject: [PATCH 2/2] cifs: Call close synchronously during unlink/rename/lease
 break.

During unlink/rename/lease break, deferred work for close is
scheduled immediately but in asynchronous manner which might
lead to race with actual(unlink/rename) commands.

This change will schedule close synchronously which will avoid
the race conditions with other commands.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
---
 fs/cifs/cifsglob.h |  5 +++++
 fs/cifs/file.c     | 35 +++++++++++++++++------------------
 fs/cifs/misc.c     | 46 ++++++++++++++++++++++++++++++++++------------
 3 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index c0bfc2f01030..c6a9542ca281 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1611,6 +1611,11 @@ struct dfs_info3_param {
 	int ttl;
 };
 
+struct file_list {
+	struct list_head list;
+	struct cifsFileInfo *cfile;
+};
+
 /*
  * common struct for holding inode info when searching for or updating an
  * inode with new info
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 0a72840a88f1..bb98fbdd22a9 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -4847,17 +4847,6 @@ void cifs_oplock_break(struct work_struct *work)
 		cifs_dbg(VFS, "Push locks rc = %d\n", rc);
 
 oplock_break_ack:
-	/*
-	 * releasing stale oplock after recent reconnect of smb session using
-	 * a now incorrect file handle is not a data integrity issue but do
-	 * not bother sending an oplock release if session to server still is
-	 * disconnected since oplock already released by the server
-	 */
-	if (!cfile->oplock_break_cancelled) {
-		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
-							     cinode);
-		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
-	}
 	/*
 	 * When oplock break is received and there are no active
 	 * file handles but cached, then schedule deferred close immediately.
@@ -4865,17 +4854,27 @@ void cifs_oplock_break(struct work_struct *work)
 	 */
 	spin_lock(&CIFS_I(inode)->deferred_lock);
 	is_deferred = cifs_is_deferred_close(cfile, &dclose);
+	spin_unlock(&CIFS_I(inode)->deferred_lock);
 	if (is_deferred &&
 	    cfile->deferred_close_scheduled &&
 	    delayed_work_pending(&cfile->deferred)) {
-		/*
-		 * If there is no pending work, mod_delayed_work queues new work.
-		 * So, Increase the ref count to avoid use-after-free.
-		 */
-		if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-			cifsFileInfo_get(cfile);
+		if (cancel_delayed_work(&cfile->deferred)) {
+			_cifsFileInfo_put(cfile, false, false);
+			goto oplock_break_done;
+		}
 	}
-	spin_unlock(&CIFS_I(inode)->deferred_lock);
+	/*
+	 * releasing stale oplock after recent reconnect of smb session using
+	 * a now incorrect file handle is not a data integrity issue but do
+	 * not bother sending an oplock release if session to server still is
+	 * disconnected since oplock already released by the server
+	 */
+	if (!cfile->oplock_break_cancelled) {
+		rc = tcon->ses->server->ops->oplock_response(tcon, &cfile->fid,
+							     cinode);
+		cifs_dbg(FYI, "Oplock release rc = %d\n", rc);
+	}
+oplock_break_done:
 	_cifsFileInfo_put(cfile, false /* do not wait for ourself */, false);
 	cifs_done_oplock_break(cinode);
 }
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index cdb1ec1461de..9469f1cf0b46 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -723,20 +723,32 @@ void
 cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
 {
 	struct cifsFileInfo *cfile = NULL;
+	struct file_list *tmp_list, *tmp_next_list;
+	struct list_head file_head;
 
 	if (cifs_inode == NULL)
 		return;
 
+	INIT_LIST_HEAD(&file_head);
+	spin_lock(&cifs_inode->open_file_lock);
 	list_for_each_entry(cfile, &cifs_inode->openFileList, flist) {
 		if (delayed_work_pending(&cfile->deferred)) {
-			/*
-			 * If there is no pending work, mod_delayed_work queues new work.
-			 * So, Increase the ref count to avoid use-after-free.
-			 */
-			if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-				cifsFileInfo_get(cfile);
+			if (cancel_delayed_work(&cfile->deferred)) {
+				tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+				if (tmp_list == NULL)
+					continue;
+				tmp_list->cfile = cfile;
+				list_add_tail(&tmp_list->list, &file_head);
+			}
 		}
 	}
+	spin_unlock(&cifs_inode->open_file_lock);
+
+	list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
+		list_del(&tmp_list->list);
+		kfree(tmp_list);
+	}
 }
 
 void
@@ -744,20 +756,30 @@ cifs_close_all_deferred_files(struct cifs_tcon *tcon)
 {
 	struct cifsFileInfo *cfile;
 	struct list_head *tmp;
+	struct file_list *tmp_list, *tmp_next_list;
+	struct list_head file_head;
 
+	INIT_LIST_HEAD(&file_head);
 	spin_lock(&tcon->open_file_lock);
 	list_for_each(tmp, &tcon->openFileList) {
 		cfile = list_entry(tmp, struct cifsFileInfo, tlist);
 		if (delayed_work_pending(&cfile->deferred)) {
-			/*
-			 * If there is no pending work, mod_delayed_work queues new work.
-			 * So, Increase the ref count to avoid use-after-free.
-			 */
-			if (!mod_delayed_work(deferredclose_wq, &cfile->deferred, 0))
-				cifsFileInfo_get(cfile);
+			if (cancel_delayed_work(&cfile->deferred)) {
+				tmp_list = kmalloc(sizeof(struct file_list), GFP_ATOMIC);
+				if (tmp_list == NULL)
+					continue;
+				tmp_list->cfile = cfile;
+				list_add_tail(&tmp_list->list, &file_head);
+			}
 		}
 	}
 	spin_unlock(&tcon->open_file_lock);
+
+	list_for_each_entry_safe(tmp_list, tmp_next_list, &file_head, list) {
+		_cifsFileInfo_put(tmp_list->cfile, true, false);
+		list_del(&tmp_list->list);
+		kfree(tmp_list);
+	}
 }
 
 /* parses DFS refferal V3 structure
-- 
2.33.0.rc1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-10 19:48         ` Rohith Surabattula
@ 2021-08-11  5:28           ` Steve French
  2021-08-11  5:35             ` Rohith Surabattula
  0 siblings, 1 reply; 8+ messages in thread
From: Steve French @ 2021-08-11  5:28 UTC (permalink / raw)
  To: Rohith Surabattula; +Cc: Shyam Prasad N, linux-cifs, ronnie sahlberg

I added

Cc: stable@vger.kernel.org # 5.13

Sound correct?

On Tue, Aug 10, 2021 at 2:48 PM Rohith Surabattula
<rohiths.msft@gmail.com> wrote:
>
> Hi All,
>
> I have attached an old version of patch by mistake.
> Have attached the correct one.
>
> Sorry for the confusion.
>
> Regards.
> Rohith
>
> On Wed, Aug 11, 2021 at 1:02 AM Steve French <smfrench@gmail.com> wrote:
> >
> > This is in http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/766
> >
> > On Tue, Aug 10, 2021 at 2:28 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > I saw some test failures in
> > >
> > > and looking in /var/log/messages I see e.g.
> > >
> > > Aug 10 14:00:22 fedora29 systemd[1]: Started /usr/bin/bash -c exit 77.
> > > Aug 10 14:00:25 fedora29 kernel: kmemleak: 10 new suspected memory
> > > leaks (see /sys/kernel/debug/kmemleak)
> > > Aug 10 14:00:28 fedora29 kernel: kmemleak: 1 new suspected memory
> > > leaks (see /sys/kernel/debug/kmemleak)
> > > Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> > > \\win16.vm.test\Scratch
> > > Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> > > \\win16.vm.test\Scratch
> > > Aug 10 14:00:29 fedora29 root[6460]: run xfstest cifs/100
> > > Aug 10 14:00:29 fedora29 journal: run fstests cifs/100 at 2021-08-10 14:00:29
> > > Aug 10 14:00:29 fedora29 systemd[1]: Started /usr/bin/bash -c test -w
> > > /proc/self/oom_score_adj && echo 250 > /proc/self/oom_score_adj; exec
> > > ./tests/cifs/100.
> > > Aug 10 14:00:30 fedora29 kernel: general protection fault, probably
> > > for non-canonical address 0xdead000000000110: 0000 [#4] SMP PTI
> > > Aug 10 14:00:30 fedora29 kernel: CPU: 0 PID: 6624 Comm: rm Tainted: G
> > >     D           5.14.0-rc4 #1
> > > Aug 10 14:00:30 fedora29 kernel: Hardware name: Red Hat KVM, BIOS
> > > 0.5.1 01/01/2011
> > > Aug 10 14:00:30 fedora29 kernel: RIP:
> > > 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> > > Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> > > 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> > > bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> > > fe ff 48 89 df e8 61 46
> > > Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb121806dbdd0 EFLAGS: 00010287
> > > Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc55b8ca01 RBX:
> > > dead000000000100 RCX: 00000000000034d9
> > > Aug 10 14:00:30 fedora29 kernel: RDX: 00000000000034d8 RSI:
> > > 67739b6f43694caa RDI: 0000000000032080
> > >
> > > and
> > >
> > > Aug 10 14:00:30 fedora29 kernel: ---[ end trace b062b12a095f4dd5 ]---
> > > Aug 10 14:00:30 fedora29 kernel: RIP:
> > > 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> > > Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> > > 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> > > bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> > > fe ff 48 89 df e8 61 46
> > > Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb12180f0fc80 EFLAGS: 00010283
> > > Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc5b2ca201 RBX:
> > > dead000000000100 RCX: 0000000000002367
> > > Aug 10 14:00:30 fedora29 kernel: RDX: 0000000000002366 RSI:
> > > 67100f614dfd246a RDI: 0000000000032080
> > > Aug 10 14:00:30 fedora29 kernel: RBP: dead000000000122 R08:
> > > 0000000000000001 R09: 0000000000000001
> > > Aug 10 14:00:30 fedora29 kernel: R10: ffffb12180f0fba0 R11:
> > > 0000000000000001 R12: ffffb12180f0fc80
> > >
> > > On Tue, Aug 10, 2021 at 12:29 PM Rohith Surabattula
> > > <rohiths.msft@gmail.com> wrote:
> > > >
> > > > Hi All,
> > > >
> > > > Have updated the patch to fix the memleak.
> > > >
> > > > Regards,
> > > > Rohith
> > > >
> > > > On Tue, Aug 10, 2021 at 5:18 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> > > > >
> > > > > Already gave my review comments along with this one to the other email.
> > > > >
> > > > > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > >
> > > > > On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
> > > > > <rohiths.msft@gmail.com> wrote:
> > > > > >
> > > > > > Hi Steve/All,
> > > > > >
> > > > > > During unlink/rename/lease break, deferred work for close is
> > > > > > scheduled immediately but in asynchronous manner which might
> > > > > > lead to race with actual(unlink/rename) commands.
> > > > > >
> > > > > > This change will schedule close synchronously which will avoid
> > > > > > the race conditions with other commands.
> > > > > >
> > > > > > Regards,
> > > > > > Rohith
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards,
> > > > > Shyam
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH][CIFS]Call close synchronously during unlink/rename/lease break.
  2021-08-11  5:28           ` Steve French
@ 2021-08-11  5:35             ` Rohith Surabattula
  0 siblings, 0 replies; 8+ messages in thread
From: Rohith Surabattula @ 2021-08-11  5:35 UTC (permalink / raw)
  To: Steve French; +Cc: Shyam Prasad N, linux-cifs, ronnie sahlberg

Yes, Sounds good.

Regards,
Rohith

On Wed, Aug 11, 2021 at 10:58 AM Steve French <smfrench@gmail.com> wrote:
>
> I added
>
> Cc: stable@vger.kernel.org # 5.13
>
> Sound correct?
>
> On Tue, Aug 10, 2021 at 2:48 PM Rohith Surabattula
> <rohiths.msft@gmail.com> wrote:
> >
> > Hi All,
> >
> > I have attached an old version of patch by mistake.
> > Have attached the correct one.
> >
> > Sorry for the confusion.
> >
> > Regards.
> > Rohith
> >
> > On Wed, Aug 11, 2021 at 1:02 AM Steve French <smfrench@gmail.com> wrote:
> > >
> > > This is in http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/766
> > >
> > > On Tue, Aug 10, 2021 at 2:28 PM Steve French <smfrench@gmail.com> wrote:
> > > >
> > > > I saw some test failures in
> > > >
> > > > and looking in /var/log/messages I see e.g.
> > > >
> > > > Aug 10 14:00:22 fedora29 systemd[1]: Started /usr/bin/bash -c exit 77.
> > > > Aug 10 14:00:25 fedora29 kernel: kmemleak: 10 new suspected memory
> > > > leaks (see /sys/kernel/debug/kmemleak)
> > > > Aug 10 14:00:28 fedora29 kernel: kmemleak: 1 new suspected memory
> > > > leaks (see /sys/kernel/debug/kmemleak)
> > > > Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> > > > \\win16.vm.test\Scratch
> > > > Aug 10 14:00:28 fedora29 kernel: CIFS: Attempting to mount
> > > > \\win16.vm.test\Scratch
> > > > Aug 10 14:00:29 fedora29 root[6460]: run xfstest cifs/100
> > > > Aug 10 14:00:29 fedora29 journal: run fstests cifs/100 at 2021-08-10 14:00:29
> > > > Aug 10 14:00:29 fedora29 systemd[1]: Started /usr/bin/bash -c test -w
> > > > /proc/self/oom_score_adj && echo 250 > /proc/self/oom_score_adj; exec
> > > > ./tests/cifs/100.
> > > > Aug 10 14:00:30 fedora29 kernel: general protection fault, probably
> > > > for non-canonical address 0xdead000000000110: 0000 [#4] SMP PTI
> > > > Aug 10 14:00:30 fedora29 kernel: CPU: 0 PID: 6624 Comm: rm Tainted: G
> > > >     D           5.14.0-rc4 #1
> > > > Aug 10 14:00:30 fedora29 kernel: Hardware name: Red Hat KVM, BIOS
> > > > 0.5.1 01/01/2011
> > > > Aug 10 14:00:30 fedora29 kernel: RIP:
> > > > 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> > > > Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> > > > 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> > > > bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> > > > fe ff 48 89 df e8 61 46
> > > > Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb121806dbdd0 EFLAGS: 00010287
> > > > Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc55b8ca01 RBX:
> > > > dead000000000100 RCX: 00000000000034d9
> > > > Aug 10 14:00:30 fedora29 kernel: RDX: 00000000000034d8 RSI:
> > > > 67739b6f43694caa RDI: 0000000000032080
> > > >
> > > > and
> > > >
> > > > Aug 10 14:00:30 fedora29 kernel: ---[ end trace b062b12a095f4dd5 ]---
> > > > Aug 10 14:00:30 fedora29 kernel: RIP:
> > > > 0010:cifs_close_deferred_file+0x77/0x140 [cifs]
> > > > Aug 10 14:00:30 fedora29 kernel: Code: 85 de 00 00 00 4c 89 ef e8 06
> > > > 96 2e e8 48 8b 1c 24 4c 39 e3 74 55 49 bd 00 01 00 00 00 00 ad de 48
> > > > bd 22 01 00 00 00 00 ad de <48> 8b 7b 10 31 d2 be 01 00 00 00 e8 a9 c5
> > > > fe ff 48 89 df e8 61 46
> > > > Aug 10 14:00:30 fedora29 kernel: RSP: 0018:ffffb12180f0fc80 EFLAGS: 00010283
> > > > Aug 10 14:00:30 fedora29 kernel: RAX: ffff88dc5b2ca201 RBX:
> > > > dead000000000100 RCX: 0000000000002367
> > > > Aug 10 14:00:30 fedora29 kernel: RDX: 0000000000002366 RSI:
> > > > 67100f614dfd246a RDI: 0000000000032080
> > > > Aug 10 14:00:30 fedora29 kernel: RBP: dead000000000122 R08:
> > > > 0000000000000001 R09: 0000000000000001
> > > > Aug 10 14:00:30 fedora29 kernel: R10: ffffb12180f0fba0 R11:
> > > > 0000000000000001 R12: ffffb12180f0fc80
> > > >
> > > > On Tue, Aug 10, 2021 at 12:29 PM Rohith Surabattula
> > > > <rohiths.msft@gmail.com> wrote:
> > > > >
> > > > > Hi All,
> > > > >
> > > > > Have updated the patch to fix the memleak.
> > > > >
> > > > > Regards,
> > > > > Rohith
> > > > >
> > > > > On Tue, Aug 10, 2021 at 5:18 PM Shyam Prasad N <nspmangalore@gmail.com> wrote:
> > > > > >
> > > > > > Already gave my review comments along with this one to the other email.
> > > > > >
> > > > > > Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>
> > > > > >
> > > > > > On Mon, Aug 9, 2021 at 3:28 PM Rohith Surabattula
> > > > > > <rohiths.msft@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Steve/All,
> > > > > > >
> > > > > > > During unlink/rename/lease break, deferred work for close is
> > > > > > > scheduled immediately but in asynchronous manner which might
> > > > > > > lead to race with actual(unlink/rename) commands.
> > > > > > >
> > > > > > > This change will schedule close synchronously which will avoid
> > > > > > > the race conditions with other commands.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Rohith
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Regards,
> > > > > > Shyam
> > > >
> > > >
> > > >
> > > > --
> > > > Thanks,
> > > >
> > > > Steve
> > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
>
>
>
> --
> Thanks,
>
> Steve

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-08-11  5:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09  9:58 [PATCH][CIFS]Call close synchronously during unlink/rename/lease break Rohith Surabattula
2021-08-10 11:47 ` Shyam Prasad N
2021-08-10 17:29   ` Rohith Surabattula
2021-08-10 19:28     ` Steve French
2021-08-10 19:31       ` Steve French
2021-08-10 19:48         ` Rohith Surabattula
2021-08-11  5:28           ` Steve French
2021-08-11  5:35             ` Rohith Surabattula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).