Thanks Jeff. Sure, noted.
Regards,
Shirish
On Fri, Sep 6, 2013 at 8:11 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> On Thu, 29 Aug 2013 08:35:09 -0500
> Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
>
>> Move the post (successful) session setup code to respective dialect routines.
>>
>> For smb1, session key is per smb connection.
>> For smb2/smb3, session key is per smb session.
>>
>> If client and server do not require signing, free session key for smb1/2/3.
>>
>> If client and server require signing
>> smb1 - Copy (kmemdup) session key for the first session to connection.
>> Free session key of that and subsequent sessions on this connection.
>> smb2 - For every session, keep the session key and free it when the
>> session is being shutdown.
>> smb3 - For every session, generate the smb3 signing key using the session key
>> and then free the session key.
>>
>> There are two unrelated line formatting changes as well.
>> ---
>> fs/cifs/connect.c | 27 +--------------------------
>> fs/cifs/misc.c | 1 +
>> fs/cifs/sess.c | 40 +++++++++++++++++++++++++++++++++++++---
>> fs/cifs/smb2pdu.c | 31 +++++++++++++++++++++++++++++++
>> 4 files changed, 70 insertions(+), 29 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index d67c550..84a7bde 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -3826,33 +3826,8 @@ cifs_setup_session(const unsigned int xid, struct cifs_ses *ses,
>> if (server->ops->sess_setup)
>> rc = server->ops->sess_setup(xid, ses, nls_info);
>>
>> - if (rc) {
>> + if (rc)
>> cifs_dbg(VFS, "Send error in SessSetup = %d\n", rc);
>> - } else {
>> - mutex_lock(&server->srv_mutex);
>> - if (!server->session_estab) {
>> - server->session_key.response = ses->auth_key.response;
>> - server->session_key.len = ses->auth_key.len;
>> - server->sequence_number = 0x2;
>> - server->session_estab = true;
>> - ses->auth_key.response = NULL;
>> - if (server->ops->generate_signingkey)
>> - server->ops->generate_signingkey(server);
>> - }
>> - mutex_unlock(&server->srv_mutex);
>> -
>> - cifs_dbg(FYI, "CIFS Session Established successfully\n");
>> - spin_lock(&GlobalMid_Lock);
>> - ses->status = CifsGood;
>> - ses->need_reconnect = false;
>> - spin_unlock(&GlobalMid_Lock);
>> - }
>> -
>> - kfree(ses->auth_key.response);
>> - ses->auth_key.response = NULL;
>> - ses->auth_key.len = 0;
>> - kfree(ses->ntlmssp);
>> - ses->ntlmssp = NULL;
>>
>> return rc;
>> }
>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
>> index f7d4b22..82a2b9f 100644
>> --- a/fs/cifs/misc.c
>> +++ b/fs/cifs/misc.c
>> @@ -105,6 +105,7 @@ sesInfoFree(struct cifs_ses *buf_to_free)
>> }
>> kfree(buf_to_free->user_name);
>> kfree(buf_to_free->domainName);
>> + kfree(buf_to_free->auth_key.response);
>> kfree(buf_to_free);
>> }
>>
>> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
>> index 08dd37b..7afd54a 100644
>> --- a/fs/cifs/sess.c
>> +++ b/fs/cifs/sess.c
>> @@ -629,7 +629,8 @@ CIFS_SessSetup(const unsigned int xid, struct cifs_ses *ses,
>> type = select_sectype(ses->server, ses->sectype);
>> cifs_dbg(FYI, "sess setup type %d\n", type);
>> if (type == Unspecified) {
>> - cifs_dbg(VFS, "Unable to select appropriate authentication method!");
>> + cifs_dbg(VFS,
>> + "Unable to select appropriate authentication method!");
>> return -EINVAL;
>> }
>>
>> @@ -815,8 +816,9 @@ ssetup_ntlmssp_authenticate:
>> ses->auth_key.response = kmemdup(msg->data, msg->sesskey_len,
>> GFP_KERNEL);
>> if (!ses->auth_key.response) {
>> - cifs_dbg(VFS, "Kerberos can't allocate (%u bytes) memory",
>> - msg->sesskey_len);
>> + cifs_dbg(VFS,
>> + "Kerberos can't allocate (%u bytes) memory",
>> + msg->sesskey_len);
>> rc = -ENOMEM;
>> goto ssetup_exit;
>> }
>> @@ -1005,5 +1007,37 @@ ssetup_exit:
>> if ((phase == NtLmChallenge) && (rc == 0))
>> goto ssetup_ntlmssp_authenticate;
>>
>> + if (!rc) {
>> + mutex_lock(&ses->server->srv_mutex);
>> + if (!ses->server->session_estab) {
>> + if (ses->server->sign) {
>> + ses->server->session_key.response =
>> + kmemdup(ses->auth_key.response,
>> + ses->auth_key.len, GFP_KERNEL);
>> + if (!ses->server->session_key.response) {
>> + rc = -ENOMEM;
>> + mutex_unlock(&ses->server->srv_mutex);
>> + goto keycp_exit;
>> + }
>> + ses->server->session_key.len =
>> + ses->auth_key.len;
>> + }
>> + ses->server->sequence_number = 0x2;
>> + ses->server->session_estab = true;
>> + }
>> + mutex_unlock(&ses->server->srv_mutex);
>> +
>> + cifs_dbg(FYI, "CIFS session established successfully\n");
>> + spin_lock(&GlobalMid_Lock);
>> + ses->status = CifsGood;
>> + ses->need_reconnect = false;
>> + spin_unlock(&GlobalMid_Lock);
>> + }
>> +
>> +keycp_exit:
>> + kfree(ses->auth_key.response);
>> + ses->auth_key.response = NULL;
>> + kfree(ses->ntlmssp);
>> +
>> return rc;
>> }
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index abc9c28..05a0186 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -478,6 +478,13 @@ SMB2_sess_setup(const unsigned int xid, struct cifs_ses *ses,
>> }
>>
>> /*
>> + * If we are here due to reconnect, free per-smb session key
>> + * in case signing was required.
>> + */
>> + kfree(ses->auth_key.response);
>> + ses->auth_key.response = NULL;
>> +
>> + /*
>> * If memory allocation is successful, caller of this function
>> * frees it.
>> */
>> @@ -628,6 +635,30 @@ ssetup_exit:
>> /* if ntlmssp, and negotiate succeeded, proceed to authenticate phase */
>> if ((phase == NtLmChallenge) && (rc == 0))
>> goto ssetup_ntlmssp_authenticate;
>> +
>> + if (!rc) {
>> + mutex_lock(&server->srv_mutex);
>> + if (!server->session_estab) {
>> + server->sequence_number = 0x2;
>> + server->session_estab = true;
>> + if (server->ops->generate_signingkey)
>> + server->ops->generate_signingkey(server);
>> + }
>> + mutex_unlock(&server->srv_mutex);
>> +
>> + cifs_dbg(FYI, "SMB2/3 session established successfully\n");
>> + spin_lock(&GlobalMid_Lock);
>> + ses->status = CifsGood;
>> + ses->need_reconnect = false;
>> + spin_unlock(&GlobalMid_Lock);
>> + }
>> +
>> + if (!server->sign) {
>> + kfree(ses->auth_key.response);
>> + ses->auth_key.response = NULL;
>> + }
>> + kfree(ses->ntlmssp);
>> +
>> return rc;
>> }
>>
>
> Nice cleanup. I think this looks reasonable.
>
> Another possible cleanup in the future might be to get rid of the
> ses->ntlmssp field. Since it's only used during session setup, it would
> probably be better to pass that in as a separate parm, but that can and
> should be done in a later patch.
>
> --
> Jeff Layton <jlayton@xxxxxxxxx>
--
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
