| Message ID | 1478740076-60526-1-git-send-email-pshilov@microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016-11-09 17:07 GMT-08:00 Pavel Shilovsky <piastryyy@gmail.com>: > We can not unlock/lock spinlock in the middle of the reconnect > loop since it can corrupt list iterator pointers and a tcon > structure can be released if we don't hold an extra reference. > Fix it by moving a reconnect process to a separate delayed work > and acquiring a reference to every tcon that needs to be reconnected. > Also do not send an echo request on newly established connections. > > CC: Stable <stable@vger.kernel.org> > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > --- > fs/cifs/cifsglob.h | 3 +++ > fs/cifs/cifsproto.h | 3 +++ > fs/cifs/connect.c | 32 ++++++++++++++++++----- > fs/cifs/smb2pdu.c | 74 ++++++++++++++++++++++++++++++++++++----------------- > fs/cifs/smb2proto.h | 1 + > 5 files changed, 82 insertions(+), 31 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 1f17f6b..6dcefc8 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -632,6 +632,7 @@ struct TCP_Server_Info { > bool sec_mskerberos; /* supports legacy MS Kerberos */ > bool large_buf; /* is current buffer large? */ > struct delayed_work echo; /* echo ping workqueue job */ > + struct delayed_work reconnect; /* reconnect workqueue job */ > char *smallbuf; /* pointer to current "small" buffer */ > char *bigbuf; /* pointer to current "big" buffer */ > unsigned int total_read; /* total amount of data read in this pass */ > @@ -648,6 +649,7 @@ struct TCP_Server_Info { > __u8 preauth_hash[512]; > #endif /* CONFIG_CIFS_SMB2 */ > unsigned long echo_interval; > + struct mutex reconnect_mutex; /* prevent simultaneous reconnects */ > }; > > static inline unsigned int > @@ -850,6 +852,7 @@ struct cifs_tcon { > struct list_head tcon_list; > int tc_count; > struct list_head openFileList; > + struct list_head rlist; /* reconnect list */ > spinlock_t open_file_lock; /* protects list above */ > struct cifs_ses *ses; /* pointer to session associated with */ > char treeName[MAX_TREE_SIZE + 1]; /* UNC name of resource in ASCII */ > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index ced0e42..cd8025a 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -206,6 +206,9 @@ extern void cifs_add_pending_open_locked(struct cifs_fid *fid, > struct tcon_link *tlink, > struct cifs_pending_open *open); > extern void cifs_del_pending_open(struct cifs_pending_open *open); > +extern void cifs_put_tcp_session(struct TCP_Server_Info *server, > + int from_reconnect); > +extern void cifs_put_tcon(struct cifs_tcon *tcon); > > #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) > extern void cifs_dfs_release_automount_timer(void); > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 4547aed..69452f7 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -52,6 +52,9 @@ > #include "nterr.h" > #include "rfc1002pdu.h" > #include "fscache.h" > +#ifdef CONFIG_CIFS_SMB2 > +#include "smb2proto.h" > +#endif > > #define CIFS_PORT 445 > #define RFC1001_PORT 139 > @@ -2100,8 +2103,8 @@ cifs_find_tcp_session(struct smb_vol *vol) > return NULL; > } > > -static void > -cifs_put_tcp_session(struct TCP_Server_Info *server) > +void > +cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect) > { > struct task_struct *task; > > @@ -2118,6 +2121,17 @@ cifs_put_tcp_session(struct TCP_Server_Info *server) > > cancel_delayed_work_sync(&server->echo); > > + if (from_reconnect) > + /* > + * Avoid deadlock here: reconnect work calls > + * cifs_put_tcp_session() at its end. Need to be sure > + * that reconnect work does nothing with server pointer after > + * that step. > + */ > + cancel_delayed_work(&server->reconnect); > + else > + cancel_delayed_work_sync(&server->reconnect); > + > spin_lock(&GlobalMid_Lock); > server->tcpStatus = CifsExiting; > spin_unlock(&GlobalMid_Lock); > @@ -2171,6 +2185,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info) > init_waitqueue_head(&tcp_ses->request_q); > INIT_LIST_HEAD(&tcp_ses->pending_mid_q); > mutex_init(&tcp_ses->srv_mutex); > + mutex_init(&tcp_ses->reconnect_mutex); > memcpy(tcp_ses->workstation_RFC1001_name, > volume_info->source_rfc1001_name, RFC1001_NAME_LEN_WITH_NULL); > memcpy(tcp_ses->server_RFC1001_name, > @@ -2182,6 +2197,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info) > INIT_LIST_HEAD(&tcp_ses->tcp_ses_list); > INIT_LIST_HEAD(&tcp_ses->smb_ses_list); > INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request); > +#ifdef CONFIG_CIFS_SMB2 > + INIT_DELAYED_WORK(&tcp_ses->reconnect, smb2_reconnect_server); > +#endif > memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr, > sizeof(tcp_ses->srcaddr)); > memcpy(&tcp_ses->dstaddr, &volume_info->dstaddr, > @@ -2340,7 +2358,7 @@ cifs_put_smb_ses(struct cifs_ses *ses) > spin_unlock(&cifs_tcp_ses_lock); > > sesInfoFree(ses); > - cifs_put_tcp_session(server); > + cifs_put_tcp_session(server, 0); > } > > #ifdef CONFIG_KEYS > @@ -2514,7 +2532,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) > mutex_unlock(&ses->session_mutex); > > /* existing SMB ses has a server reference already */ > - cifs_put_tcp_session(server); > + cifs_put_tcp_session(server, 0); > free_xid(xid); > return ses; > } > @@ -2604,7 +2622,7 @@ cifs_find_tcon(struct cifs_ses *ses, const char *unc) > return NULL; > } > > -static void > +void > cifs_put_tcon(struct cifs_tcon *tcon) > { > unsigned int xid; > @@ -3792,7 +3810,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info) > else if (ses) > cifs_put_smb_ses(ses); > else > - cifs_put_tcp_session(server); > + cifs_put_tcp_session(server, 0); > bdi_destroy(&cifs_sb->bdi); > } > > @@ -4103,7 +4121,7 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, kuid_t fsuid) > ses = cifs_get_smb_ses(master_tcon->ses->server, vol_info); > if (IS_ERR(ses)) { > tcon = (struct cifs_tcon *)ses; > - cifs_put_tcp_session(master_tcon->ses->server); > + cifs_put_tcp_session(master_tcon->ses->server, 0); > goto out; > } > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 5ca5ea46..af4545d 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1972,6 +1972,53 @@ smb2_echo_callback(struct mid_q_entry *mid) > add_credits(server, credits_received, CIFS_ECHO_OP); > } > > +void smb2_reconnect_server(struct work_struct *work) > +{ > + struct TCP_Server_Info *server = container_of(work, > + struct TCP_Server_Info, reconnect.work); > + struct cifs_ses *ses; > + struct cifs_tcon *tcon, *tcon2; > + struct list_head tmp_list; > + int ses_exist = false; > + > + /* Prevent simultaneous reconnects that can corrupt tcon->rlist list */ > + mutex_lock(&server->reconnect_mutex); > + > + INIT_LIST_HEAD(&tmp_list); > + cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n"); > + > + spin_lock(&cifs_tcp_ses_lock); > + list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) { > + ses_exist = true; > + list_for_each_entry(tcon, &ses->tcon_list, tcon_list) { > + if (tcon && tcon->need_reconnect) { > + tcon->tc_count++; > + list_add_tail(&tcon->rlist, &tmp_list); > + } > + } > + } > + /* > + * Get the reference to server struct to be sure that the last call of > + * cifs_put_tcon() in the loop below won't release the server pointer. > + */ > + if (ses_exist) > + server->srv_count++; > + > + spin_unlock(&cifs_tcp_ses_lock); > + > + list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) { > + smb2_reconnect(SMB2_ECHO, tcon); > + list_del_init(&tcon->rlist); > + cifs_put_tcon(tcon); > + } > + > + cifs_dbg(FYI, "Reconnecting tcons finished\n"); > + mutex_unlock(&server->reconnect_mutex); > + > + /* now we can safely release srv struct */ > + cifs_put_tcp_session(server, 1); > +} > + > int > SMB2_echo(struct TCP_Server_Info *server) > { > @@ -1984,32 +2031,11 @@ SMB2_echo(struct TCP_Server_Info *server) > cifs_dbg(FYI, "In echo request\n"); > > if (server->tcpStatus == CifsNeedNegotiate) { > - struct list_head *tmp, *tmp2; > - struct cifs_ses *ses; > - struct cifs_tcon *tcon; > - > - cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n"); > - spin_lock(&cifs_tcp_ses_lock); > - list_for_each(tmp, &server->smb_ses_list) { > - ses = list_entry(tmp, struct cifs_ses, smb_ses_list); > - list_for_each(tmp2, &ses->tcon_list) { > - tcon = list_entry(tmp2, struct cifs_tcon, > - tcon_list); > - /* add check for persistent handle reconnect */ > - if (tcon && tcon->need_reconnect) { > - spin_unlock(&cifs_tcp_ses_lock); > - rc = smb2_reconnect(SMB2_ECHO, tcon); > - spin_lock(&cifs_tcp_ses_lock); > - } > - } > - } > - spin_unlock(&cifs_tcp_ses_lock); > + /* No need to send echo an a newly established connections */ > + mod_delayed_work(cifsiod_wq, &server->reconnect, 0); > + return rc; > } > > - /* if no session, renegotiate failed above */ > - if (server->tcpStatus == CifsNeedNegotiate) > - return -EIO; > - > rc = small_smb2_init(SMB2_ECHO, NULL, (void **)&req); > if (rc) > return rc; > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index eb2cde2..f2d511a 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -96,6 +96,7 @@ extern int smb2_open_file(const unsigned int xid, > extern int smb2_unlock_range(struct cifsFileInfo *cfile, > struct file_lock *flock, const unsigned int xid); > extern int smb2_push_mandatory_locks(struct cifsFileInfo *cfile); > +extern void smb2_reconnect_server(struct work_struct *work); > > /* > * SMB2 Worker functions - most of protocol specific implementation details > -- > 2.7.4 > Added missed #ifdef CONFIG_CIFS_SMB2 in cifs_put_tcp_session() and fixed a bug in smb2_reconnect_server() that releases a server pointer unconditionally. Posted the 2nd version of the patch with an improved description.
On Wed, 2016-11-09 at 17:07 -0800, Pavel Shilovsky wrote: > We can not unlock/lock spinlock in the middle of the reconnect > loop since it can corrupt list iterator pointers and a tcon > structure can be released if we don't hold an extra reference. > Fix it by moving a reconnect process to a separate delayed work > and acquiring a reference to every tcon that needs to be reconnected. > Also do not send an echo request on newly established connections. > > CC: Stable <stable@vger.kernel.org> > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > --- > fs/cifs/cifsglob.h | 3 +++ > fs/cifs/cifsproto.h | 3 +++ > fs/cifs/connect.c | 32 ++++++++++++++++++----- > fs/cifs/smb2pdu.c | 74 ++++++++++++++++++++++++++++++++++++------- > ---------- > fs/cifs/smb2proto.h | 1 + > 5 files changed, 82 insertions(+), 31 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 1f17f6b..6dcefc8 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -632,6 +632,7 @@ struct TCP_Server_Info { > bool sec_mskerberos; /* supports > legacy MS Kerberos */ > bool large_buf; /* is current buffer > large? */ > struct delayed_work echo; /* echo ping workqueue job > */ > + struct delayed_work reconnect; /* reconnect workqueue > job */ > char *smallbuf; /* pointer to current "small" > buffer */ > char *bigbuf; /* pointer to current "big" > buffer */ > unsigned int total_read; /* total amount of data read in > this pass */ > @@ -648,6 +649,7 @@ struct TCP_Server_Info { > __u8 preauth_hash[512]; > #endif /* CONFIG_CIFS_SMB2 */ > unsigned long echo_interval; > + struct mutex reconnect_mutex; /* prevent simultaneous > reconnects */ > }; > > static inline unsigned int > @@ -850,6 +852,7 @@ struct cifs_tcon { > struct list_head tcon_list; > int tc_count; > struct list_head openFileList; > + struct list_head rlist; /* reconnect list */ > spinlock_t open_file_lock; /* protects list above */ > struct cifs_ses *ses; /* pointer to session > associated with */ > char treeName[MAX_TREE_SIZE + 1]; /* UNC name of resource in > ASCII */ > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index ced0e42..cd8025a 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -206,6 +206,9 @@ extern void cifs_add_pending_open_locked(struct > cifs_fid *fid, > struct tcon_link *tlink, > struct cifs_pending_open > *open); > extern void cifs_del_pending_open(struct cifs_pending_open *open); > +extern void cifs_put_tcp_session(struct TCP_Server_Info *server, > + int from_reconnect); > +extern void cifs_put_tcon(struct cifs_tcon *tcon); > > #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) > extern void cifs_dfs_release_automount_timer(void); > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index 4547aed..69452f7 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -52,6 +52,9 @@ > #include "nterr.h" > #include "rfc1002pdu.h" > #include "fscache.h" > +#ifdef CONFIG_CIFS_SMB2 > +#include "smb2proto.h" > +#endif > > #define CIFS_PORT 445 > #define RFC1001_PORT 139 > @@ -2100,8 +2103,8 @@ cifs_find_tcp_session(struct smb_vol *vol) > return NULL; > } > > -static void > -cifs_put_tcp_session(struct TCP_Server_Info *server) > +void > +cifs_put_tcp_session(struct TCP_Server_Info *server, int > from_reconnect) > { > struct task_struct *task; > > @@ -2118,6 +2121,17 @@ cifs_put_tcp_session(struct TCP_Server_Info > *server) > > cancel_delayed_work_sync(&server->echo); > > + if (from_reconnect) > + /* > + * Avoid deadlock here: reconnect work calls > + * cifs_put_tcp_session() at its end. Need to be > sure > + * that reconnect work does nothing with server > pointer after > + * that step. > + */ > + cancel_delayed_work(&server->reconnect); > + else > + cancel_delayed_work_sync(&server->reconnect); > + Hello Pavel, I don't understand this part. Can you please explain this part. Sachin Prabhu > spin_lock(&GlobalMid_Lock); > server->tcpStatus = CifsExiting; > spin_unlock(&GlobalMid_Lock); > @@ -2171,6 +2185,7 @@ cifs_get_tcp_session(struct smb_vol > *volume_info) > init_waitqueue_head(&tcp_ses->request_q); > INIT_LIST_HEAD(&tcp_ses->pending_mid_q); > mutex_init(&tcp_ses->srv_mutex); > + mutex_init(&tcp_ses->reconnect_mutex); > memcpy(tcp_ses->workstation_RFC1001_name, > volume_info->source_rfc1001_name, > RFC1001_NAME_LEN_WITH_NULL); > memcpy(tcp_ses->server_RFC1001_name, > @@ -2182,6 +2197,9 @@ cifs_get_tcp_session(struct smb_vol > *volume_info) > INIT_LIST_HEAD(&tcp_ses->tcp_ses_list); > INIT_LIST_HEAD(&tcp_ses->smb_ses_list); > INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request); > +#ifdef CONFIG_CIFS_SMB2 > + INIT_DELAYED_WORK(&tcp_ses->reconnect, > smb2_reconnect_server); > +#endif > memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr, > sizeof(tcp_ses->srcaddr)); > memcpy(&tcp_ses->dstaddr, &volume_info->dstaddr, > @@ -2340,7 +2358,7 @@ cifs_put_smb_ses(struct cifs_ses *ses) > spin_unlock(&cifs_tcp_ses_lock); > > sesInfoFree(ses); > - cifs_put_tcp_session(server); > + cifs_put_tcp_session(server, 0); > } > > #ifdef CONFIG_KEYS > @@ -2514,7 +2532,7 @@ cifs_get_smb_ses(struct TCP_Server_Info > *server, struct smb_vol *volume_info) > mutex_unlock(&ses->session_mutex); > > /* existing SMB ses has a server reference already > */ > - cifs_put_tcp_session(server); > + cifs_put_tcp_session(server, 0); > free_xid(xid); > return ses; > } > @@ -2604,7 +2622,7 @@ cifs_find_tcon(struct cifs_ses *ses, const char > *unc) > return NULL; > } > > -static void > +void > cifs_put_tcon(struct cifs_tcon *tcon) > { > unsigned int xid; > @@ -3792,7 +3810,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct > smb_vol *volume_info) > else if (ses) > cifs_put_smb_ses(ses); > else > - cifs_put_tcp_session(server); > + cifs_put_tcp_session(server, 0); > bdi_destroy(&cifs_sb->bdi); > } > > @@ -4103,7 +4121,7 @@ cifs_construct_tcon(struct cifs_sb_info > *cifs_sb, kuid_t fsuid) > ses = cifs_get_smb_ses(master_tcon->ses->server, vol_info); > if (IS_ERR(ses)) { > tcon = (struct cifs_tcon *)ses; > - cifs_put_tcp_session(master_tcon->ses->server); > + cifs_put_tcp_session(master_tcon->ses->server, 0); > goto out; > } > > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c > index 5ca5ea46..af4545d 100644 > --- a/fs/cifs/smb2pdu.c > +++ b/fs/cifs/smb2pdu.c > @@ -1972,6 +1972,53 @@ smb2_echo_callback(struct mid_q_entry *mid) > add_credits(server, credits_received, CIFS_ECHO_OP); > } > > +void smb2_reconnect_server(struct work_struct *work) > +{ > + struct TCP_Server_Info *server = container_of(work, > + struct TCP_Server_Info, > reconnect.work); > + struct cifs_ses *ses; > + struct cifs_tcon *tcon, *tcon2; > + struct list_head tmp_list; > + int ses_exist = false; > + > + /* Prevent simultaneous reconnects that can corrupt tcon- > >rlist list */ > + mutex_lock(&server->reconnect_mutex); > + > + INIT_LIST_HEAD(&tmp_list); > + cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n"); > + > + spin_lock(&cifs_tcp_ses_lock); > + list_for_each_entry(ses, &server->smb_ses_list, > smb_ses_list) { > + ses_exist = true; > + list_for_each_entry(tcon, &ses->tcon_list, > tcon_list) { > + if (tcon && tcon->need_reconnect) { > + tcon->tc_count++; > + list_add_tail(&tcon->rlist, > &tmp_list); > + } > + } > + } > + /* > + * Get the reference to server struct to be sure that the > last call of > + * cifs_put_tcon() in the loop below won't release the > server pointer. > + */ > + if (ses_exist) > + server->srv_count++; > + > + spin_unlock(&cifs_tcp_ses_lock); > + > + list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) { > + smb2_reconnect(SMB2_ECHO, tcon); > + list_del_init(&tcon->rlist); > + cifs_put_tcon(tcon); > + } > + > + cifs_dbg(FYI, "Reconnecting tcons finished\n"); > + mutex_unlock(&server->reconnect_mutex); > + > + /* now we can safely release srv struct */ > + cifs_put_tcp_session(server, 1); > +} > + > int > SMB2_echo(struct TCP_Server_Info *server) > { > @@ -1984,32 +2031,11 @@ SMB2_echo(struct TCP_Server_Info *server) > cifs_dbg(FYI, "In echo request\n"); > > if (server->tcpStatus == CifsNeedNegotiate) { > - struct list_head *tmp, *tmp2; > - struct cifs_ses *ses; > - struct cifs_tcon *tcon; > - > - cifs_dbg(FYI, "Need negotiate, reconnecting > tcons\n"); > - spin_lock(&cifs_tcp_ses_lock); > - list_for_each(tmp, &server->smb_ses_list) { > - ses = list_entry(tmp, struct cifs_ses, > smb_ses_list); > - list_for_each(tmp2, &ses->tcon_list) { > - tcon = list_entry(tmp2, struct > cifs_tcon, > - tcon_list); > - /* add check for persistent handle > reconnect */ > - if (tcon && tcon->need_reconnect) { > - spin_unlock(&cifs_tcp_ses_lo > ck); > - rc = > smb2_reconnect(SMB2_ECHO, tcon); > - spin_lock(&cifs_tcp_ses_lock > ); > - } > - } > - } > - spin_unlock(&cifs_tcp_ses_lock); > + /* No need to send echo an a newly established > connections */ > + mod_delayed_work(cifsiod_wq, &server->reconnect, 0); > + return rc; > } > > - /* if no session, renegotiate failed above */ > - if (server->tcpStatus == CifsNeedNegotiate) > - return -EIO; > - > rc = small_smb2_init(SMB2_ECHO, NULL, (void **)&req); > if (rc) > return rc; > diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h > index eb2cde2..f2d511a 100644 > --- a/fs/cifs/smb2proto.h > +++ b/fs/cifs/smb2proto.h > @@ -96,6 +96,7 @@ extern int smb2_open_file(const unsigned int xid, > extern int smb2_unlock_range(struct cifsFileInfo *cfile, > struct file_lock *flock, const unsigned > int xid); > extern int smb2_push_mandatory_locks(struct cifsFileInfo *cfile); > +extern void smb2_reconnect_server(struct work_struct *work); > > /* > * SMB2 Worker functions - most of protocol specific implementation > details -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-11-24 3:50 GMT-08:00 Sachin Prabhu <sprabhu@redhat.com>: > On Wed, 2016-11-09 at 17:07 -0800, Pavel Shilovsky wrote: >> We can not unlock/lock spinlock in the middle of the reconnect >> loop since it can corrupt list iterator pointers and a tcon >> structure can be released if we don't hold an extra reference. >> Fix it by moving a reconnect process to a separate delayed work >> and acquiring a reference to every tcon that needs to be reconnected. >> Also do not send an echo request on newly established connections. >> >> CC: Stable <stable@vger.kernel.org> >> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> >> --- >> fs/cifs/cifsglob.h | 3 +++ >> fs/cifs/cifsproto.h | 3 +++ >> fs/cifs/connect.c | 32 ++++++++++++++++++----- >> fs/cifs/smb2pdu.c | 74 ++++++++++++++++++++++++++++++++++++------- >> ---------- >> fs/cifs/smb2proto.h | 1 + >> 5 files changed, 82 insertions(+), 31 deletions(-) >> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h >> index 1f17f6b..6dcefc8 100644 >> --- a/fs/cifs/cifsglob.h >> +++ b/fs/cifs/cifsglob.h >> @@ -632,6 +632,7 @@ struct TCP_Server_Info { >> bool sec_mskerberos; /* supports >> legacy MS Kerberos */ >> bool large_buf; /* is current buffer >> large? */ >> struct delayed_work echo; /* echo ping workqueue job >> */ >> + struct delayed_work reconnect; /* reconnect workqueue >> job */ >> char *smallbuf; /* pointer to current "small" >> buffer */ >> char *bigbuf; /* pointer to current "big" >> buffer */ >> unsigned int total_read; /* total amount of data read in >> this pass */ >> @@ -648,6 +649,7 @@ struct TCP_Server_Info { >> __u8 preauth_hash[512]; >> #endif /* CONFIG_CIFS_SMB2 */ >> unsigned long echo_interval; >> + struct mutex reconnect_mutex; /* prevent simultaneous >> reconnects */ >> }; >> >> static inline unsigned int >> @@ -850,6 +852,7 @@ struct cifs_tcon { >> struct list_head tcon_list; >> int tc_count; >> struct list_head openFileList; >> + struct list_head rlist; /* reconnect list */ >> spinlock_t open_file_lock; /* protects list above */ >> struct cifs_ses *ses; /* pointer to session >> associated with */ >> char treeName[MAX_TREE_SIZE + 1]; /* UNC name of resource in >> ASCII */ >> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h >> index ced0e42..cd8025a 100644 >> --- a/fs/cifs/cifsproto.h >> +++ b/fs/cifs/cifsproto.h >> @@ -206,6 +206,9 @@ extern void cifs_add_pending_open_locked(struct >> cifs_fid *fid, >> struct tcon_link *tlink, >> struct cifs_pending_open >> *open); >> extern void cifs_del_pending_open(struct cifs_pending_open *open); >> +extern void cifs_put_tcp_session(struct TCP_Server_Info *server, >> + int from_reconnect); >> +extern void cifs_put_tcon(struct cifs_tcon *tcon); >> >> #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) >> extern void cifs_dfs_release_automount_timer(void); >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c >> index 4547aed..69452f7 100644 >> --- a/fs/cifs/connect.c >> +++ b/fs/cifs/connect.c >> @@ -52,6 +52,9 @@ >> #include "nterr.h" >> #include "rfc1002pdu.h" >> #include "fscache.h" >> +#ifdef CONFIG_CIFS_SMB2 >> +#include "smb2proto.h" >> +#endif >> >> #define CIFS_PORT 445 >> #define RFC1001_PORT 139 >> @@ -2100,8 +2103,8 @@ cifs_find_tcp_session(struct smb_vol *vol) >> return NULL; >> } >> >> -static void >> -cifs_put_tcp_session(struct TCP_Server_Info *server) >> +void >> +cifs_put_tcp_session(struct TCP_Server_Info *server, int >> from_reconnect) >> { >> struct task_struct *task; >> >> @@ -2118,6 +2121,17 @@ cifs_put_tcp_session(struct TCP_Server_Info >> *server) >> >> cancel_delayed_work_sync(&server->echo); >> >> + if (from_reconnect) >> + /* >> + * Avoid deadlock here: reconnect work calls >> + * cifs_put_tcp_session() at its end. Need to be >> sure >> + * that reconnect work does nothing with server >> pointer after >> + * that step. >> + */ >> + cancel_delayed_work(&server->reconnect); >> + else >> + cancel_delayed_work_sync(&server->reconnect); >> + > > Hello Pavel, > > I don't understand this part. Can you please explain this part. Hello Sachin, In smb2_reconnect_server() we grabbed a reference to a server struct if any sessions/tcons (other references) exist to prevent cifs_put_tcon() releasing of a server pointer. At the end of the work functon we need to put the obtained reference but it can be the last one that will trigger releasing of the server pointer and calling cifs_put_tcp_session(). In the latter function we are trying to cancel delayed work (which we currently in), so cancel_delayed_work_sync() will be waiting forever.That's why "from_reconnect" argument was introduced to call non-blocking cancel_delayed_work() rather than blocking cancel_delayed_work_sync().
On Mon, 2016-11-28 at 11:47 -0800, Pavel Shilovsky wrote: > 2016-11-24 3:50 GMT-08:00 Sachin Prabhu <sprabhu@redhat.com>: > > > > On Wed, 2016-11-09 at 17:07 -0800, Pavel Shilovsky wrote: > > > > > > We can not unlock/lock spinlock in the middle of the reconnect > > > loop since it can corrupt list iterator pointers and a tcon > > > structure can be released if we don't hold an extra reference. > > > Fix it by moving a reconnect process to a separate delayed work > > > and acquiring a reference to every tcon that needs to be > > > reconnected. > > > Also do not send an echo request on newly established > > > connections. > > > > > > CC: Stable <stable@vger.kernel.org> > > > Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> > > > --- > > > fs/cifs/cifsglob.h | 3 +++ > > > fs/cifs/cifsproto.h | 3 +++ > > > fs/cifs/connect.c | 32 ++++++++++++++++++----- > > > fs/cifs/smb2pdu.c | 74 ++++++++++++++++++++++++++++++++++++--- > > > ---- > > > ---------- > > > fs/cifs/smb2proto.h | 1 + > > > 5 files changed, 82 insertions(+), 31 deletions(-) > > > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > > > index 1f17f6b..6dcefc8 100644 > > > --- a/fs/cifs/cifsglob.h > > > +++ b/fs/cifs/cifsglob.h > > > @@ -632,6 +632,7 @@ struct TCP_Server_Info { > > > bool sec_mskerberos; /* supports > > > legacy MS Kerberos */ > > > bool large_buf; /* is current buffer > > > large? */ > > > struct delayed_work echo; /* echo ping workqueue job > > > */ > > > + struct delayed_work reconnect; /* reconnect workqueue > > > job */ > > > char *smallbuf; /* pointer to current "small" > > > buffer */ > > > char *bigbuf; /* pointer to current "big" > > > buffer */ > > > unsigned int total_read; /* total amount of data read in > > > this pass */ > > > @@ -648,6 +649,7 @@ struct TCP_Server_Info { > > > __u8 preauth_hash[512]; > > > #endif /* CONFIG_CIFS_SMB2 */ > > > unsigned long echo_interval; > > > + struct mutex reconnect_mutex; /* prevent simultaneous > > > reconnects */ > > > }; > > > > > > static inline unsigned int > > > @@ -850,6 +852,7 @@ struct cifs_tcon { > > > struct list_head tcon_list; > > > int tc_count; > > > struct list_head openFileList; > > > + struct list_head rlist; /* reconnect list */ > > > spinlock_t open_file_lock; /* protects list above */ > > > struct cifs_ses *ses; /* pointer to session > > > associated with */ > > > char treeName[MAX_TREE_SIZE + 1]; /* UNC name of resource > > > in > > > ASCII */ > > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > > > index ced0e42..cd8025a 100644 > > > --- a/fs/cifs/cifsproto.h > > > +++ b/fs/cifs/cifsproto.h > > > @@ -206,6 +206,9 @@ extern void > > > cifs_add_pending_open_locked(struct > > > cifs_fid *fid, > > > struct tcon_link *tlink, > > > struct cifs_pending_open > > > *open); > > > extern void cifs_del_pending_open(struct cifs_pending_open > > > *open); > > > +extern void cifs_put_tcp_session(struct TCP_Server_Info *server, > > > + int from_reconnect); > > > +extern void cifs_put_tcon(struct cifs_tcon *tcon); > > > > > > #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) > > > extern void cifs_dfs_release_automount_timer(void); > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > > index 4547aed..69452f7 100644 > > > --- a/fs/cifs/connect.c > > > +++ b/fs/cifs/connect.c > > > @@ -52,6 +52,9 @@ > > > #include "nterr.h" > > > #include "rfc1002pdu.h" > > > #include "fscache.h" > > > +#ifdef CONFIG_CIFS_SMB2 > > > +#include "smb2proto.h" > > > +#endif > > > > > > #define CIFS_PORT 445 > > > #define RFC1001_PORT 139 > > > @@ -2100,8 +2103,8 @@ cifs_find_tcp_session(struct smb_vol *vol) > > > return NULL; > > > } > > > > > > -static void > > > -cifs_put_tcp_session(struct TCP_Server_Info *server) > > > +void > > > +cifs_put_tcp_session(struct TCP_Server_Info *server, int > > > from_reconnect) > > > { > > > struct task_struct *task; > > > > > > @@ -2118,6 +2121,17 @@ cifs_put_tcp_session(struct > > > TCP_Server_Info > > > *server) > > > > > > cancel_delayed_work_sync(&server->echo); > > > > > > + if (from_reconnect) > > > + /* > > > + * Avoid deadlock here: reconnect work calls > > > + * cifs_put_tcp_session() at its end. Need to be > > > sure > > > + * that reconnect work does nothing with server > > > pointer after > > > + * that step. > > > + */ > > > + cancel_delayed_work(&server->reconnect); > > > + else > > > + cancel_delayed_work_sync(&server->reconnect); > > > + > > > > Hello Pavel, > > > > I don't understand this part. Can you please explain this part. > > > Hello Sachin, > > In smb2_reconnect_server() we grabbed a reference to a server struct > if any sessions/tcons (other references) exist to prevent > cifs_put_tcon() releasing of a server pointer. At the end of the work > functon we need to put the obtained reference but it can be the last > one that will trigger releasing of the server pointer and calling > cifs_put_tcp_session(). > > In the latter function we are trying to cancel delayed work (which we > currently in), so cancel_delayed_work_sync() will be waiting > forever.That's why "from_reconnect" argument was introduced to call > non-blocking cancel_delayed_work() rather than blocking > cancel_delayed_work_sync(). > Thanks Pavel. The explanation makes it clear. I'll review the rest of the patch and get back to you. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 1f17f6b..6dcefc8 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -632,6 +632,7 @@ struct TCP_Server_Info { bool sec_mskerberos; /* supports legacy MS Kerberos */ bool large_buf; /* is current buffer large? */ struct delayed_work echo; /* echo ping workqueue job */ + struct delayed_work reconnect; /* reconnect workqueue job */ char *smallbuf; /* pointer to current "small" buffer */ char *bigbuf; /* pointer to current "big" buffer */ unsigned int total_read; /* total amount of data read in this pass */ @@ -648,6 +649,7 @@ struct TCP_Server_Info { __u8 preauth_hash[512]; #endif /* CONFIG_CIFS_SMB2 */ unsigned long echo_interval; + struct mutex reconnect_mutex; /* prevent simultaneous reconnects */ }; static inline unsigned int @@ -850,6 +852,7 @@ struct cifs_tcon { struct list_head tcon_list; int tc_count; struct list_head openFileList; + struct list_head rlist; /* reconnect list */ spinlock_t open_file_lock; /* protects list above */ struct cifs_ses *ses; /* pointer to session associated with */ char treeName[MAX_TREE_SIZE + 1]; /* UNC name of resource in ASCII */ diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index ced0e42..cd8025a 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -206,6 +206,9 @@ extern void cifs_add_pending_open_locked(struct cifs_fid *fid, struct tcon_link *tlink, struct cifs_pending_open *open); extern void cifs_del_pending_open(struct cifs_pending_open *open); +extern void cifs_put_tcp_session(struct TCP_Server_Info *server, + int from_reconnect); +extern void cifs_put_tcon(struct cifs_tcon *tcon); #if IS_ENABLED(CONFIG_CIFS_DFS_UPCALL) extern void cifs_dfs_release_automount_timer(void); diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 4547aed..69452f7 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -52,6 +52,9 @@ #include "nterr.h" #include "rfc1002pdu.h" #include "fscache.h" +#ifdef CONFIG_CIFS_SMB2 +#include "smb2proto.h" +#endif #define CIFS_PORT 445 #define RFC1001_PORT 139 @@ -2100,8 +2103,8 @@ cifs_find_tcp_session(struct smb_vol *vol) return NULL; } -static void -cifs_put_tcp_session(struct TCP_Server_Info *server) +void +cifs_put_tcp_session(struct TCP_Server_Info *server, int from_reconnect) { struct task_struct *task; @@ -2118,6 +2121,17 @@ cifs_put_tcp_session(struct TCP_Server_Info *server) cancel_delayed_work_sync(&server->echo); + if (from_reconnect) + /* + * Avoid deadlock here: reconnect work calls + * cifs_put_tcp_session() at its end. Need to be sure + * that reconnect work does nothing with server pointer after + * that step. + */ + cancel_delayed_work(&server->reconnect); + else + cancel_delayed_work_sync(&server->reconnect); + spin_lock(&GlobalMid_Lock); server->tcpStatus = CifsExiting; spin_unlock(&GlobalMid_Lock); @@ -2171,6 +2185,7 @@ cifs_get_tcp_session(struct smb_vol *volume_info) init_waitqueue_head(&tcp_ses->request_q); INIT_LIST_HEAD(&tcp_ses->pending_mid_q); mutex_init(&tcp_ses->srv_mutex); + mutex_init(&tcp_ses->reconnect_mutex); memcpy(tcp_ses->workstation_RFC1001_name, volume_info->source_rfc1001_name, RFC1001_NAME_LEN_WITH_NULL); memcpy(tcp_ses->server_RFC1001_name, @@ -2182,6 +2197,9 @@ cifs_get_tcp_session(struct smb_vol *volume_info) INIT_LIST_HEAD(&tcp_ses->tcp_ses_list); INIT_LIST_HEAD(&tcp_ses->smb_ses_list); INIT_DELAYED_WORK(&tcp_ses->echo, cifs_echo_request); +#ifdef CONFIG_CIFS_SMB2 + INIT_DELAYED_WORK(&tcp_ses->reconnect, smb2_reconnect_server); +#endif memcpy(&tcp_ses->srcaddr, &volume_info->srcaddr, sizeof(tcp_ses->srcaddr)); memcpy(&tcp_ses->dstaddr, &volume_info->dstaddr, @@ -2340,7 +2358,7 @@ cifs_put_smb_ses(struct cifs_ses *ses) spin_unlock(&cifs_tcp_ses_lock); sesInfoFree(ses); - cifs_put_tcp_session(server); + cifs_put_tcp_session(server, 0); } #ifdef CONFIG_KEYS @@ -2514,7 +2532,7 @@ cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb_vol *volume_info) mutex_unlock(&ses->session_mutex); /* existing SMB ses has a server reference already */ - cifs_put_tcp_session(server); + cifs_put_tcp_session(server, 0); free_xid(xid); return ses; } @@ -2604,7 +2622,7 @@ cifs_find_tcon(struct cifs_ses *ses, const char *unc) return NULL; } -static void +void cifs_put_tcon(struct cifs_tcon *tcon) { unsigned int xid; @@ -3792,7 +3810,7 @@ cifs_mount(struct cifs_sb_info *cifs_sb, struct smb_vol *volume_info) else if (ses) cifs_put_smb_ses(ses); else - cifs_put_tcp_session(server); + cifs_put_tcp_session(server, 0); bdi_destroy(&cifs_sb->bdi); } @@ -4103,7 +4121,7 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, kuid_t fsuid) ses = cifs_get_smb_ses(master_tcon->ses->server, vol_info); if (IS_ERR(ses)) { tcon = (struct cifs_tcon *)ses; - cifs_put_tcp_session(master_tcon->ses->server); + cifs_put_tcp_session(master_tcon->ses->server, 0); goto out; } diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 5ca5ea46..af4545d 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1972,6 +1972,53 @@ smb2_echo_callback(struct mid_q_entry *mid) add_credits(server, credits_received, CIFS_ECHO_OP); } +void smb2_reconnect_server(struct work_struct *work) +{ + struct TCP_Server_Info *server = container_of(work, + struct TCP_Server_Info, reconnect.work); + struct cifs_ses *ses; + struct cifs_tcon *tcon, *tcon2; + struct list_head tmp_list; + int ses_exist = false; + + /* Prevent simultaneous reconnects that can corrupt tcon->rlist list */ + mutex_lock(&server->reconnect_mutex); + + INIT_LIST_HEAD(&tmp_list); + cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n"); + + spin_lock(&cifs_tcp_ses_lock); + list_for_each_entry(ses, &server->smb_ses_list, smb_ses_list) { + ses_exist = true; + list_for_each_entry(tcon, &ses->tcon_list, tcon_list) { + if (tcon && tcon->need_reconnect) { + tcon->tc_count++; + list_add_tail(&tcon->rlist, &tmp_list); + } + } + } + /* + * Get the reference to server struct to be sure that the last call of + * cifs_put_tcon() in the loop below won't release the server pointer. + */ + if (ses_exist) + server->srv_count++; + + spin_unlock(&cifs_tcp_ses_lock); + + list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) { + smb2_reconnect(SMB2_ECHO, tcon); + list_del_init(&tcon->rlist); + cifs_put_tcon(tcon); + } + + cifs_dbg(FYI, "Reconnecting tcons finished\n"); + mutex_unlock(&server->reconnect_mutex); + + /* now we can safely release srv struct */ + cifs_put_tcp_session(server, 1); +} + int SMB2_echo(struct TCP_Server_Info *server) { @@ -1984,32 +2031,11 @@ SMB2_echo(struct TCP_Server_Info *server) cifs_dbg(FYI, "In echo request\n"); if (server->tcpStatus == CifsNeedNegotiate) { - struct list_head *tmp, *tmp2; - struct cifs_ses *ses; - struct cifs_tcon *tcon; - - cifs_dbg(FYI, "Need negotiate, reconnecting tcons\n"); - spin_lock(&cifs_tcp_ses_lock); - list_for_each(tmp, &server->smb_ses_list) { - ses = list_entry(tmp, struct cifs_ses, smb_ses_list); - list_for_each(tmp2, &ses->tcon_list) { - tcon = list_entry(tmp2, struct cifs_tcon, - tcon_list); - /* add check for persistent handle reconnect */ - if (tcon && tcon->need_reconnect) { - spin_unlock(&cifs_tcp_ses_lock); - rc = smb2_reconnect(SMB2_ECHO, tcon); - spin_lock(&cifs_tcp_ses_lock); - } - } - } - spin_unlock(&cifs_tcp_ses_lock); + /* No need to send echo an a newly established connections */ + mod_delayed_work(cifsiod_wq, &server->reconnect, 0); + return rc; } - /* if no session, renegotiate failed above */ - if (server->tcpStatus == CifsNeedNegotiate) - return -EIO; - rc = small_smb2_init(SMB2_ECHO, NULL, (void **)&req); if (rc) return rc; diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index eb2cde2..f2d511a 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -96,6 +96,7 @@ extern int smb2_open_file(const unsigned int xid, extern int smb2_unlock_range(struct cifsFileInfo *cfile, struct file_lock *flock, const unsigned int xid); extern int smb2_push_mandatory_locks(struct cifsFileInfo *cfile); +extern void smb2_reconnect_server(struct work_struct *work); /* * SMB2 Worker functions - most of protocol specific implementation details
We can not unlock/lock spinlock in the middle of the reconnect loop since it can corrupt list iterator pointers and a tcon structure can be released if we don't hold an extra reference. Fix it by moving a reconnect process to a separate delayed work and acquiring a reference to every tcon that needs to be reconnected. Also do not send an echo request on newly established connections. CC: Stable <stable@vger.kernel.org> Signed-off-by: Pavel Shilovsky <pshilov@microsoft.com> --- fs/cifs/cifsglob.h | 3 +++ fs/cifs/cifsproto.h | 3 +++ fs/cifs/connect.c | 32 ++++++++++++++++++----- fs/cifs/smb2pdu.c | 74 ++++++++++++++++++++++++++++++++++++----------------- fs/cifs/smb2proto.h | 1 + 5 files changed, 82 insertions(+), 31 deletions(-)