On Thu, 2016-11-10 at 15:31 -0800, Pavel Shilovsky wrote:
> We can not unlock/lock cifs_tcp_ses_lock while walking through ses
> and tcon lists because 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Pavel Shilovsky <pshilov@xxxxxxxxxxxxx>
> ---
> fs/cifs/cifsglob.h | 3 +++
> fs/cifs/cifsproto.h | 3 +++
> fs/cifs/connect.c | 34 +++++++++++++++++++-----
> fs/cifs/smb2pdu.c | 75 ++++++++++++++++++++++++++++++++++++-------
> ----------
> fs/cifs/smb2proto.h | 1 +
> 5 files changed, 85 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..75503af 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,19 @@ cifs_put_tcp_session(struct TCP_Server_Info
> *server)
>
> cancel_delayed_work_sync(&server->echo);
>
> +#ifdef CONFIG_CIFS_SMB2
> + 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);
> +#endif
Do you need the #ifdef CONFIG_CIFS_SMB2 here considering that there are
no such macro definitions around the declarations around delayed_work
reconnect in struct TCP_Server_Info. It would also lead to unused
variable in case the kernel is compiled without CONFIG_CIFS_SMB2.
> +
> spin_lock(&GlobalMid_Lock);
> server->tcpStatus = CifsExiting;
> spin_unlock(&GlobalMid_Lock);
> @@ -2171,6 +2187,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 +2199,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 +2360,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 +2534,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 +2624,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 +3812,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 +4123,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..950dbf7 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -1972,6 +1972,54 @@ 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 tcon_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) {
> + 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);
> + tcon_exist = true;
> + }
> + }
> + }
> + /*
> + * 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 (tcon_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 */
> + if (tcon_exist)
> + cifs_put_tcp_session(server, 1);
> +}
> +
> int
> SMB2_echo(struct TCP_Server_Info *server)
> {
> @@ -1984,32 +2032,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 on newly established
> connections */
> + mod_delayed_work(cifsiod_wq, &server->reconnect, 0);
> + return rc;
Shouldn't this be queue_work?
> }
>
> - /* 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
Sachin Prabhu
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
