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