diff mbox series

[3/7] cifs: Fix potential softlockups while refreshing DFS cache

Message ID 20191122153057.6608-4-pc@cjr.nz (mailing list archive)
State New, archived
Headers show
Series DFS fixes | expand

Commit Message

Paulo Alcantara Nov. 22, 2019, 3:30 p.m. UTC
We used to skip reconnects on all SMB2_IOCTL commands due to SMB3+
FSCTL_VALIDATE_NEGOTIATE_INFO - which made sense since we're still
establishing a SMB session.

However, when refresh_cache_worker() calls smb2_get_dfs_refer() and
we're under reconnect, SMB2_ioctl() will not be able to get a proper
status error (e.g. -EHOSTDOWN in case we failed to reconnect) but an
-EAGAIN from cifs_send_recv() thus looping forever in
refresh_cache_worker().

Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Suggested-by: Aurelien Aptel <aaptel@suse.com>
---
 fs/cifs/smb2pdu.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

Comments

Aurélien Aptel Nov. 25, 2019, 11:41 a.m. UTC | #1
"Paulo Alcantara (SUSE)" <pc@cjr.nz> writes:
> We used to skip reconnects on all SMB2_IOCTL commands due to SMB3+
> FSCTL_VALIDATE_NEGOTIATE_INFO - which made sense since we're still
> establishing a SMB session.
>
> However, when refresh_cache_worker() calls smb2_get_dfs_refer() and
> we're under reconnect, SMB2_ioctl() will not be able to get a proper
> status error (e.g. -EHOSTDOWN in case we failed to reconnect) but an
> -EAGAIN from cifs_send_recv() thus looping forever in
> refresh_cache_worker().


I think we can add a Fixes tag:

Fixes: e99c63e4d86d ("SMB3: Fix deadlock in validate negotiate hits reconnect")

Otherwise looks good.

Reviewed-by: Aurelien Aptel <aaptel@suse.com>
Steve French Nov. 25, 2019, 3:35 p.m. UTC | #2
tags added - tentatively merged into cifs-2.6.git for-next pending
testing (buildbot)

On Mon, Nov 25, 2019 at 5:41 AM Aurélien Aptel <aaptel@suse.com> wrote:
>
> "Paulo Alcantara (SUSE)" <pc@cjr.nz> writes:
> > We used to skip reconnects on all SMB2_IOCTL commands due to SMB3+
> > FSCTL_VALIDATE_NEGOTIATE_INFO - which made sense since we're still
> > establishing a SMB session.
> >
> > However, when refresh_cache_worker() calls smb2_get_dfs_refer() and
> > we're under reconnect, SMB2_ioctl() will not be able to get a proper
> > status error (e.g. -EHOSTDOWN in case we failed to reconnect) but an
> > -EAGAIN from cifs_send_recv() thus looping forever in
> > refresh_cache_worker().
>
>
> I think we can add a Fixes tag:
>
> Fixes: e99c63e4d86d ("SMB3: Fix deadlock in validate negotiate hits reconnect")
>
> Otherwise looks good.
>
> Reviewed-by: Aurelien Aptel <aaptel@suse.com>
>
> --
> Aurélien Aptel / SUSE Labs Samba Team
> GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
Pavel Shilovsky Nov. 25, 2019, 7:53 p.m. UTC | #3
Stable candidate?
--
Best regards,
Pavel Shilovsky

