* [PATCH v1 4.20.y stable] CIFS: fix deadlock in cached root handling
[not found] <CAKywueScmnouPGgaEkpH3pfXCzSnddz-qCZqYPjBQLGX5c__ng@mail.gmail.com>
@ 2019-07-06 19:17 ` Aurelien Aptel
2019-07-06 19:30 ` Pavel Shilovsky
0 siblings, 1 reply; 3+ messages in thread
From: Aurelien Aptel @ 2019-07-06 19:17 UTC (permalink / raw)
To: linux-cifs; +Cc: piastryyy, Aurelien Aptel
Prevent deadlock between open_shroot() and
cifs_mark_open_files_invalid() by releasing the lock before entering
SMB2_open, taking it again after and checking if we still need to use
the result.
Link: https://lore.kernel.org/linux-cifs/684ed01c-cbca-2716-bc28-b0a59a0f8521@prodrive-technologies.com/T/#u
Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
This patch applies on top of the 4.20 stable tree.
Please review this carefuly, I have not tested it.
XXX: do we need to call SMB2_close() when the work was already done
concurrently? if yes we need to do it outside the critical
section otherwise we hit the same issue again
fs/cifs/smb2ops.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index aa71e620f3cd..55991e43d74f 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -609,7 +609,27 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
oparams.fid = pfid;
oparams.reconnect = false;
+ /*
+ * We do not hold the lock for the open because in case
+ * SMB2_open needs to reconnect, it will end up calling
+ * cifs_mark_open_files_invalid() which takes the lock again
+ * thus causing a deadlock
+ */
+ mutex_unlock(&tcon->crfid.fid_mutex);
rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
+ mutex_lock(&tcon->crfid.fid_mutex);
+
+ /*
+ * Now we need to check again as the cached root might have
+ * been successfully re-opened from a concurrent process
+ */
+
+ if (tcon->crfid.is_valid) {
+ /* work was already done */
+ rc = 0;
+ goto out;
+ }
+
if (rc == 0) {
memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
tcon->crfid.tcon = tcon;
@@ -617,6 +637,8 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
kref_init(&tcon->crfid.refcount);
kref_get(&tcon->crfid.refcount);
}
+
+out:
mutex_unlock(&tcon->crfid.fid_mutex);
return rc;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1 4.20.y stable] CIFS: fix deadlock in cached root handling
2019-07-06 19:17 ` [PATCH v1 4.20.y stable] CIFS: fix deadlock in cached root handling Aurelien Aptel
@ 2019-07-06 19:30 ` Pavel Shilovsky
2019-07-06 20:11 ` [PATCH v2 " Aurelien Aptel
0 siblings, 1 reply; 3+ messages in thread
From: Pavel Shilovsky @ 2019-07-06 19:30 UTC (permalink / raw)
To: Aurelien Aptel; +Cc: linux-cifs
сб, 6 июл. 2019 г. в 12:17, Aurelien Aptel <aaptel@suse.com>:
>
> Prevent deadlock between open_shroot() and
> cifs_mark_open_files_invalid() by releasing the lock before entering
> SMB2_open, taking it again after and checking if we still need to use
> the result.
>
> Link: https://lore.kernel.org/linux-cifs/684ed01c-cbca-2716-bc28-b0a59a0f8521@prodrive-technologies.com/T/#u
> Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
> Signed-off-by: Aurelien Aptel <aaptel@suse.com>
> ---
>
> This patch applies on top of the 4.20 stable tree.
>
> Please review this carefuly, I have not tested it.
>
> XXX: do we need to call SMB2_close() when the work was already done
> concurrently? if yes we need to do it outside the critical
> section otherwise we hit the same issue again
Yes, we do need to close a handle we just opened. Otherwise it will
leak a handle on the server.
>
> fs/cifs/smb2ops.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index aa71e620f3cd..55991e43d74f 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -609,7 +609,27 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> oparams.fid = pfid;
> oparams.reconnect = false;
>
> + /*
> + * We do not hold the lock for the open because in case
> + * SMB2_open needs to reconnect, it will end up calling
> + * cifs_mark_open_files_invalid() which takes the lock again
> + * thus causing a deadlock
> + */
> + mutex_unlock(&tcon->crfid.fid_mutex);
> rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
> + mutex_lock(&tcon->crfid.fid_mutex);
> +
> + /*
> + * Now we need to check again as the cached root might have
> + * been successfully re-opened from a concurrent process
> + */
> +
> + if (tcon->crfid.is_valid) {
> + /* work was already done */
> + rc = 0;
> + goto out;
> + }
> +
> if (rc == 0) {
> memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
> tcon->crfid.tcon = tcon;
> @@ -617,6 +637,8 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
> kref_init(&tcon->crfid.refcount);
> kref_get(&tcon->crfid.refcount);
> }
> +
> +out:
> mutex_unlock(&tcon->crfid.fid_mutex);
> return rc;
> }
> --
> 2.16.4
>
--
Best regards,
Pavel Shilovsky
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v2 4.20.y stable] CIFS: fix deadlock in cached root handling
2019-07-06 19:30 ` Pavel Shilovsky
@ 2019-07-06 20:11 ` Aurelien Aptel
0 siblings, 0 replies; 3+ messages in thread
From: Aurelien Aptel @ 2019-07-06 20:11 UTC (permalink / raw)
To: linux-cifs; +Cc: piastryyy, Aurelien Aptel
Prevent deadlock between open_shroot() and
cifs_mark_open_files_invalid() by releasing the lock before entering
SMB2_open, taking it again after and checking if we still need to use
the result.
Link: https://lore.kernel.org/linux-cifs/684ed01c-cbca-2716-bc28-b0a59a0f8521@prodrive-technologies.com/T/#u
Fixes: 3d4ef9a15343 ("smb3: fix redundant opens on root")
Signed-off-by: Aurelien Aptel <aaptel@suse.com>
---
Changes since v1:
* added SMB2_close() of extra root handle
* copied valid handle to caller's pfid
* added reference get() of the handle
Please review carefuly, I haven't tested this.
fs/cifs/smb2ops.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index aa71e620f3cd..53e14fd74abc 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -609,7 +609,49 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
oparams.fid = pfid;
oparams.reconnect = false;
+ /*
+ * We do not hold the lock for the open because in case
+ * SMB2_open needs to reconnect, it will end up calling
+ * cifs_mark_open_files_invalid() which takes the lock again
+ * thus causing a deadlock
+ */
+ mutex_unlock(&tcon->crfid.fid_mutex);
rc = SMB2_open(xid, &oparams, &srch_path, &oplock, NULL, NULL, NULL);
+ mutex_lock(&tcon->crfid.fid_mutex);
+
+ /*
+ * Now we need to check again as the cached root might have
+ * been successfully re-opened from a concurrent process
+ */
+
+ if (tcon->crfid.is_valid) {
+ /* work was already done */
+
+ /* stash fids for close() later */
+ struct cifs_fid fid = {
+ .persistent_fid = pfid->persistent_fid,
+ .volatile_fid = pfid->volatile_fid,
+ };
+
+ /*
+ * caller expects this func to set pfid to a valid
+ * cached root, so we copy the existing one and get a
+ * reference.
+ */
+ memcpy(pfid, tcon->crfid.fid, sizeof(*pfid));
+ kref_get(&tcon->crfid.refcount);
+
+ mutex_unlock(&tcon->crfid.fid_mutex);
+
+ if (rc == 0) {
+ /* close extra handle outside of crit sec */
+ SMB2_close(xid, tcon, fid.persistent_fid, fid.volatile_fid);
+ }
+ return 0;
+ }
+
+ /* Cached root is still invalid, continue normaly */
+
if (rc == 0) {
memcpy(tcon->crfid.fid, pfid, sizeof(struct cifs_fid));
tcon->crfid.tcon = tcon;
@@ -617,6 +659,7 @@ int open_shroot(unsigned int xid, struct cifs_tcon *tcon, struct cifs_fid *pfid)
kref_init(&tcon->crfid.refcount);
kref_get(&tcon->crfid.refcount);
}
+
mutex_unlock(&tcon->crfid.fid_mutex);
return rc;
}
--
2.16.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-07-06 20:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAKywueScmnouPGgaEkpH3pfXCzSnddz-qCZqYPjBQLGX5c__ng@mail.gmail.com>
2019-07-06 19:17 ` [PATCH v1 4.20.y stable] CIFS: fix deadlock in cached root handling Aurelien Aptel
2019-07-06 19:30 ` Pavel Shilovsky
2019-07-06 20:11 ` [PATCH v2 " Aurelien Aptel
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).