linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Fwd: [PATCH][CIFS] Handle race conditions during rename
       [not found] <CAH2r5ms3XvZSLbAOb7irrZpS0aFsDUW59qRDdN19H8ZaR_YX+w@mail.gmail.com>
@ 2021-08-04 17:44 ` Steve French
  2021-08-09  9:56 ` Rohith Surabattula
  1 sibling, 0 replies; 3+ messages in thread
From: Steve French @ 2021-08-04 17:44 UTC (permalink / raw)
  To: CIFS; +Cc: rohiths msft

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

Lightly updated version of Rohith's newer version of his deferred
close fix (fixed minor checkpatch issue and added cc:stable #5.13

-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-Handle-race-conditions-during-rename.patch --]
[-- Type: application/x-patch, Size: 3077 bytes --]

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

* Re: [PATCH][CIFS] Handle race conditions during rename
       [not found] <CAH2r5ms3XvZSLbAOb7irrZpS0aFsDUW59qRDdN19H8ZaR_YX+w@mail.gmail.com>
  2021-08-04 17:44 ` Fwd: [PATCH][CIFS] Handle race conditions during rename Steve French
@ 2021-08-09  9:56 ` Rohith Surabattula
  2021-08-10 11:46   ` Shyam Prasad N
  1 sibling, 1 reply; 3+ messages in thread
From: Rohith Surabattula @ 2021-08-09  9:56 UTC (permalink / raw)
  To: Steve French; +Cc: CIFS

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

Hi Steve, All,

Have updated the patch slightly with null check to avoid kernel oops.

Regards,
Rohith

On Wed, Aug 4, 2021 at 11:13 PM Steve French <smfrench@gmail.com> wrote:
>
> Lightly updated version of Rohith's newer version of his deferred close fix (fixed minor checkpatch issue and added cc:stable #5.13
>
> --
> Thanks,
>
> Steve

[-- Attachment #2: cifs-Handle-race-conditions-during-rename.patch --]
[-- Type: application/octet-stream, Size: 3109 bytes --]

From d89960e39622dc1112bcb7cf3fded8063f3ef7fc Mon Sep 17 00:00:00 2001
From: Rohith Surabattula <rohiths@microsoft.com>
Date: Thu, 29 Jul 2021 07:45:29 +0000
Subject: [PATCH] cifs: Handle race conditions during rename

When rename is executed on directory which has files for which
close is deferred, then rename will fail with EACCES.

This patch will try to close all deferred files when EACCES is received
and retry rename on a directory.

Signed-off-by: Rohith Surabattula <rohiths@microsoft.com>
Cc: stable@vger.kernel.org # 5.13
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/inode.c | 19 +++++++++++++++++--
 fs/cifs/misc.c  | 16 +++++++++++-----
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index b96b253e7635..65f8a70cece3 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -1625,7 +1625,7 @@ int cifs_unlink(struct inode *dir, struct dentry *dentry)
 		goto unlink_out;
 	}
 
-	cifs_close_all_deferred_files(tcon);
+	cifs_close_deferred_file(CIFS_I(inode));
 	if (cap_unix(tcon->ses) && (CIFS_UNIX_POSIX_PATH_OPS_CAP &
 				le64_to_cpu(tcon->fsUnixInfo.Capability))) {
 		rc = CIFSPOSIXDelFile(xid, tcon, full_path,
@@ -2084,6 +2084,7 @@ cifs_rename2(struct user_namespace *mnt_userns, struct inode *source_dir,
 	FILE_UNIX_BASIC_INFO *info_buf_target;
 	unsigned int xid;
 	int rc, tmprc;
+	int retry_count = 0;
 
 	if (flags & ~RENAME_NOREPLACE)
 		return -EINVAL;
@@ -2113,10 +2114,24 @@ cifs_rename2(struct user_namespace *mnt_userns, struct inode *source_dir,
 		goto cifs_rename_exit;
 	}
 
-	cifs_close_all_deferred_files(tcon);
+	cifs_close_deferred_file(CIFS_I(d_inode(source_dentry)));
+	if (d_inode(target_dentry) != NULL)
+		cifs_close_deferred_file(CIFS_I(d_inode(target_dentry)));
+
 	rc = cifs_do_rename(xid, source_dentry, from_name, target_dentry,
 			    to_name);
 
+	if (rc == -EACCES) {
+		while (retry_count < 3) {
+			cifs_close_all_deferred_files(tcon);
+			rc = cifs_do_rename(xid, source_dentry, from_name, target_dentry,
+					    to_name);
+			if (rc != -EACCES)
+				break;
+			retry_count++;
+		}
+	}
+
 	/*
 	 * No-replace is the natural behavior for CIFS, so skip unlink hacks.
 	 */
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 844abeb2b48f..cdb1ec1461de 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -723,13 +723,19 @@ void
 cifs_close_deferred_file(struct cifsInodeInfo *cifs_inode)
 {
 	struct cifsFileInfo *cfile = NULL;
-	struct cifs_deferred_close *dclose;
+
+	if (cifs_inode == NULL)
+		return;
 
 	list_for_each_entry(cfile, &cifs_inode->openFileList, flist) {
-		spin_lock(&cifs_inode->deferred_lock);
-		if (cifs_is_deferred_close(cfile, &dclose))
-			mod_delayed_work(deferredclose_wq, &cfile->deferred, 0);
-		spin_unlock(&cifs_inode->deferred_lock);
+		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);
+		}
 	}
 }
 
-- 
2.30.2


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

* Re: [PATCH][CIFS] Handle race conditions during rename
  2021-08-09  9:56 ` Rohith Surabattula
@ 2021-08-10 11:46   ` Shyam Prasad N
  0 siblings, 0 replies; 3+ messages in thread
From: Shyam Prasad N @ 2021-08-10 11:46 UTC (permalink / raw)
  To: Rohith Surabattula; +Cc: Steve French, CIFS

I see a memory leak of dclose in cifs_close. I hope that's the one you
were referring to earlier?
Also, we'll need to avoid cifs_close_all_deferred_files in the future.
Maybe not for this patch.

Other than that, looks good to me.
Reviewed-by: Shyam Prasad N <sprasad@microsoft.com>

On Mon, Aug 9, 2021 at 3:54 PM Rohith Surabattula
<rohiths.msft@gmail.com> wrote:
>
> Hi Steve, All,
>
> Have updated the patch slightly with null check to avoid kernel oops.
>
> Regards,
> Rohith
>
> On Wed, Aug 4, 2021 at 11:13 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Lightly updated version of Rohith's newer version of his deferred close fix (fixed minor checkpatch issue and added cc:stable #5.13
> >
> > --
> > Thanks,
> >
> > Steve



-- 
Regards,
Shyam

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

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

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CAH2r5ms3XvZSLbAOb7irrZpS0aFsDUW59qRDdN19H8ZaR_YX+w@mail.gmail.com>
2021-08-04 17:44 ` Fwd: [PATCH][CIFS] Handle race conditions during rename Steve French
2021-08-09  9:56 ` Rohith Surabattula
2021-08-10 11:46   ` Shyam Prasad N

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