On Mon, 25 Oct 2010 12:39:54 -0500
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Mon, Oct 25, 2010 at 12:13 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> > On Mon, 25 Oct 2010 11:26:53 -0500
> > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> >
> >> On Mon, Oct 25, 2010 at 11:14 AM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> >> > On Thu, 21 Oct 2010 06:42:55 -0500
> >> > shirishpargaonkar@xxxxxxxxx wrote:
> >> >
> >> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> >>
> >> >>
> >> >> Start calculating auth response within a session. Move/Add pertinet
> >> >> data structures like session key, server challenge and ntlmv2_hash in
> >> >> a session structure. We should do the calculations within a session
> >> >> before copying session key and response over to server data
> >> >> structures because a session setup can fail.
> >> >>
> >> >> Only after a very first smb session succeeds, it copy/make its
> >> >> session key, session key of smb connection. This key stays with
> >> >> the smb connection throughout its life.
> >> >> sequence_number within server is set to 0x2.
> >> >>
> >> >> The authentication Message Authentication Key (mak) which consists
> >> >> of session key followed by client response within structure session_key
> >> >> is now dynamic. Every authentication type allocates the key + response
> >> >> sized memory within its session structure and later either assigns or
> >> >> frees it once the client response is sent and if session's session key
> >> >> becomes connetion's session key.
> >> >>
> >> >> ntlm/ntlmi authentication functions are rearranged. A function
> >> >> named setup_ntlm_resp(), similar to setup_ntlmv2_resp(), replaces
> >> >> function cifs_calculate_session_key().
> >> >>
> >> >> size of CIFS_SESS_KEY_SIZE is changed to 16, to reflect the byte size
> >> >> of the key it holds.
> >> >>
> >> >>
> >> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> >> ---
> >> >> fs/cifs/cifsencrypt.c | 113 ++++++++++++++++++++++++++++---------------------
> >> >> fs/cifs/cifsglob.h | 9 +----
> >> >> fs/cifs/cifspdu.h | 7 +++-
> >> >> fs/cifs/cifsproto.h | 11 ++---
> >> >> fs/cifs/connect.c | 18 +++++++-
> >> >> fs/cifs/sess.c | 107 +++++++++++++++++-----------------------------
> >> >> fs/cifs/transport.c | 6 +-
> >> >> 7 files changed, 134 insertions(+), 137 deletions(-)
> >> >>
> >> >
> >> > [...]
> >> >
> >> >> @@ -3178,10 +3185,11 @@ int cifs_setup_session(unsigned int xid, struct cifsSesInfo *ses,
> >> >> } else {
> >> >> mutex_lock(&ses->server->srv_mutex);
> >> >> if (!server->session_estab) {
> >> >> - memcpy(&server->session_key.data,
> >> >> - &ses->auth_key.data, ses->auth_key.len);
> >> >> + server->session_key.response = ses->auth_key.response;
> >> >> server->session_key.len = ses->auth_key.len;
> >> >> - ses->server->session_estab = true;
> >> >> + server->sequence_number = 0x2;
> >> >
> >> > It looks like the sequence_number is protected by the Global_MidLock.
> >> > It's not held here. Are there potential races with setting this value
> >> > here?
> >> >
> >>
> >> Jeff, I do not think so. This setting is happening only during the very first
> >> smb session setup for that smb connection. Rest of the smb sessions
> >> and subsequent smb traffic going on on that smb connection for all the
> >> smb sessions ensues only after this step i.e. for signature calculations.
> >> So there is no resetting sequence_number by doing this.
> >> If signing is not enabled, this is not even an issue at all.
> >>
> >
> > I'm not sure I follow...
> >
> > The Global_MidLock is taken in the other places where this counter is
> > incremented. Why is it OK to set it here outside of that lock?
> >
> > Isn't it possible for two session setups to proceed in parallel? If
> > not, what serializes them?
> >
> > --
> > Jeff Layton <jlayton@xxxxxxxxx>
> >
>
> Jeff, for a brand new smb connection, if the session connections race, they
> are serialized on srv_mutex cifs_setup_session. Only one session setup grabs
> that lock does change server's sequene number, none of the ones that follow can
> reset sequence_number because session_estab is true now.
Ok. I think the locking around the sequence number could stand some
improvement. It seems a bit complex -- maybe a simple atomic counter
would be more appropriate? In any case, it's probably better to leave
that for a later patch.
You can add my reviewed-by on this one. I don't see any obvious
problems now.
--
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