пн, 25 нояб. 2019 г. в 07:35, Steve French <smfrench@gmail.com>:
>
> tags added - tentatively merged into cifs-2.6.git for-next pending
> testing (buildbot)
>
> On Mon, Nov 25, 2019 at 5:41 AM Aurélien Aptel <aaptel@suse.com> wrote:
> >
> > "Paulo Alcantara (SUSE)" <pc@cjr.nz> writes:
> > > We used to skip reconnects on all SMB2_IOCTL commands due to SMB3+
> > > FSCTL_VALIDATE_NEGOTIATE_INFO - which made sense since we're still
> > > establishing a SMB session.
> > >
> > > However, when refresh_cache_worker() calls smb2_get_dfs_refer() and
> > > we're under reconnect, SMB2_ioctl() will not be able to get a proper
> > > status error (e.g. -EHOSTDOWN in case we failed to reconnect) but an
> > > -EAGAIN from cifs_send_recv() thus looping forever in
> > > refresh_cache_worker().
> >
> >
> > I think we can add a Fixes tag:
> >
> > Fixes: e99c63e4d86d ("SMB3: Fix deadlock in validate negotiate hits reconnect")
> >
> > Otherwise looks good.
> >
> > Reviewed-by: Aurelien Aptel <aaptel@suse.com>
> >
> > --
> > Aurélien Aptel / SUSE Labs Samba Team
> > GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
> > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
> > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index c09ca6963394..3cd90088d8ac 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -252,7 +252,7 @@  smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon)
 	if (tcon == NULL)
 		return 0;
 
-	if (smb2_command == SMB2_TREE_CONNECT || smb2_command == SMB2_IOCTL)
+	if (smb2_command == SMB2_TREE_CONNECT)
 		return 0;
 
 	if (tcon->tidStatus == CifsExiting) {
@@ -426,16 +426,9 @@  fill_small_buf(__le16 smb2_command, struct cifs_tcon *tcon, void *buf,
  * SMB information in the SMB header. If the return code is zero, this
  * function must have filled in request_buf pointer.
  */
-static int
-smb2_plain_req_init(__le16 smb2_command, struct cifs_tcon *tcon,
-		    void **request_buf, unsigned int *total_len)
+static int __smb2_plain_req_init(__le16 smb2_command, struct cifs_tcon *tcon,
+				  void **request_buf, unsigned int *total_len)
 {
-	int rc;
-
-	rc = smb2_reconnect(smb2_command, tcon);
-	if (rc)
-		return rc;
-
 	/* BB eventually switch this to SMB2 specific small buf size */
 	if (smb2_command == SMB2_SET_INFO)
 		*request_buf = cifs_buf_get();
@@ -456,7 +449,31 @@  smb2_plain_req_init(__le16 smb2_command, struct cifs_tcon *tcon,
 		cifs_stats_inc(&tcon->num_smbs_sent);
 	}
 
-	return rc;
+	return 0;
+}
+
+static int smb2_plain_req_init(__le16 smb2_command, struct cifs_tcon *tcon,
+			       void **request_buf, unsigned int *total_len)
+{
+	int rc;
+
+	rc = smb2_reconnect(smb2_command, tcon);
+	if (rc)
+		return rc;
+
+	return __smb2_plain_req_init(smb2_command, tcon, request_buf,
+				     total_len);
+}
+
+static int smb2_ioctl_req_init(u32 opcode, struct cifs_tcon *tcon,
+			       void **request_buf, unsigned int *total_len)
+{
+	/* Skip reconnect only for FSCTL_VALIDATE_NEGOTIATE_INFO IOCTLs */
+	if (opcode == FSCTL_VALIDATE_NEGOTIATE_INFO) {
+		return __smb2_plain_req_init(SMB2_IOCTL, tcon, request_buf,
+					     total_len);
+	}
+	return smb2_plain_req_init(SMB2_IOCTL, tcon, request_buf, total_len);
 }
 
 /* For explanation of negotiate contexts see MS-SMB2 section 2.2.3.1 */
@@ -2686,7 +2703,7 @@  SMB2_ioctl_init(struct cifs_tcon *tcon, struct smb_rqst *rqst,
 	int rc;
 	char *in_data_buf;
 
-	rc = smb2_plain_req_init(SMB2_IOCTL, tcon, (void **) &req, &total_len);
+	rc = smb2_ioctl_req_init(opcode, tcon, (void **) &req, &total_len);
 	if (rc)
 		return rc;