Re: [PATCH] cifs: stop trying to use virtual circuits

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 16 Sep 2013 11:23:45 -0400
Jeff Layton <jlayton@xxxxxxxxxx> wrote:

> Currently, we try to ensure that we use vcnum of 0 on the first
> established session on a connection and then try to use a different
> vcnum on each session after that.
> 
> This is a little odd, since there's no real reason to use a different
> vcnum for each SMB session. I can only assume there was some confusion
> between SMB sessions and VCs. That's somewhat understandable since they
> both get created during SESSION_SETUP, but the documentation indicates
> that they are really orthogonal. The comment on max_vcs in particular
> looks quite misguided. An SMB session is already uniquely identified
> by the SMB UID value -- there's no need to again uniquely ID with a
> VC.
> 
> Furthermore, a vcnum of 0 is a cue to the server that it should release
> any resources that were previously held by the client. This sounds like
> a good thing, until you consider that:
> 
> a) it totally ignores the fact that other programs on the box (e.g.
> smbclient) might have connections established to the server. Using a
> vcnum of 0 causes them to get kicked off.
> 
> b) it causes problems with NAT. If several clients are connected to the
> same server via the same NAT'ed address, whenever one connects to the
> server it kicks off all the others, which then reconnect and kick off
> the first one...ad nauseum.
> 
> I don't see any reason to ignore the advice in "Implementing CIFS" which
> has a comprehensive treatment of virtual circuits. In there, it states
> "...and contrary to the specs the client should always use a VcNumber of
> one, never zero."
> 
> Have the client just use a hardcoded vcnum of 1, and stop abusing the
> special behavior of vcnum 0.
> 


Forgot to mention too that I believe this patch will fix:

    https://bugzilla.samba.org/show_bug.cgi?id=10113


