Thinking about this a bit more, it seems to me that if you do want to support an explicit dialect selection at mount time, then you may want to check whether you have an existing mount over a different dialect from the client before allowing it. The alternative may be to select a new ClientGUID for the new mount, to avoid the session create failure - but that will mean leasing won't work very well. Might be a tough choice, if you do want this dialect option. > -----Original Message----- > From: linux-cifs-owner@xxxxxxxxxxxxxxx [mailto:linux-cifs- > owner@xxxxxxxxxxxxxxx] On Behalf Of Tom Talpey > Sent: Thursday, May 15, 2014 10:35 AM > To: Steve French > Cc: Sachin Prabhu; linux-cifs > Subject: RE: [PATCH] cifs: Set client guid on per connection basis > > > Would multichannel require a new negotiate protocol? > > A new connection always requires a negotiate, multichannel is no exception. > What's different is that to make a multichannel association, the client should > provide the original ClientGUID, and establish the same dialect and > capabilities as the original connection. The session bind needs these in order > join appropriately on the new multichannel. > > The ClientGUID is mostly for supporting leases, and if you swizzle it per- > connection then clients will start breaking their own leases when using > multichannel. There is also some enforcement at session setup/bind time, as > documented in MS-SMB2 3.3.5.3.3: > > "The server MUST look up all existing connections from the client in the > global ConnectionList where Connection.ClientGuid matches > Session.Connection.ClientGuid. For any matching Connection, if > Connection.Dialect is not the same as Session.Connection.Dialect, the server > SHOULD<228> close the newly created Session, as specified in section > 3.3.4.12, by providing Session.SessionGlobalId as the input parameter, and > fail the session setup request with STATUS_USER_SESSION_DELETED." > > > I guess if you only call get_tcp_session once, then you might be ok (that's > your call). I was just pointing out that a new ClientGUID per-*connection* > will get in the way for you later, because multichannel reuses sessions on > many connections. Normally the protocol specifies a single ClientGUID which > is used for all connections from a given client. > > > > -----Original Message----- > > From: Steve French [mailto:smfrench@xxxxxxxxx] > > Sent: Thursday, May 15, 2014 10:12 AM > > To: Tom Talpey > > Cc: Sachin Prabhu; linux-cifs > > Subject: Re: [PATCH] cifs: Set client guid on per connection basis > > > > Would multichannel require a new negotiate protocol? We are only > > generating it in get_tcp_session - and would probably not go through > > that path when connecting another socket to the original parent > > struct. Currently the model where tcons hang off smb sessions which > > hang off tcp sessions looks strange when you consider multiple tcp > > connections but have to distinguish the code which connects the first > > tcp session for a machine and sets up the parent structure that all > > the others (tcons and smb sessions) hang off, from the code which > > connects subsequent tcp sessions for multichannel > > > > On Thu, May 15, 2014 at 8:36 AM, Tom Talpey <ttalpey@xxxxxxxxxxxxx> > > wrote: > > > Won't this need to be undone for multichannel support? The client > > > GUID > > needs to be the same when connecting multiple channels, a different > > one will break cross-channel replay detection. > > > > > > It's definitely true that if you intend to negotiate a different > > > dialect, then > > the client GUID should not be reused. > > > > > >> -----Original Message----- > > >> From: linux-cifs-owner@xxxxxxxxxxxxxxx [mailto:linux-cifs- > > >> owner@xxxxxxxxxxxxxxx] On Behalf Of Steve French > > >> Sent: Tuesday, May 13, 2014 10:49 PM > > >> To: Sachin Prabhu > > >> Cc: linux-cifs > > >> Subject: Re: [PATCH] cifs: Set client guid on per connection basis > > >> > > >> merged into cifs-2.6.git for-next > > >> > > >> but added an additional patch to fix the problem where we are > > >> sending a non-zero ClientGUID for SMB2.02 dialect (where it MUST be > > >> zero according to MS-SMB2, it is non-zero starting in SMB2.1) > > >> > > >> On Tue, May 13, 2014 at 1:48 AM, Sachin Prabhu > <sprabhu@xxxxxxxxxx> > > >> wrote: > > >> > When mounting from a Windows 2012R2 server, we hit the following > > >> > problem: > > >> > 1) Mount with any of the following versions - 2.0, 2.1 or 3.0 > > >> > 2) unmount > > >> > 3) Attempt a mount again using a different SMB version >= 2.0. > > >> > > > >> > You end up with the following failure: > > >> > Status code returned 0xc0000203 STATUS_USER_SESSION_DELETED > CIFS > > >> VFS: > > >> > Send error in SessSetup = -5 CIFS VFS: cifs_mount failed w/return > > >> > code = -5 > > >> > > > >> > I cannot reproduce this issue using a Windows 2008 R2 server. > > >> > > > >> > This appears to be caused because we use the same client guid for > > >> > the connection on first mount which we then disconnect and > > >> > attempt to mount again using a different protocol version. By > > >> > generating a new guid each time a new connection is Negotiated, > > >> > we avoid hitting this > > >> problem. > > >> > > > >> > Signed-off-by: Sachin Prabhu <sprabhu@xxxxxxxxxx> > > >> > --- > > >> > fs/cifs/cifsfs.c | 8 -------- > > >> > fs/cifs/cifsglob.h | 1 + > > >> > fs/cifs/connect.c | 3 +++ > > >> > fs/cifs/smb2pdu.c | 5 +++-- > > >> > fs/cifs/smb2pdu.h | 2 -- > > >> > 5 files changed, 7 insertions(+), 12 deletions(-) > > >> > > > >> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index > > >> > 5be1f99..c80aa5a 100644 > > >> > --- a/fs/cifs/cifsfs.c > > >> > +++ b/fs/cifs/cifsfs.c > > >> > @@ -87,10 +87,6 @@ extern mempool_t *cifs_mid_poolp; > > >> > > > >> > struct workqueue_struct *cifsiod_wq; > > >> > > > >> > -#ifdef CONFIG_CIFS_SMB2 > > >> > -__u8 cifs_client_guid[SMB2_CLIENT_GUID_SIZE]; > > >> > -#endif > > >> > - > > >> > /* > > >> > * Bumps refcount for cifs super block. > > >> > * Note that it should be only called if a referece to VFS super > > >> > block is @@ -1192,10 +1188,6 @@ init_cifs(void) > > >> > spin_lock_init(&cifs_file_list_lock); > > >> > spin_lock_init(&GlobalMid_Lock); > > >> > > > >> > -#ifdef CONFIG_CIFS_SMB2 > > >> > - get_random_bytes(cifs_client_guid, SMB2_CLIENT_GUID_SIZE); > > >> > -#endif > > >> > - > > >> > if (cifs_max_pending < 2) { > > >> > cifs_max_pending = 2; > > >> > cifs_dbg(FYI, "cifs_max_pending set to min of > > >> > 2\n"); diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index > > >> > 30f6e92..f74edd2 100644 > > >> > --- a/fs/cifs/cifsglob.h > > >> > +++ b/fs/cifs/cifsglob.h > > >> > @@ -559,6 +559,7 @@ struct TCP_Server_Info { > > >> > int echo_credits; /* echo reserved slots */ > > >> > int oplock_credits; /* oplock break reserved slots */ > > >> > bool echoes:1; /* enable echoes */ > > >> > + __u8 client_guid[SMB2_CLIENT_GUID_SIZE]; /* Client GUID > > >> > + */ > > >> > #endif > > >> > u16 dialect; /* dialect index that server chose */ > > >> > bool oplocks:1; /* enable oplocks */ diff --git > > >> > a/fs/cifs/connect.c b/fs/cifs/connect.c index 8813ff7..8b8fe9b > > >> > 100644 > > >> > --- a/fs/cifs/connect.c > > >> > +++ b/fs/cifs/connect.c > > >> > @@ -2144,6 +2144,9 @@ cifs_get_tcp_session(struct smb_vol > > >> *volume_info) > > >> > sizeof(tcp_ses->srcaddr)); > > >> > memcpy(&tcp_ses->dstaddr, &volume_info->dstaddr, > > >> > sizeof(tcp_ses->dstaddr)); > > >> > +#ifdef CONFIG_CIFS_SMB2 > > >> > + get_random_bytes(tcp_ses->client_guid, > > >> > +SMB2_CLIENT_GUID_SIZE); #endif > > >> > /* > > >> > * at this point we are the only ones with the pointer > > >> > * to the struct since the kernel thread not created yet > > >> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index > > >> > 3802f8c..dc44610 > > >> > 100644 > > >> > --- a/fs/cifs/smb2pdu.c > > >> > +++ b/fs/cifs/smb2pdu.c > > >> > @@ -375,7 +375,7 @@ SMB2_negotiate(const unsigned int xid, struct > > >> > cifs_ses *ses) > > >> > > > >> > req->Capabilities = > > >> > cpu_to_le32(ses->server->vals->req_capabilities); > > >> > > > >> > - memcpy(req->ClientGUID, cifs_client_guid, > > >> SMB2_CLIENT_GUID_SIZE); > > >> > + memcpy(req->ClientGUID, server->client_guid, > > >> > + SMB2_CLIENT_GUID_SIZE); > > >> > > > >> > iov[0].iov_base = (char *)req; > > >> > /* 4 for rfc1002 length field */ @@ -478,7 +478,8 @@ int > > >> > smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon > > >> > *tcon) > > >> > > > >> > vneg_inbuf.Capabilities = > > >> > cpu_to_le32(tcon->ses->server->vals->req_capabilities); > > >> > - memcpy(vneg_inbuf.Guid, cifs_client_guid, > > >> SMB2_CLIENT_GUID_SIZE); > > >> > + memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid, > > >> > + SMB2_CLIENT_GUID_SIZE); > > >> > > > >> > if (tcon->ses->sign) > > >> > vneg_inbuf.SecurityMode = diff --git > > >> > a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index 2022c54..743e11e > > >> > 100644 > > >> > --- a/fs/cifs/smb2pdu.h > > >> > +++ b/fs/cifs/smb2pdu.h > > >> > @@ -183,8 +183,6 @@ struct smb2_symlink_err_rsp { > > >> > > > >> > #define SMB2_CLIENT_GUID_SIZE 16 > > >> > > > >> > -extern __u8 cifs_client_guid[SMB2_CLIENT_GUID_SIZE]; > > >> > - > > >> > struct smb2_negotiate_req { > > >> > struct smb2_hdr hdr; > > >> > __le16 StructureSize; /* Must be 36 */ > > >> > -- > > >> > 1.8.4.2 > > >> > > > >> > -- > > >> > 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 > > >> > > >> > > >> > > >> -- > > >> Thanks, > > >> > > >> Steve > > >> -- > > >> 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 > > > > > > > > -- > > Thanks, > > > > Steve > N r y b X ǧv ^ ){.n + { r' {ay ʇڙ ,j f h z w j:+v w j m zZ+ ݢj" > ! i ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
