| Message ID | 1379345025-10868-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 16 Sep 2013 11:23:45 -0400 Jeff Layton <jlayton@redhat.com> 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@gmx.de <sauron99@gmx.de> > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > 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 */ >
On Mon, Sep 16, 2013 at 11:23:45AM -0400, Jeff Layton 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. It's kindof a tough choice, but I like this patch. It does solve real problems. The downside certainly is that we have worse cleanup behaviour of dead connections, but I would prefer not killing NATed connections. Volker
On Wed, 18 Sep 2013 07:42:09 -0700 Volker Lendecke <Volker.Lendecke@SerNet.DE> wrote: > On Mon, Sep 16, 2013 at 11:23:45AM -0400, Jeff Layton 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. > > It's kindof a tough choice, but I like this patch. It does > solve real problems. The downside certainly is that we have > worse cleanup behaviour of dead connections, but I would > prefer not killing NATed connections. > > Volker > Agreed, but I'm not convinced that there's really a downside. We don't currently use share modes, so that shouldn't be an issue. Pavel's patchset may eventually change that, but I don't see it happening anytime too soon. My initial thinking was that we might end up not getting an oplock on a reconnect when we held one before. OTOH, one would think that if the client was reclaiming the open file, the server would try to issue an oplock break on the previous connection. At that point, it should find that it's dead anyway, so that also shouldn't be an issue. Implementing CIFS clearly states that VC handling by servers is spotty at best, so I think we're best off avoiding any use of them. In principle, we could add a knob that tells the server to use vcnum==0 on the first session to allow it to opt-in to this behavior, but I'd prefer not to do that until someone demonstrates a clear need for it so we can understand how best to implement it. If we did need to do that, there's clearly no need to use a different vcnum on every session. Simply using vcnum==0 on the first one and then vcnum==1 on every subsequent one would be sufficient.
On Wed, Sep 18, 2013 at 11:05:35AM -0400, Jeff Layton wrote: > Agreed, but I'm not convinced that there's really a downside. > > We don't currently use share modes, so that shouldn't be an issue. > Pavel's patchset may eventually change that, but I don't see it > happening anytime too soon. > > My initial thinking was that we might end up not getting an oplock on a > reconnect when we held one before. OTOH, one would think that if the > client was reclaiming the open file, the server would try to issue an > oplock break on the previous connection. At that point, it should find > that it's dead anyway, so that also shouldn't be an issue. > > Implementing CIFS clearly states that VC handling by servers is spotty > at best, so I think we're best off avoiding any use of them. > > In principle, we could add a knob that tells the server to use vcnum==0 > on the first session to allow it to opt-in to this behavior, but I'd > prefer not to do that until someone demonstrates a clear need for it so > we can understand how best to implement it. > > If we did need to do that, there's clearly no need to use a different > vcnum on every session. Simply using vcnum==0 on the first one and then > vcnum==1 on every subsequent one would be sufficient. Ack. Volker
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 */
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. Reported-by: Sauron99@gmx.de <sauron99@gmx.de> Signed-off-by: Jeff Layton <jlayton@redhat.com> --- fs/cifs/cifsglob.h | 4 --- fs/cifs/cifssmb.c | 1 - fs/cifs/sess.c | 84 +----------------------------------------------------- 3 files changed, 1 insertion(+), 88 deletions(-)