| Message ID | 20191122153057.6608-4-pc@cjr.nz |
|---|---|
| State | New |
| Headers | show |
| Series | DFS fixes | expand |
"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>
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)
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 --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;
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(-)