Patchwork [06/11] cifs: eliminate "first_time" parm to CIFS_SessSetup

login
register
mail settings
Submitter Jeff Layton
Date April 24, 2010, 11:57 a.m.
Message ID <1272110272-20686-7-git-send-email-jlayton@redhat.com>
Download mbox | patch
Permalink /patch/50890/
State New
Headers show

Comments

Jeff Layton - April 24, 2010, 11:57 a.m.
We can use the is_first_ses_reconnect() function to determine this.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/connect.c   |    4 +---
 fs/cifs/sess.c      |   21 ++++++++++++++-------
 3 files changed, 15 insertions(+), 11 deletions(-)
Steve French - April 27, 2010, 2:45 a.m.
On Sat, Apr 24, 2010 at 6:57 AM, Jeff Layton <jlayton@redhat.com> wrote:
> We can use the is_first_ses_reconnect() function to determine this.

This is a slight behavior change - we will end up issuing
another SMB negprot after reconnect (which we didn't do before).

May be harmless, but worth thinking through
Jeff Layton - April 27, 2010, 11:03 a.m.
On Mon, 26 Apr 2010 21:45:53 -0500
Steve French <smfrench@gmail.com> wrote:

> On Sat, Apr 24, 2010 at 6:57 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > We can use the is_first_ses_reconnect() function to determine this.
> 
> This is a slight behavior change - we will end up issuing
> another SMB negprot after reconnect (which we didn't do before).
> 
> May be harmless, but worth thinking through
> 
> 

The comment in cifs_setup_session is misleading. I don't think there's
a behavior change. cifs_reconnect does this:

    server->maxBuf = 0;

...and that triggers a new negprot on reconnect in the current
code.
Jeff Layton - April 27, 2010, 11:20 a.m.
On Tue, 27 Apr 2010 07:03:20 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Mon, 26 Apr 2010 21:45:53 -0500
> Steve French <smfrench@gmail.com> wrote:
> 
> > On Sat, Apr 24, 2010 at 6:57 AM, Jeff Layton <jlayton@redhat.com> wrote:
> > > We can use the is_first_ses_reconnect() function to determine this.
> > 
> > This is a slight behavior change - we will end up issuing
> > another SMB negprot after reconnect (which we didn't do before).
> > 
> > May be harmless, but worth thinking through
> > 
> > 
> 
> The comment in cifs_setup_session is misleading. I don't think there's
> a behavior change. cifs_reconnect does this:
> 
>     server->maxBuf = 0;
> 
> ...and that triggers a new negprot on reconnect in the current
> code.

...and I think the current behavior is correct according to the spec.
The MS-CIFS doc says:

"An SMB_COM_NEGOTIATE exchange MUST be completed before any other SMB
messages MAY be sent to the server."

...since the server doesn't have any idea that this is a reconnect, I'd
assume that some (most? all?) servers would reject other SMB's on a
socket that hasn't had a negprot done on it.
Steve French - April 28, 2010, 12:36 a.m.
On Tue, Apr 27, 2010 at 6:20 AM, Jeff Layton <jlayton@redhat.com> wrote:
> On Tue, 27 Apr 2010 07:03:20 -0400
> Jeff Layton <jlayton@redhat.com> wrote:
>
>> On Mon, 26 Apr 2010 21:45:53 -0500
>> Steve French <smfrench@gmail.com> wrote:
>>
>> > On Sat, Apr 24, 2010 at 6:57 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> > > We can use the is_first_ses_reconnect() function to determine this.
>> >
>> > This is a slight behavior change - we will end up issuing
>> > another SMB negprot after reconnect (which we didn't do before).
>> >
>> > May be harmless, but worth thinking through
>> >
>> >
>>
>> The comment in cifs_setup_session is misleading. I don't think there's
>> a behavior change. cifs_reconnect does this:
>>
>>     server->maxBuf = 0;
>>
>> ...and that triggers a new negprot on reconnect in the current
>> code.
>
> ...and I think the current behavior is correct according to the spec.
> The MS-CIFS doc says:
>
> "An SMB_COM_NEGOTIATE exchange MUST be completed before any other SMB
> messages MAY be sent to the server."
>
> ...since the server doesn't have any idea that this is a reconnect, I'd
> assume that some (most? all?) servers would reject other SMB's on a
> socket that hasn't had a negprot done on it.

Your explanation makes sense - merged.

Patch

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index cc622a7..8e92142 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -95,7 +95,6 @@  extern int small_smb_init_no_tc(const int smb_cmd, const int wct,
 				struct cifsSesInfo *ses,
 				void **request_buf);
 extern int CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
-			     const int stage,
 			     const struct nls_table *nls_cp);
 extern __u16 GetNextMid(struct TCP_Server_Info *server);
 extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index c2793bd..49a7358 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2878,7 +2878,6 @@  int cifs_setup_session(unsigned int xid, struct cifsSesInfo *pSesInfo,
 					   struct nls_table *nls_info)
 {
 	int rc = 0;
-	int first_time = 0;
 	struct TCP_Server_Info *server = pSesInfo->server;
 
 	/* what if server changes its buffer size after dropping the session? */
@@ -2899,7 +2898,6 @@  int cifs_setup_session(unsigned int xid, struct cifsSesInfo *pSesInfo,
 			spin_unlock(&GlobalMid_Lock);
 
 		}
-		first_time = 1;
 	}
 
 	if (rc)
@@ -2913,7 +2911,7 @@  int cifs_setup_session(unsigned int xid, struct cifsSesInfo *pSesInfo,
 	cFYI(1, "Security Mode: 0x%x Capabilities: 0x%x TimeAdjust: %d",
 		 server->secMode, server->capabilities, server->timeAdj);
 
-	rc = CIFS_SessSetup(xid, pSesInfo, first_time, nls_info);
+	rc = CIFS_SessSetup(xid, pSesInfo, nls_info);
 	if (rc) {
 		cERROR(1, "Send error in SessSetup = %d", rc);
 	} else {
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index da9729d..84b92df 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -35,9 +35,11 @@ 
 extern void SMBNTencrypt(unsigned char *passwd, unsigned char *c8,
 			 unsigned char *p24);
 
-/* 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 */
+/*
+ * 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 cifsSesInfo *ses)
 {
 	struct list_head *tmp;
@@ -447,7 +449,7 @@  static void build_ntlmssp_negotiate_blob(unsigned char *pbuffer,
    This function returns the length of the data in the blob */
 static int build_ntlmssp_auth_blob(unsigned char *pbuffer,
 				   struct cifsSesInfo *ses,
-				   const struct nls_table *nls_cp, int first)
+				   const struct nls_table *nls_cp, bool first)
 {
 	AUTHENTICATE_MESSAGE *sec_blob = (AUTHENTICATE_MESSAGE *)pbuffer;
 	__u32 flags;
@@ -546,7 +548,7 @@  static void setup_ntlmssp_neg_req(SESSION_SETUP_ANDX *pSMB,
 
 static int setup_ntlmssp_auth_req(SESSION_SETUP_ANDX *pSMB,
 				  struct cifsSesInfo *ses,
-				  const struct nls_table *nls, int first_time)
+				  const struct nls_table *nls, bool first_time)
 {
 	int bloblen;
 
@@ -559,8 +561,8 @@  static int setup_ntlmssp_auth_req(SESSION_SETUP_ANDX *pSMB,
 #endif
 
 int
-CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses, int first_time,
-		const struct nls_table *nls_cp)
+CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses,
+	       const struct nls_table *nls_cp)
 {
 	int rc = 0;
 	int wct;
@@ -577,10 +579,15 @@  CIFS_SessSetup(unsigned int xid, struct cifsSesInfo *ses, int first_time,
 	int bytes_remaining;
 	struct key *spnego_key = NULL;
 	__le32 phase = NtLmNegotiate; /* NTLMSSP, if needed, is multistage */
+	bool first_time;
 
 	if (ses == NULL)
 		return -EINVAL;
 
+	read_lock(&cifs_tcp_ses_lock);
+	first_time = is_first_ses_reconnect(ses);
+	read_unlock(&cifs_tcp_ses_lock);
+
 	type = ses->server->secType;
 
 	cFYI(1, "sess setup type %d", type);