> Reported-by: Sauron99@xxxxxx <sauron99@xxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
>  fs/cifs/cifsglob.h |  4 ---
>  fs/cifs/cifssmb.c  |  1 -
>  fs/cifs/sess.c     | 84 +-----------------------------------------------------
>  3 files changed, 1 insertion(+), 88 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index cfa14c8..9c72be6 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -547,9 +547,6 @@ struct TCP_Server_Info {
>  	unsigned int max_rw;	/* maxRw specifies the maximum */
>  	/* message size the server can send or receive for */
>  	/* SMB_COM_WRITE_RAW or SMB_COM_READ_RAW. */
> -	unsigned int max_vcs;	/* maximum number of smb sessions, at least
> -				   those that can be specified uniquely with
> -				   vcnumbers */
>  	unsigned int capabilities; /* selective disabling of caps by smb sess */
>  	int timeAdj;  /* Adjust for difference in server time zone in sec */
>  	__u64 CurrentMid;         /* multiplex id - rotating counter */
> @@ -715,7 +712,6 @@ struct cifs_ses {
>  	enum statusEnum status;
>  	unsigned overrideSecFlg;  /* if non-zero override global sec flags */
>  	__u16 ipc_tid;		/* special tid for connection to IPC share */
> -	__u16 vcnum;
>  	char *serverOS;		/* name of operating system underlying server */
>  	char *serverNOS;	/* name of network operating system of server */
>  	char *serverDomain;	/* security realm of server */
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index a3d74fe..4baf359 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -463,7 +463,6 @@ decode_lanman_negprot_rsp(struct TCP_Server_Info *server, NEGOTIATE_RSP *pSMBr)
>  			       cifs_max_pending);
>  	set_credits(server, server->maxReq);
>  	server->maxBuf = le16_to_cpu(rsp->MaxBufSize);
> -	server->max_vcs = le16_to_cpu(rsp->MaxNumberVcs);
>  	/* even though we do not use raw we might as well set this
>  	accurately, in case we ever find a need for it */
>  	if ((le16_to_cpu(rsp->RawMode) & RAW_ENABLE) == RAW_ENABLE) {
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 5f99b7f..352358d 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -32,88 +32,6 @@
>  #include <linux/slab.h>
>  #include "cifs_spnego.h"
>  
> -/*
> - * Checks if this is the first smb session to be reconnected after
> - * the socket has been reestablished (so we know whether to use vc 0).
> - * Called while holding the cifs_tcp_ses_lock, so do not block
> - */
> -static bool is_first_ses_reconnect(struct cifs_ses *ses)
> -{
> -	struct list_head *tmp;
> -	struct cifs_ses *tmp_ses;
> -
> -	list_for_each(tmp, &ses->server->smb_ses_list) {
> -		tmp_ses = list_entry(tmp, struct cifs_ses,
> -				     smb_ses_list);
> -		if (tmp_ses->need_reconnect == false)
> -			return false;
> -	}
> -	/* could not find a session that was already connected,
> -	   this must be the first one we are reconnecting */
> -	return true;
> -}
> -
> -/*
> - *	vc number 0 is treated specially by some servers, and should be the
> - *      first one we request.  After that we can use vcnumbers up to maxvcs,
> - *	one for each smb session (some Windows versions set maxvcs incorrectly
> - *	so maxvc=1 can be ignored).  If we have too many vcs, we can reuse
> - *	any vc but zero (some servers reset the connection on vcnum zero)
> - *
> - */
> -static __le16 get_next_vcnum(struct cifs_ses *ses)
> -{
> -	__u16 vcnum = 0;
> -	struct list_head *tmp;
> -	struct cifs_ses *tmp_ses;
> -	__u16 max_vcs = ses->server->max_vcs;
> -	__u16 i;
> -	int free_vc_found = 0;
> -
> -	/* Quoting the MS-SMB specification: "Windows-based SMB servers set this
> -	field to one but do not enforce this limit, which allows an SMB client
> -	to establish more virtual circuits than allowed by this value ... but
> -	other server implementations can enforce this limit." */
> -	if (max_vcs < 2)
> -		max_vcs = 0xFFFF;
> -
> -	spin_lock(&cifs_tcp_ses_lock);
> -	if ((ses->need_reconnect) && is_first_ses_reconnect(ses))
> -			goto get_vc_num_exit;  /* vcnum will be zero */
> -	for (i = ses->server->srv_count - 1; i < max_vcs; i++) {
> -		if (i == 0) /* this is the only connection, use vc 0 */
> -			break;
> -
> -		free_vc_found = 1;
> -
> -		list_for_each(tmp, &ses->server->smb_ses_list) {
> -			tmp_ses = list_entry(tmp, struct cifs_ses,
> -					     smb_ses_list);
> -			if (tmp_ses->vcnum == i) {
> -				free_vc_found = 0;
> -				break; /* found duplicate, try next vcnum */
> -			}
> -		}
> -		if (free_vc_found)
> -			break; /* we found a vcnumber that will work - use it */
> -	}
> -
> -	if (i == 0)
> -		vcnum = 0; /* for most common case, ie if one smb session, use
> -			      vc zero.  Also for case when no free vcnum, zero
> -			      is safest to send (some clients only send zero) */
> -	else if (free_vc_found == 0)
> -		vcnum = 1;  /* we can not reuse vc=0 safely, since some servers
> -				reset all uids on that, but 1 is ok. */
> -	else
> -		vcnum = i;
> -	ses->vcnum = vcnum;
> -get_vc_num_exit:
> -	spin_unlock(&cifs_tcp_ses_lock);
> -
> -	return cpu_to_le16(vcnum);
> -}
> -
>  static __u32 cifs_ssetup_hdr(struct cifs_ses *ses, SESSION_SETUP_ANDX *pSMB)
>  {
>  	__u32 capabilities = 0;
> @@ -128,7 +46,7 @@ static __u32 cifs_ssetup_hdr(struct cifs_ses *ses, SESSION_SETUP_ANDX *pSMB)
>  					CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4,
>  					USHRT_MAX));
>  	pSMB->req.MaxMpxCount = cpu_to_le16(ses->server->maxReq);
> -	pSMB->req.VcNumber = get_next_vcnum(ses);
> +	pSMB->req.VcNumber = __constant_cpu_to_le16(1);
>  
>  	/* Now no need to set SMBFLG_CASELESS or obsolete CANONICAL PATH */
>  

-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
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




[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux