[PATCH v3 0/7] transport/session code move to ops struct

[PATCH v3 0/7] transport/session code move to ops struct

[Pavel Shilovsky]

This is the regular patchset that makes the existing cifs code work through
ops struct of the server.

The version #3 consists the S_OP code that makes the code execution safer where
we need. Also it is not targeted to use for all ops struct call - it is much
better to opses in some critical place than having something unprepictable
(writeback, transport, credits, etc).

Pavel Shilovsky (7):
  CIFS: Introduce S_OP/S_OPV macros for a safe access to ops struct
    field
  CIFS: Move get_next_mid to ops struct
  CIFS: Move trans2 processing to ops struct
  CIFS: Extend credit mechanism to process request type
  CIFS: Move protocol specific negotiate code to ops struct
  CIFS: Move protocol specific session setup/logoff code to ops struct
  CIFS: Move protocol specific tcon/tdis code to ops struct

 fs/cifs/cifsglob.h  |   61 ++++++++++-
 fs/cifs/cifsproto.h |   21 ++--
 fs/cifs/cifssmb.c   |   24 ++--
 fs/cifs/connect.c   |  240 +++++++----------------------------------
 fs/cifs/misc.c      |   89 +---------------
 fs/cifs/sess.c      |    4 +-
 fs/cifs/smb1ops.c   |  298 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/cifs/transport.c |   39 ++++---
 8 files changed, 435 insertions(+), 341 deletions(-)

[PATCH v3 1/7] CIFS: Introduce S_OP/S_OPV macros for a safe access to ops struct field

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h  |   18 ++++++++++++++++++
 fs/cifs/connect.c   |    4 ++--
 fs/cifs/transport.c |    3 +--
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 20350a9..48d1009 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -367,6 +367,24 @@ struct TCP_Server_Info {
 #endif
 };
 
+/*
+ * The macro is used for safe access to ops field.
+ *
+ * @server - pointer to the server.
+ * @op - name of ops field.
+ * @miss_code - return code.
+ * @... - ops field arguments.
+ *
+ * Return miss_code if server->ops doesn't have op field. Otherwise - return
+ * servr->ops->op(...).
+ */
+
+#define S_OP(server, op, miss_code, ...) \
+	(server->ops->op ? server->ops->op(__VA_ARGS__) : miss_code)
+
+/* Void returned version of S_OP macro */
+#define S_OPV(server, op, ...) S_OP(server, op, 0, __VA_ARGS__)
+
 static inline unsigned int
 in_flight(struct TCP_Server_Info *server)
 {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index ccafded..a849aca 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1058,13 +1058,13 @@ cifs_demultiplex_thread(void *p)
 		if (mid_entry != NULL) {
 			if (!mid_entry->multiRsp || mid_entry->multiEnd)
 				mid_entry->callback(mid_entry);
-		} else if (!server->ops->is_oplock_break(buf, server)) {
+		} else if (!S_OP(server, is_oplock_break, false, buf, server)) {
 			cERROR(1, "No task to wake, unknown frame received! "
 				   "NumMids %d", atomic_read(&midCount));
 			cifs_dump_mem("Received Data is: ", buf,
 				      HEADER_SIZE(server));
 #ifdef CONFIG_CIFS_DEBUG2
-			server->ops->dump_detail(buf);
+			S_OPV(server, dump_detail, buf);
 			cifs_dump_mids(server);
 #endif /* CIFS_DEBUG2 */
 
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 1b36ffe..81d68f4 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -487,8 +487,7 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
 static inline int
 send_cancel(struct TCP_Server_Info *server, void *buf, struct mid_q_entry *mid)
 {
-	return server->ops->send_cancel ?
-				server->ops->send_cancel(server, buf, mid) : 0;
+	return S_OP(server, send_cancel, 0, server, buf, mid);
 }
 
 int
-- 
1.7.1

[PATCH v3 2/7] CIFS: Move get_next_mid to ops struct

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    7 ++++
 fs/cifs/cifsproto.h |    1 -
 fs/cifs/cifssmb.c   |    8 ++--
 fs/cifs/connect.c   |    2 +-
 fs/cifs/misc.c      |   89 +--------------------------------------------------
 fs/cifs/smb1ops.c   |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/transport.c |    2 +-
 7 files changed, 103 insertions(+), 95 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 48d1009..a660577 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -174,6 +174,7 @@ struct smb_version_operations {
 	void (*add_credits)(struct TCP_Server_Info *, const unsigned int);
 	void (*set_credits)(struct TCP_Server_Info *, const int);
 	int * (*get_credits_field)(struct TCP_Server_Info *);
+	__u64 (*get_next_mid)(struct TCP_Server_Info *);
 	/* data offset from read response message */
 	unsigned int (*read_data_offset)(char *);
 	/* data length from read response message */
@@ -417,6 +418,12 @@ set_credits(struct TCP_Server_Info *server, const int val)
 	server->ops->set_credits(server, val);
 }
 
+static inline __u64
+get_next_mid(struct TCP_Server_Info *server)
+{
+	return server->ops->get_next_mid(server);
+}
+
 /*
  * Macros to allow the TCP_Server_Info->net field and related code to drop out
  * when CONFIG_NET_NS isn't set.
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 5ec21ec..0a6cbfe 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -114,7 +114,6 @@ extern int small_smb_init_no_tc(const int smb_cmd, const int wct,
 				void **request_buf);
 extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses,
 			     const struct nls_table *nls_cp);
-extern __u64 GetNextMid(struct TCP_Server_Info *server);
 extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
 extern u64 cifs_UnixTimeToNT(struct timespec);
 extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index b5ad716..5b40073 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -268,7 +268,7 @@ small_smb_init_no_tc(const int smb_command, const int wct,
 		return rc;
 
 	buffer = (struct smb_hdr *)*request_buf;
-	buffer->Mid = GetNextMid(ses->server);
+	buffer->Mid = get_next_mid(ses->server);
 	if (ses->capabilities & CAP_UNICODE)
 		buffer->Flags2 |= SMBFLG2_UNICODE;
 	if (ses->capabilities & CAP_STATUS32)
@@ -402,7 +402,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses)
 
 	cFYI(1, "secFlags 0x%x", secFlags);
 
-	pSMB->hdr.Mid = GetNextMid(server);
+	pSMB->hdr.Mid = get_next_mid(server);
 	pSMB->hdr.Flags2 |= (SMBFLG2_UNICODE | SMBFLG2_ERR_STATUS);
 
 	if ((secFlags & CIFSSEC_MUST_KRB5) == CIFSSEC_MUST_KRB5)
@@ -782,7 +782,7 @@ CIFSSMBLogoff(const int xid, struct cifs_ses *ses)
 		return rc;
 	}
 
-	pSMB->hdr.Mid = GetNextMid(ses->server);
+	pSMB->hdr.Mid = get_next_mid(ses->server);
 
 	if (ses->server->sec_mode &
 		   (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED))
@@ -4762,7 +4762,7 @@ getDFSRetry:
 
 	/* server pointer checked in called function,
 	but should never be null here anyway */
-	pSMB->hdr.Mid = GetNextMid(ses->server);
+	pSMB->hdr.Mid = get_next_mid(ses->server);
 	pSMB->hdr.Tid = ses->ipc_tid;
 	pSMB->hdr.Uid = ses->Suid;
 	if (ses->capabilities & CAP_STATUS32)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a849aca..9362f90 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3938,7 +3938,7 @@ CIFSTCon(unsigned int xid, struct cifs_ses *ses,
 	header_assemble(smb_buffer, SMB_COM_TREE_CONNECT_ANDX,
 			NULL /*no tid */ , 4 /*wct */ );
 
-	smb_buffer->Mid = GetNextMid(ses->server);
+	smb_buffer->Mid = get_next_mid(ses->server);
 	smb_buffer->Uid = ses->Suid;
 	pSMB = (TCONX_REQ *) smb_buffer;
 	pSMBr = (TCONX_RSP *) smb_buffer_response;
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index e2552d2..557506a 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -212,93 +212,6 @@ cifs_small_buf_release(void *buf_to_free)
 	return;
 }
 
-/*
- * Find a free multiplex id (SMB mid). Otherwise there could be
- * mid collisions which might cause problems, demultiplexing the
- * wrong response to this request. Multiplex ids could collide if
- * one of a series requests takes much longer than the others, or
- * if a very large number of long lived requests (byte range
- * locks or FindNotify requests) are pending. No more than
- * 64K-1 requests can be outstanding at one time. If no
- * mids are available, return zero. A future optimization
- * could make the combination of mids and uid the key we use
- * to demultiplex on (rather than mid alone).
- * In addition to the above check, the cifs demultiplex
- * code already used the command code as a secondary
- * check of the frame and if signing is negotiated the
- * response would be discarded if the mid were the same
- * but the signature was wrong. Since the mid is not put in the
- * pending queue until later (when it is about to be dispatched)
- * we do have to limit the number of outstanding requests
- * to somewhat less than 64K-1 although it is hard to imagine
- * so many threads being in the vfs at one time.
- */
-__u64 GetNextMid(struct TCP_Server_Info *server)
-{
-	__u64 mid = 0;
-	__u16 last_mid, cur_mid;
-	bool collision;
-
-	spin_lock(&GlobalMid_Lock);
-
-	/* mid is 16 bit only for CIFS/SMB */
-	cur_mid = (__u16)((server->CurrentMid) & 0xffff);
-	/* we do not want to loop forever */
-	last_mid = cur_mid;
-	cur_mid++;
-
-	/*
-	 * This nested loop looks more expensive than it is.
-	 * In practice the list of pending requests is short,
-	 * fewer than 50, and the mids are likely to be unique
-	 * on the first pass through the loop unless some request
-	 * takes longer than the 64 thousand requests before it
-	 * (and it would also have to have been a request that
-	 * did not time out).
-	 */
-	while (cur_mid != last_mid) {
-		struct mid_q_entry *mid_entry;
-		unsigned int num_mids;
-
-		collision = false;
-		if (cur_mid == 0)
-			cur_mid++;
-
-		num_mids = 0;
-		list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
-			++num_mids;
-			if (mid_entry->mid == cur_mid &&
-			    mid_entry->mid_state == MID_REQUEST_SUBMITTED) {
-				/* This mid is in use, try a different one */
-				collision = true;
-				break;
-			}
-		}
-
-		/*
-		 * if we have more than 32k mids in the list, then something
-		 * is very wrong. Possibly a local user is trying to DoS the
-		 * box by issuing long-running calls and SIGKILL'ing them. If
-		 * we get to 2^16 mids then we're in big trouble as this
-		 * function could loop forever.
-		 *
-		 * Go ahead and assign out the mid in this situation, but force
-		 * an eventual reconnect to clean out the pending_mid_q.
-		 */
-		if (num_mids > 32768)
-			server->tcpStatus = CifsNeedReconnect;
-
-		if (!collision) {
-			mid = (__u64)cur_mid;
-			server->CurrentMid = mid;
-			break;
-		}
-		cur_mid++;
-	}
-	spin_unlock(&GlobalMid_Lock);
-	return mid;
-}
-
 /* NB: MID can not be set if treeCon not passed in, in that
    case it is responsbility of caller to set the mid */
 void
@@ -334,7 +247,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ ,
 
 			/* Uid is not converted */
 			buffer->Uid = treeCon->ses->Suid;
-			buffer->Mid = GetNextMid(treeCon->ses->server);
+			buffer->Mid = get_next_mid(treeCon->ses->server);
 		}
 		if (treeCon->Flags & SMB_SHARE_IS_IN_DFS)
 			buffer->Flags2 |= SMBFLG2_DFS;
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index d9d615f..6dec38f 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -125,6 +125,94 @@ cifs_get_credits_field(struct TCP_Server_Info *server)
 	return &server->credits;
 }
 
+/*
+ * Find a free multiplex id (SMB mid). Otherwise there could be
+ * mid collisions which might cause problems, demultiplexing the
+ * wrong response to this request. Multiplex ids could collide if
+ * one of a series requests takes much longer than the others, or
+ * if a very large number of long lived requests (byte range
+ * locks or FindNotify requests) are pending. No more than
+ * 64K-1 requests can be outstanding at one time. If no
+ * mids are available, return zero. A future optimization
+ * could make the combination of mids and uid the key we use
+ * to demultiplex on (rather than mid alone).
+ * In addition to the above check, the cifs demultiplex
+ * code already used the command code as a secondary
+ * check of the frame and if signing is negotiated the
+ * response would be discarded if the mid were the same
+ * but the signature was wrong. Since the mid is not put in the
+ * pending queue until later (when it is about to be dispatched)
+ * we do have to limit the number of outstanding requests
+ * to somewhat less than 64K-1 although it is hard to imagine
+ * so many threads being in the vfs at one time.
+ */
+static __u64
+cifs_get_next_mid(struct TCP_Server_Info *server)
+{
+	__u64 mid = 0;
+	__u16 last_mid, cur_mid;
+	bool collision;
+
+	spin_lock(&GlobalMid_Lock);
+
+	/* mid is 16 bit only for CIFS/SMB */
+	cur_mid = (__u16)((server->CurrentMid) & 0xffff);
+	/* we do not want to loop forever */
+	last_mid = cur_mid;
+	cur_mid++;
+
+	/*
+	 * This nested loop looks more expensive than it is.
+	 * In practice the list of pending requests is short,
+	 * fewer than 50, and the mids are likely to be unique
+	 * on the first pass through the loop unless some request
+	 * takes longer than the 64 thousand requests before it
+	 * (and it would also have to have been a request that
+	 * did not time out).
+	 */
+	while (cur_mid != last_mid) {
+		struct mid_q_entry *mid_entry;
+		unsigned int num_mids;
+
+		collision = false;
+		if (cur_mid == 0)
+			cur_mid++;
+
+		num_mids = 0;
+		list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
+			++num_mids;
+			if (mid_entry->mid == cur_mid &&
+			    mid_entry->mid_state == MID_REQUEST_SUBMITTED) {
+				/* This mid is in use, try a different one */
+				collision = true;
+				break;
+			}
+		}
+
+		/*
+		 * if we have more than 32k mids in the list, then something
+		 * is very wrong. Possibly a local user is trying to DoS the
+		 * box by issuing long-running calls and SIGKILL'ing them. If
+		 * we get to 2^16 mids then we're in big trouble as this
+		 * function could loop forever.
+		 *
+		 * Go ahead and assign out the mid in this situation, but force
+		 * an eventual reconnect to clean out the pending_mid_q.
+		 */
+		if (num_mids > 32768)
+			server->tcpStatus = CifsNeedReconnect;
+
+		if (!collision) {
+			mid = (__u64)cur_mid;
+			server->CurrentMid = mid;
+			break;
+		}
+		cur_mid++;
+	}
+	spin_unlock(&GlobalMid_Lock);
+	return mid;
+}
+
 struct smb_version_operations smb1_operations = {
 	.send_cancel = send_nt_cancel,
 	.compare_fids = cifs_compare_fids,
@@ -133,6 +221,7 @@ struct smb_version_operations smb1_operations = {
 	.add_credits = cifs_add_credits,
 	.set_credits = cifs_set_credits,
 	.get_credits_field = cifs_get_credits_field,
+	.get_next_mid = cifs_get_next_mid,
 	.read_data_offset = cifs_read_data_offset,
 	.read_data_length = cifs_read_data_length,
 	.map_error = map_smb_to_linux_error,
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 81d68f4..6fdb5be 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -778,7 +778,7 @@ send_lock_cancel(const unsigned int xid, struct cifs_tcon *tcon,
 
 	pSMB->LockType = LOCKING_ANDX_CANCEL_LOCK|LOCKING_ANDX_LARGE_FILES;
 	pSMB->Timeout = 0;
-	pSMB->hdr.Mid = GetNextMid(ses->server);
+	pSMB->hdr.Mid = get_next_mid(ses->server);
 
 	return SendReceive(xid, ses, in_buf, out_buf,
 			&bytes_returned, 0);
-- 
1.7.1

[PATCH v3 3/7] CIFS: Move trans2 processing to ops struct

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h |    3 +
 fs/cifs/connect.c  |  160 +------------------------------------------------
 fs/cifs/smb1ops.c  |  170 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 159 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a660577..3b8828b 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -187,6 +187,9 @@ struct smb_version_operations {
 	/* verify the message */
 	int (*check_message)(char *, unsigned int);
 	bool (*is_oplock_break)(char *, struct TCP_Server_Info *);
+	/* process transaction2 response */
+	bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
+			     char *, int);
 };
 
 struct smb_version_values {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 9362f90..dc96835 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -394,143 +394,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
 	return rc;
 }
 
-/*
-	return codes:
-		0 	not a transact2, or all data present
-		>0 	transact2 with that much data missing
-		-EINVAL = invalid transact2
-
- */
-static int check2ndT2(char *buf)
-{
-	struct smb_hdr *pSMB = (struct smb_hdr *)buf;
-	struct smb_t2_rsp *pSMBt;
-	int remaining;
-	__u16 total_data_size, data_in_this_rsp;
-
-	if (pSMB->Command != SMB_COM_TRANSACTION2)
-		return 0;
-
-	/* check for plausible wct, bcc and t2 data and parm sizes */
-	/* check for parm and data offset going beyond end of smb */
-	if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */
-		cFYI(1, "invalid transact2 word count");
-		return -EINVAL;
-	}
-
-	pSMBt = (struct smb_t2_rsp *)pSMB;
-
-	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
-	data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
-
-	if (total_data_size == data_in_this_rsp)
-		return 0;
-	else if (total_data_size < data_in_this_rsp) {
-		cFYI(1, "total data %d smaller than data in frame %d",
-			total_data_size, data_in_this_rsp);
-		return -EINVAL;
-	}
-
-	remaining = total_data_size - data_in_this_rsp;
-
-	cFYI(1, "missing %d bytes from transact2, check next response",
-		remaining);
-	if (total_data_size > CIFSMaxBufSize) {
-		cERROR(1, "TotalDataSize %d is over maximum buffer %d",
-			total_data_size, CIFSMaxBufSize);
-		return -EINVAL;
-	}
-	return remaining;
-}
-
-static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
-{
-	struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf;
-	struct smb_t2_rsp *pSMBt  = (struct smb_t2_rsp *)target_hdr;
-	char *data_area_of_tgt;
-	char *data_area_of_src;
-	int remaining;
-	unsigned int byte_count, total_in_tgt;
-	__u16 tgt_total_cnt, src_total_cnt, total_in_src;
-
-	src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount);
-	tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
-
-	if (tgt_total_cnt != src_total_cnt)
-		cFYI(1, "total data count of primary and secondary t2 differ "
-			"source=%hu target=%hu", src_total_cnt, tgt_total_cnt);
-
-	total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
-
-	remaining = tgt_total_cnt - total_in_tgt;
-
-	if (remaining < 0) {
-		cFYI(1, "Server sent too much data. tgt_total_cnt=%hu "
-			"total_in_tgt=%hu", tgt_total_cnt, total_in_tgt);
-		return -EPROTO;
-	}
-
-	if (remaining == 0) {
-		/* nothing to do, ignore */
-		cFYI(1, "no more data remains");
-		return 0;
-	}
-
-	total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount);
-	if (remaining < total_in_src)
-		cFYI(1, "transact2 2nd response contains too much data");
-
-	/* find end of first SMB data area */
-	data_area_of_tgt = (char *)&pSMBt->hdr.Protocol +
-				get_unaligned_le16(&pSMBt->t2_rsp.DataOffset);
-
-	/* validate target area */
-	data_area_of_src = (char *)&pSMBs->hdr.Protocol +
-				get_unaligned_le16(&pSMBs->t2_rsp.DataOffset);
-
-	data_area_of_tgt += total_in_tgt;
-
-	total_in_tgt += total_in_src;
-	/* is the result too big for the field? */
-	if (total_in_tgt > USHRT_MAX) {
-		cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt);
-		return -EPROTO;
-	}
-	put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
-
-	/* fix up the BCC */
-	byte_count = get_bcc(target_hdr);
-	byte_count += total_in_src;
-	/* is the result too big for the field? */
-	if (byte_count > USHRT_MAX) {
-		cFYI(1, "coalesced BCC too large (%u)", byte_count);
-		return -EPROTO;
-	}
-	put_bcc(byte_count, target_hdr);
-
-	byte_count = be32_to_cpu(target_hdr->smb_buf_length);
-	byte_count += total_in_src;
-	/* don't allow buffer to overflow */
-	if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) {
-		cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count);
-		return -ENOBUFS;
-	}
-	target_hdr->smb_buf_length = cpu_to_be32(byte_count);
-
-	/* copy second buffer into end of first buffer */
-	memcpy(data_area_of_tgt, data_area_of_src, total_in_src);
-
-	if (remaining != total_in_src) {
-		/* more responses to go */
-		cFYI(1, "waiting for more secondary responses");
-		return 1;
-	}
-
-	/* we are done */
-	cFYI(1, "found the last secondary response");
-	return 0;
-}
-
 static void
 cifs_echo_request(struct work_struct *work)
 {
@@ -803,29 +666,8 @@ static void
 handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
 	   char *buf, int malformed)
 {
-	if (malformed == 0 && check2ndT2(buf) > 0) {
-		mid->multiRsp = true;
-		if (mid->resp_buf) {
-			/* merge response - fix up 1st*/
-			malformed = coalesce_t2(buf, mid->resp_buf);
-			if (malformed > 0)
-				return;
-
-			/* All parts received or packet is malformed. */
-			mid->multiEnd = true;
-			return dequeue_mid(mid, malformed);
-		}
-		if (!server->large_buf) {
-			/*FIXME: switch to already allocated largebuf?*/
-			cERROR(1, "1st trans2 resp needs bigbuf");
-		} else {
-			/* Have first buffer */
-			mid->resp_buf = buf;
-			mid->large_buf = true;
-			server->bigbuf = NULL;
-		}
+	if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
 		return;
-	}
 	mid->resp_buf = buf;
 	mid->large_buf = server->large_buf;
 	/* Was previous buf put in mpx struct for multi-rsp? */
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 6dec38f..28359e7 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -213,6 +213,175 @@ cifs_get_next_mid(struct TCP_Server_Info *server)
 	return mid;
 }
 
+/*
+	return codes:
+		0	not a transact2, or all data present
+		>0	transact2 with that much data missing
+		-EINVAL	invalid transact2
+ */
+static int
+check2ndT2(char *buf)
+{
+	struct smb_hdr *pSMB = (struct smb_hdr *)buf;
+	struct smb_t2_rsp *pSMBt;
+	int remaining;
+	__u16 total_data_size, data_in_this_rsp;
+
+	if (pSMB->Command != SMB_COM_TRANSACTION2)
+		return 0;
+
+	/* check for plausible wct, bcc and t2 data and parm sizes */
+	/* check for parm and data offset going beyond end of smb */
+	if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */
+		cFYI(1, "invalid transact2 word count");
+		return -EINVAL;
+	}
+
+	pSMBt = (struct smb_t2_rsp *)pSMB;
+
+	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
+	data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
+
+	if (total_data_size == data_in_this_rsp)
+		return 0;
+	else if (total_data_size < data_in_this_rsp) {
+		cFYI(1, "total data %d smaller than data in frame %d",
+			total_data_size, data_in_this_rsp);
+		return -EINVAL;
+	}
+
+	remaining = total_data_size - data_in_this_rsp;
+
+	cFYI(1, "missing %d bytes from transact2, check next response",
+		remaining);
+	if (total_data_size > CIFSMaxBufSize) {
+		cERROR(1, "TotalDataSize %d is over maximum buffer %d",
+			total_data_size, CIFSMaxBufSize);
+		return -EINVAL;
+	}
+	return remaining;
+}
+
+static int
+coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
+{
+	struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf;
+	struct smb_t2_rsp *pSMBt  = (struct smb_t2_rsp *)target_hdr;
+	char *data_area_of_tgt;
+	char *data_area_of_src;
+	int remaining;
+	unsigned int byte_count, total_in_tgt;
+	__u16 tgt_total_cnt, src_total_cnt, total_in_src;
+
+	src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount);
+	tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
+
+	if (tgt_total_cnt != src_total_cnt)
+		cFYI(1, "total data count of primary and secondary t2 differ "
+			"source=%hu target=%hu", src_total_cnt, tgt_total_cnt);
+
+	total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
+
+	remaining = tgt_total_cnt - total_in_tgt;
+
+	if (remaining < 0) {
+		cFYI(1, "Server sent too much data. tgt_total_cnt=%hu "
+			"total_in_tgt=%hu", tgt_total_cnt, total_in_tgt);
+		return -EPROTO;
+	}
+
+	if (remaining == 0) {
+		/* nothing to do, ignore */
+		cFYI(1, "no more data remains");
+		return 0;
+	}
+
+	total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount);
+	if (remaining < total_in_src)
+		cFYI(1, "transact2 2nd response contains too much data");
+
+	/* find end of first SMB data area */
+	data_area_of_tgt = (char *)&pSMBt->hdr.Protocol +
+				get_unaligned_le16(&pSMBt->t2_rsp.DataOffset);
+
+	/* validate target area */
+	data_area_of_src = (char *)&pSMBs->hdr.Protocol +
+				get_unaligned_le16(&pSMBs->t2_rsp.DataOffset);
+
+	data_area_of_tgt += total_in_tgt;
+
+	total_in_tgt += total_in_src;
+	/* is the result too big for the field? */
+	if (total_in_tgt > USHRT_MAX) {
+		cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt);
+		return -EPROTO;
+	}
+	put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
+
+	/* fix up the BCC */
+	byte_count = get_bcc(target_hdr);
+	byte_count += total_in_src;
+	/* is the result too big for the field? */
+	if (byte_count > USHRT_MAX) {
+		cFYI(1, "coalesced BCC too large (%u)", byte_count);
+		return -EPROTO;
+	}
+	put_bcc(byte_count, target_hdr);
+
+	byte_count = be32_to_cpu(target_hdr->smb_buf_length);
+	byte_count += total_in_src;
+	/* don't allow buffer to overflow */
+	if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) {
+		cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count);
+		return -ENOBUFS;
+	}
+	target_hdr->smb_buf_length = cpu_to_be32(byte_count);
+
+	/* copy second buffer into end of first buffer */
+	memcpy(data_area_of_tgt, data_area_of_src, total_in_src);
+
+	if (remaining != total_in_src) {
+		/* more responses to go */
+		cFYI(1, "waiting for more secondary responses");
+		return 1;
+	}
+
+	/* we are done */
+	cFYI(1, "found the last secondary response");
+	return 0;
+}
+
+static bool
+cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server,
+		  char *buf, int malformed)
+{
+	if (malformed)
+		return false;
+	if (check2ndT2(buf) <= 0)
+		return false;
+	mid->multiRsp = true;
+	if (mid->resp_buf) {
+		/* merge response - fix up 1st*/
+		malformed = coalesce_t2(buf, mid->resp_buf);
+		if (malformed > 0)
+			return true;
+		/* All parts received or packet is malformed. */
+		mid->multiEnd = true;
+		dequeue_mid(mid, malformed);
+		return true;
+	}
+	if (!server->large_buf) {
+		/*FIXME: switch to already allocated largebuf?*/
+		cERROR(1, "1st trans2 resp needs bigbuf");
+	} else {
+		/* Have first buffer */
+		mid->resp_buf = buf;
+		mid->large_buf = true;
+		server->bigbuf = NULL;
+	}
+	return true;
+}
+
 struct smb_version_operations smb1_operations = {
 	.send_cancel = send_nt_cancel,
 	.compare_fids = cifs_compare_fids,
@@ -229,6 +398,7 @@ struct smb_version_operations smb1_operations = {
 	.check_message = checkSMB,
 	.dump_detail = cifs_dump_detail,
 	.is_oplock_break = is_valid_oplock_break,
+	.check_trans2 = cifs_check_trans2,
 };
 
 struct smb_version_values smb1_values = {
-- 
1.7.1

[PATCH v3 4/7] CIFS: Extend credit mechanism to process request type

[Pavel Shilovsky]

Split all requests to echos, oplocks and others - each group uses
its own credit slot. This change is required to support SMB2 code.

As for CIFS - we don't use this approach and the logic isn't changed.

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h  |   14 ++++++++++----
 fs/cifs/cifsproto.h |    2 +-
 fs/cifs/cifssmb.c   |   12 ++++++------
 fs/cifs/smb1ops.c   |   12 ++++++++++--
 fs/cifs/transport.c |   34 +++++++++++++++++++---------------
 5 files changed, 46 insertions(+), 28 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 3b8828b..b0afcb6 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -171,9 +171,11 @@ struct smb_version_operations {
 	/* check response: verify signature, map error */
 	int (*check_receive)(struct mid_q_entry *, struct TCP_Server_Info *,
 			     bool);
-	void (*add_credits)(struct TCP_Server_Info *, const unsigned int);
+	void (*add_credits)(struct TCP_Server_Info *, const unsigned int,
+			    const int);
 	void (*set_credits)(struct TCP_Server_Info *, const int);
-	int * (*get_credits_field)(struct TCP_Server_Info *);
+	int * (*get_credits_field)(struct TCP_Server_Info *, const int);
+	unsigned int (*get_credits)(struct mid_q_entry *);
 	__u64 (*get_next_mid)(struct TCP_Server_Info *);
 	/* data offset from read response message */
 	unsigned int (*read_data_offset)(char *);
@@ -410,9 +412,10 @@ has_credits(struct TCP_Server_Info *server, int *credits)
 }
 
 static inline void
-add_credits(struct TCP_Server_Info *server, const unsigned int add)
+add_credits(struct TCP_Server_Info *server, const unsigned int add,
+	    const int optype)
 {
-	server->ops->add_credits(server, add);
+	server->ops->add_credits(server, add, optype);
 }
 
 static inline void
@@ -974,6 +977,9 @@ static inline void free_dfs_info_array(struct dfs_info3_param *param,
 #define   CIFS_LOG_ERROR    0x010    /* log NT STATUS if non-zero */
 #define   CIFS_LARGE_BUF_OP 0x020    /* large request buffer */
 #define   CIFS_NO_RESP      0x040    /* no response buffer required */
+#define   CIFS_ECHO_OP      0x080    /* echo request */
+#define   CIFS_OBREAK_OP   0x0100    /* oplock break request */
+#define   CIFS_REQ_MASK    0x0180    /* mask request type */
 
 /* Security Flags: indicate type of session setup needed */
 #define   CIFSSEC_MAY_SIGN	0x00001
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 0a6cbfe..41d5a76 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -71,7 +71,7 @@ extern void DeleteMidQEntry(struct mid_q_entry *midEntry);
 extern int cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 			   unsigned int nvec, mid_receive_t *receive,
 			   mid_callback_t *callback, void *cbdata,
-			   bool ignore_pend);
+			   const int optype);
 extern int SendReceive(const unsigned int /* xid */ , struct cifs_ses *,
 			struct smb_hdr * /* input */ ,
 			struct smb_hdr * /* out */ ,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 5b40073..65898cc 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -720,7 +720,7 @@ cifs_echo_callback(struct mid_q_entry *mid)
 	struct TCP_Server_Info *server = mid->callback_data;
 
 	DeleteMidQEntry(mid);
-	add_credits(server, 1);
+	add_credits(server, 1, 0);
 }
 
 int
@@ -747,7 +747,7 @@ CIFSSMBEcho(struct TCP_Server_Info *server)
 	iov.iov_len = be32_to_cpu(smb->hdr.smb_buf_length) + 4;
 
 	rc = cifs_call_async(server, &iov, 1, NULL, cifs_echo_callback,
-			     server, true);
+			     server, CIFS_ASYNC_OP);
 	if (rc)
 		cFYI(1, "Echo request failed: %d", rc);
 
@@ -1563,7 +1563,7 @@ cifs_readv_callback(struct mid_q_entry *mid)
 
 	queue_work(cifsiod_wq, &rdata->work);
 	DeleteMidQEntry(mid);
-	add_credits(server, 1);
+	add_credits(server, 1, 0);
 }
 
 /* cifs_async_readv - send an async write, and set up mid to handle result */
@@ -1619,7 +1619,7 @@ cifs_async_readv(struct cifs_readdata *rdata)
 	kref_get(&rdata->refcount);
 	rc = cifs_call_async(tcon->ses->server, rdata->iov, 1,
 			     cifs_readv_receive, cifs_readv_callback,
-			     rdata, false);
+			     rdata, 0);
 
 	if (rc == 0)
 		cifs_stats_inc(&tcon->num_reads);
@@ -2010,7 +2010,7 @@ cifs_writev_callback(struct mid_q_entry *mid)
 
 	queue_work(cifsiod_wq, &wdata->work);
 	DeleteMidQEntry(mid);
-	add_credits(tcon->ses->server, 1);
+	add_credits(tcon->ses->server, 1, 0);
 }
 
 /* cifs_async_writev - send an async write, and set up mid to handle result */
@@ -2090,7 +2090,7 @@ cifs_async_writev(struct cifs_writedata *wdata)
 
 	kref_get(&wdata->refcount);
 	rc = cifs_call_async(tcon->ses->server, iov, wdata->nr_pages + 1,
-			     NULL, cifs_writev_callback, wdata, false);
+			     NULL, cifs_writev_callback, wdata, 0);
 
 	if (rc == 0)
 		cifs_stats_inc(&tcon->num_writes);
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 28359e7..f4f8394 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -101,7 +101,8 @@ cifs_find_mid(struct TCP_Server_Info *server, char *buffer)
 }
 
 static void
-cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add)
+cifs_add_credits(struct TCP_Server_Info *server, const unsigned int add,
+		 const int optype)
 {
 	spin_lock(&server->req_lock);
 	server->credits += add;
@@ -120,11 +121,17 @@ cifs_set_credits(struct TCP_Server_Info *server, const int val)
 }
 
 static int *
-cifs_get_credits_field(struct TCP_Server_Info *server)
+cifs_get_credits_field(struct TCP_Server_Info *server, const int optype)
 {
 	return &server->credits;
 }
 
+static unsigned int
+cifs_get_credits(struct mid_q_entry *mid)
+{
+	return 1;
+}
+
 /*
  * Find a free multiplex id (SMB mid). Otherwise there could be
  * mid collisions which might cause problems, demultiplexing the
@@ -390,6 +397,7 @@ struct smb_version_operations smb1_operations = {
 	.add_credits = cifs_add_credits,
 	.set_credits = cifs_set_credits,
 	.get_credits_field = cifs_get_credits_field,
+	.get_credits = cifs_get_credits,
 	.get_next_mid = cifs_get_next_mid,
 	.read_data_offset = cifs_read_data_offset,
 	.read_data_length = cifs_read_data_length,
diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 6fdb5be..0497c65 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -305,7 +305,7 @@ static int
 wait_for_free_request(struct TCP_Server_Info *server, const int optype)
 {
 	return wait_for_free_credits(server, optype,
-				     server->ops->get_credits_field(server));
+				server->ops->get_credits_field(server, optype));
 }
 
 static int allocate_mid(struct cifs_ses *ses, struct smb_hdr *in_buf,
@@ -384,12 +384,12 @@ cifs_setup_async_request(struct TCP_Server_Info *server, struct kvec *iov,
 int
 cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 		unsigned int nvec, mid_receive_t *receive,
-		mid_callback_t *callback, void *cbdata, bool ignore_pend)
+		mid_callback_t *callback, void *cbdata, const int optype)
 {
 	int rc;
 	struct mid_q_entry *mid;
 
-	rc = wait_for_free_request(server, ignore_pend ? CIFS_ASYNC_OP : 0);
+	rc = wait_for_free_request(server, optype);
 	if (rc)
 		return rc;
 
@@ -397,7 +397,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 	rc = cifs_setup_async_request(server, iov, nvec, &mid);
 	if (rc) {
 		mutex_unlock(&server->srv_mutex);
-		add_credits(server, 1);
+		add_credits(server, 1, optype);
 		wake_up(&server->request_q);
 		return rc;
 	}
@@ -419,7 +419,7 @@ cifs_call_async(struct TCP_Server_Info *server, struct kvec *iov,
 	return rc;
 out_err:
 	delete_mid(mid);
-	add_credits(server, 1);
+	add_credits(server, 1, optype);
 	wake_up(&server->request_q);
 	return rc;
 }
@@ -538,11 +538,13 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	     const int flags)
 {
 	int rc = 0;
-	int long_op;
+	int long_op, optype;
 	struct mid_q_entry *midQ;
 	char *buf = iov[0].iov_base;
+	unsigned int credits = 1;
 
 	long_op = flags & CIFS_TIMEOUT_MASK;
+	optype = flags & CIFS_REQ_MASK;
 
 	*pRespBufType = CIFS_NO_BUFFER;  /* no response buf yet */
 
@@ -563,7 +565,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	 * use ses->maxReq.
 	 */
 
-	rc = wait_for_free_request(ses->server, long_op);
+	rc = wait_for_free_request(ses->server, optype);
 	if (rc) {
 		cifs_small_buf_release(buf);
 		return rc;
@@ -582,7 +584,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 		mutex_unlock(&ses->server->srv_mutex);
 		cifs_small_buf_release(buf);
 		/* Update # of requests on wire to server */
-		add_credits(ses->server, 1);
+		add_credits(ses->server, 1, optype);
 		return rc;
 	}
 
@@ -612,7 +614,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 			midQ->callback = DeleteMidQEntry;
 			spin_unlock(&GlobalMid_Lock);
 			cifs_small_buf_release(buf);
-			add_credits(ses->server, 1);
+			add_credits(ses->server, 1, optype);
 			return rc;
 		}
 		spin_unlock(&GlobalMid_Lock);
@@ -622,7 +624,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 
 	rc = cifs_sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
-		add_credits(ses->server, 1);
+		add_credits(ses->server, 1, optype);
 		return rc;
 	}
 
@@ -640,6 +642,8 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 	else
 		*pRespBufType = CIFS_SMALL_BUFFER;
 
+	credits = ses->server->ops->get_credits(midQ);
+
 	rc = ses->server->ops->check_receive(midQ, ses->server,
 					     flags & CIFS_LOG_ERROR);
 
@@ -648,7 +652,7 @@ SendReceive2(const unsigned int xid, struct cifs_ses *ses,
 		midQ->resp_buf = NULL;
 out:
 	delete_mid(midQ);
-	add_credits(ses->server, 1);
+	add_credits(ses->server, credits, optype);
 
 	return rc;
 }
@@ -698,7 +702,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
 	if (rc) {
 		mutex_unlock(&ses->server->srv_mutex);
 		/* Update # of requests on wire to server */
-		add_credits(ses->server, 1);
+		add_credits(ses->server, 1, 0);
 		return rc;
 	}
 
@@ -730,7 +734,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
 			/* no longer considered to be "in-flight" */
 			midQ->callback = DeleteMidQEntry;
 			spin_unlock(&GlobalMid_Lock);
-			add_credits(ses->server, 1);
+			add_credits(ses->server, 1, 0);
 			return rc;
 		}
 		spin_unlock(&GlobalMid_Lock);
@@ -738,7 +742,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
 
 	rc = cifs_sync_mid_result(midQ, ses->server);
 	if (rc != 0) {
-		add_credits(ses->server, 1);
+		add_credits(ses->server, 1, 0);
 		return rc;
 	}
 
@@ -754,7 +758,7 @@ SendReceive(const unsigned int xid, struct cifs_ses *ses,
 	rc = cifs_check_receive(midQ, ses->server, 0);
 out:
 	delete_mid(midQ);
-	add_credits(ses->server, 1);
+	add_credits(ses->server, 1, 0);
 
 	return rc;
 }
-- 
1.7.1

[PATCH v3 5/7] CIFS: Move protocol specific negotiate code to ops struct

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    8 ++++++--
 fs/cifs/cifsproto.h |    5 ++---
 fs/cifs/cifssmb.c   |    4 ++--
 fs/cifs/connect.c   |   18 ++++++------------
 fs/cifs/sess.c      |    2 +-
 fs/cifs/smb1ops.c   |   23 +++++++++++++++++++++++
 6 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index b0afcb6..30fe2ac 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -192,6 +192,10 @@ struct smb_version_operations {
 	/* process transaction2 response */
 	bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
 			     char *, int);
+	/* check if we need to negotiate */
+	bool (*need_neg)(struct TCP_Server_Info *);
+	/* negotiate to the server */
+	int (*negotiate)(const int, struct cifs_ses *);
 };
 
 struct smb_version_values {
@@ -324,7 +328,7 @@ struct TCP_Server_Info {
 	struct mutex srv_mutex;
 	struct task_struct *tsk;
 	char server_GUID[16];
-	char sec_mode;
+	__u16 sec_mode;
 	bool session_estab; /* mark when very first sess is established */
 	u16 dialect; /* dialect index that server chose */
 	enum securityEnum secType;
@@ -477,7 +481,7 @@ struct cifs_ses {
 	char *serverOS;		/* name of operating system underlying server */
 	char *serverNOS;	/* name of network operating system of server */
 	char *serverDomain;	/* security realm of server */
-	int Suid;		/* remote smb uid  */
+	__u64 Suid;		/* remote smb uid  */
 	uid_t linux_uid;        /* overriding owner of files on the mount */
 	uid_t cred_uid;		/* owner of credentials */
 	int capabilities;
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 41d5a76..5e8efd8 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -178,11 +178,10 @@ extern void cifs_dfs_release_automount_timer(void);
 void cifs_proc_init(void);
 void cifs_proc_clean(void);
 
-extern int cifs_negotiate_protocol(unsigned int xid,
-				  struct cifs_ses *ses);
+extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses);
 extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
 			struct nls_table *nls_info);
-extern int CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses);
+extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses);
 
 extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses,
 			const char *tree, struct cifs_tcon *tcon,
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 65898cc..751b9c8 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -372,7 +372,7 @@ static inline void inc_rfc1001_len(void *pSMB, int count)
 }
 
 int
-CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses)
+CIFSSMBNegotiate(const int xid, struct cifs_ses *ses)
 {
 	NEGOTIATE_REQ *pSMB;
 	NEGOTIATE_RSP *pSMBr;
@@ -456,7 +456,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses)
 			rc = -EOPNOTSUPP;
 			goto neg_err_exit;
 		}
-		server->sec_mode = (__u8)le16_to_cpu(rsp->SecurityMode);
+		server->sec_mode = le16_to_cpu(rsp->SecurityMode);
 		server->maxReq = min_t(unsigned int,
 				       le16_to_cpu(rsp->MaxMpxCount),
 				       cifs_max_pending);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index dc96835..a49a2a7 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -406,7 +406,7 @@ cifs_echo_request(struct work_struct *work)
 	 * done, which is indicated by maxBuf != 0. Also, no need to ping if
 	 * we got a response recently
 	 */
-	if (server->maxBuf == 0 ||
+	if (S_OP(server, need_neg, false, server) ||
 	    time_before(jiffies, server->lstrp + SMB_ECHO_INTERVAL - HZ))
 		goto requeue_echo;
 
@@ -3939,24 +3939,19 @@ cifs_umount(struct cifs_sb_info *cifs_sb)
 	kfree(cifs_sb);
 }
 
-int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses)
+int
+cifs_negotiate_protocol(const int xid, struct cifs_ses *ses)
 {
 	int rc = 0;
 	struct TCP_Server_Info *server = ses->server;
 
 	/* only send once per connect */
-	if (server->maxBuf != 0)
+	if (!S_OP(server, need_neg, false, server))
 		return 0;
 
 	set_credits(server, 1);
-	rc = CIFSSMBNegotiate(xid, ses);
-	if (rc == -EAGAIN) {
-		/* retry only once on 1st time connection */
-		set_credits(server, 1);
-		rc = CIFSSMBNegotiate(xid, ses);
-		if (rc == -EAGAIN)
-			rc = -EHOSTDOWN;
-	}
+
+	rc = S_OP(server, negotiate, -ENOSYS, xid, ses);
 	if (rc == 0) {
 		spin_lock(&GlobalMid_Lock);
 		if (server->tcpStatus == CifsNeedNegotiate)
@@ -3964,7 +3959,6 @@ int cifs_negotiate_protocol(unsigned int xid, struct cifs_ses *ses)
 		else
 			rc = -EHOSTDOWN;
 		spin_unlock(&GlobalMid_Lock);
-
 	}
 
 	return rc;
diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 551d0c2..f88fa4d 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -898,7 +898,7 @@ ssetup_ntlmssp_authenticate:
 	if (action & GUEST_LOGIN)
 		cFYI(1, "Guest login"); /* BB mark SesInfo struct? */
 	ses->Suid = smb_buf->Uid;   /* UID left in wire format (le) */
-	cFYI(1, "UID = %d ", ses->Suid);
+	cFYI(1, "UID = %llu ", ses->Suid);
 	/* response can have either 3 or 4 word count - Samba sends 3 */
 	/* and lanman response is 3 */
 	bytes_remaining = get_bcc(smb_buf);
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index f4f8394..5bfc26a 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -389,6 +389,27 @@ cifs_check_trans2(struct mid_q_entry *mid, struct TCP_Server_Info *server,
 	return true;
 }
 
+static bool
+cifs_need_neg(struct TCP_Server_Info *server)
+{
+	return server->maxBuf == 0;
+}
+
+static int
+cifs_negotiate(const int xid, struct cifs_ses *ses)
+{
+	int rc;
+	rc = CIFSSMBNegotiate(xid, ses);
+	if (rc == -EAGAIN) {
+		/* retry only once on 1st time connection */
+		set_credits(ses->server, 1);
+		rc = CIFSSMBNegotiate(xid, ses);
+		if (rc == -EAGAIN)
+			rc = -EHOSTDOWN;
+	}
+	return rc;
+}
+
 struct smb_version_operations smb1_operations = {
 	.send_cancel = send_nt_cancel,
 	.compare_fids = cifs_compare_fids,
@@ -407,6 +428,8 @@ struct smb_version_operations smb1_operations = {
 	.dump_detail = cifs_dump_detail,
 	.is_oplock_break = is_valid_oplock_break,
 	.check_trans2 = cifs_check_trans2,
+	.need_neg = cifs_need_neg,
+	.negotiate = cifs_negotiate,
 };
 
 struct smb_version_values smb1_values = {
-- 
1.7.1

[PATCH v3 6/7] CIFS: Move protocol specific session setup/logoff code to ops struct

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    5 +++++
 fs/cifs/cifsproto.h |    8 ++++----
 fs/cifs/connect.c   |    9 +++++----
 fs/cifs/sess.c      |    2 +-
 fs/cifs/smb1ops.c   |    2 ++
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 30fe2ac..665c04e 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -196,6 +196,11 @@ struct smb_version_operations {
 	bool (*need_neg)(struct TCP_Server_Info *);
 	/* negotiate to the server */
 	int (*negotiate)(const int, struct cifs_ses *);
+	/* setup smb sessionn */
+	int (*sess_setup)(const int, struct cifs_ses *,
+			  const struct nls_table *);
+	/* close smb session */
+	int (*logoff)(const int, struct cifs_ses *);
 };
 
 struct smb_version_values {
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 5e8efd8..62e9c5b 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -112,8 +112,8 @@ extern void header_assemble(struct smb_hdr *, char /* command */ ,
 extern int small_smb_init_no_tc(const int smb_cmd, const int wct,
 				struct cifs_ses *ses,
 				void **request_buf);
-extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses,
-			     const struct nls_table *nls_cp);
+extern int CIFS_SessSetup(const int xid, struct cifs_ses *ses,
+			  const struct nls_table *nls_cp);
 extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
 extern u64 cifs_UnixTimeToNT(struct timespec);
 extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
@@ -179,8 +179,8 @@ void cifs_proc_init(void);
 void cifs_proc_clean(void);
 
 extern int cifs_negotiate_protocol(const int xid, struct cifs_ses *ses);
-extern int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
-			struct nls_table *nls_info);
+extern int cifs_setup_session(const int xid, struct cifs_ses *ses,
+			      struct nls_table *nls_info);
 extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses);
 
 extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses,
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index a49a2a7..7285c4f 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2262,7 +2262,7 @@ cifs_put_smb_ses(struct cifs_ses *ses)
 
 	if (ses->status == CifsGood) {
 		xid = GetXid();
-		CIFSSMBLogoff(xid, ses);
+		S_OPV(server, logoff, xid, ses);
 		_FreeXid(xid);
 	}
 	sesInfoFree(ses);
@@ -3965,8 +3965,9 @@ cifs_negotiate_protocol(const int xid, struct cifs_ses *ses)
 }
 
 
-int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
-			struct nls_table *nls_info)
+int
+cifs_setup_session(const int xid, struct cifs_ses *ses,
+		   struct nls_table *nls_info)
 {
 	int rc = 0;
 	struct TCP_Server_Info *server = ses->server;
@@ -3979,7 +3980,7 @@ int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
 	cFYI(1, "Security Mode: 0x%x Capabilities: 0x%x TimeAdjust: %d",
 		 server->sec_mode, server->capabilities, server->timeAdj);
 
-	rc = CIFS_SessSetup(xid, ses, nls_info);
+	rc = S_OP(server, sess_setup, -ENOSYS, xid, ses, 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 f88fa4d..16b2083 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -556,7 +556,7 @@ setup_ntlmv2_ret:
 }
 
 int
-CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses,
+CIFS_SessSetup(const int xid, struct cifs_ses *ses,
 	       const struct nls_table *nls_cp)
 {
 	int rc = 0;
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 5bfc26a..ba3071f 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -430,6 +430,8 @@ struct smb_version_operations smb1_operations = {
 	.check_trans2 = cifs_check_trans2,
 	.need_neg = cifs_need_neg,
 	.negotiate = cifs_negotiate,
+	.sess_setup = CIFS_SessSetup,
+	.logoff = CIFSSMBLogoff,
 };
 
 struct smb_version_values smb1_values = {
-- 
1.7.1

[PATCH v3 7/7] CIFS: Move protocol specific tcon/tdis code to ops struct

[Pavel Shilovsky]

and rename variables around the code changes.

Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 fs/cifs/cifsglob.h  |    6 ++++++
 fs/cifs/cifsproto.h |    5 ++---
 fs/cifs/connect.c   |   47 ++++++++++++++++++++++++-----------------------
 fs/cifs/smb1ops.c   |    2 ++
 4 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 665c04e..8660d28 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -160,6 +160,7 @@ struct mid_q_entry;
 struct TCP_Server_Info;
 struct cifsFileInfo;
 struct cifs_ses;
+struct cifs_tcon;
 
 struct smb_version_operations {
 	int (*send_cancel)(struct TCP_Server_Info *, void *,
@@ -201,6 +202,11 @@ struct smb_version_operations {
 			  const struct nls_table *);
 	/* close smb session */
 	int (*logoff)(const int, struct cifs_ses *);
+	/* connect to a server share */
+	int (*tree_connect)(const int, struct cifs_ses *, const char *,
+			    struct cifs_tcon *, const struct nls_table *);
+	/* close tree connecion */
+	int (*tree_disconnect)(const int, struct cifs_tcon *);
 };
 
 struct smb_version_values {
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 62e9c5b..43d8ae6 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -183,9 +183,8 @@ extern int cifs_setup_session(const int xid, struct cifs_ses *ses,
 			      struct nls_table *nls_info);
 extern int CIFSSMBNegotiate(const int xid, struct cifs_ses *ses);
 
-extern int CIFSTCon(unsigned int xid, struct cifs_ses *ses,
-			const char *tree, struct cifs_tcon *tcon,
-			const struct nls_table *);
+extern int CIFSTCon(const int xid, struct cifs_ses *ses, const char *tree,
+		    struct cifs_tcon *tcon, const struct nls_table *);
 
 extern int CIFSFindFirst(const int xid, struct cifs_tcon *tcon,
 		const char *searchName, const struct nls_table *nls_codepage,
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 7285c4f..72b0496 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2549,7 +2549,7 @@ cifs_put_tcon(struct cifs_tcon *tcon)
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	xid = GetXid();
-	CIFSSMBTDis(xid, tcon);
+	S_OPV(ses->server, tree_disconnect, xid, tcon);
 	_FreeXid(xid);
 
 	cifs_fscache_release_super_cookie(tcon);
@@ -2596,13 +2596,15 @@ cifs_get_tcon(struct cifs_ses *ses, struct smb_vol *volume_info)
 		goto out_fail;
 	}
 
-	/* BB Do we need to wrap session_mutex around
-	 * this TCon call and Unix SetFS as
-	 * we do on SessSetup and reconnect? */
+	/*
+	 * BB Do we need to wrap session_mutex around this TCon call and Unix
+	 * SetFS as we do on SessSetup and reconnect?
+	 */
 	xid = GetXid();
-	rc = CIFSTCon(xid, ses, volume_info->UNC, tcon, volume_info->local_nls);
+	rc = S_OP(ses->server, tree_connect, -ENOSYS, xid, ses,
+		  volume_info->UNC, tcon, volume_info->local_nls);
 	FreeXid(xid);
-	cFYI(1, "CIFS Tcon rc = %d", rc);
+	cFYI(1, "Tcon rc = %d", rc);
 	if (rc)
 		goto out_fail;
 
@@ -2748,35 +2750,34 @@ out:
 }
 
 int
-get_dfs_path(int xid, struct cifs_ses *pSesInfo, const char *old_path,
-	     const struct nls_table *nls_codepage, unsigned int *pnum_referrals,
-	     struct dfs_info3_param **preferrals, int remap)
+get_dfs_path(int xid, struct cifs_ses *ses, const char *old_path,
+	     const struct nls_table *nls_codepage, unsigned int *num_referrals,
+	     struct dfs_info3_param **referrals, int remap)
 {
 	char *temp_unc;
 	int rc = 0;
 
-	*pnum_referrals = 0;
-	*preferrals = NULL;
+	*num_referrals = 0;
+	*referrals = NULL;
 
-	if (pSesInfo->ipc_tid == 0) {
+	if (ses->ipc_tid == 0) {
 		temp_unc = kmalloc(2 /* for slashes */ +
-			strnlen(pSesInfo->serverName,
-				SERVER_NAME_LEN_WITH_NULL * 2)
-				 + 1 + 4 /* slash IPC$ */  + 2,
-				GFP_KERNEL);
+			strnlen(ses->serverName, SERVER_NAME_LEN_WITH_NULL * 2)
+				+ 1 + 4 /* slash IPC$ */ + 2, GFP_KERNEL);
 		if (temp_unc == NULL)
 			return -ENOMEM;
 		temp_unc[0] = '\\';
 		temp_unc[1] = '\\';
-		strcpy(temp_unc + 2, pSesInfo->serverName);
-		strcpy(temp_unc + 2 + strlen(pSesInfo->serverName), "\\IPC$");
-		rc = CIFSTCon(xid, pSesInfo, temp_unc, NULL, nls_codepage);
-		cFYI(1, "CIFS Tcon rc = %d ipc_tid = %d", rc, pSesInfo->ipc_tid);
+		strcpy(temp_unc + 2, ses->serverName);
+		strcpy(temp_unc + 2 + strlen(ses->serverName), "\\IPC$");
+		rc = S_OP(ses->server, tree_connect, -ENOSYS, xid, ses,
+			  temp_unc, NULL, nls_codepage);
+		cFYI(1, "Tcon rc = %d ipc_tid = %d", rc, ses->ipc_tid);
 		kfree(temp_unc);
 	}
 	if (rc == 0)
-		rc = CIFSGetDFSRefer(xid, pSesInfo, old_path, preferrals,
-				     pnum_referrals, nls_codepage, remap);
+		rc = CIFSGetDFSRefer(xid, ses, old_path, referrals,
+				     num_referrals, nls_codepage, remap);
 	/* BB map targetUNCs to dfs_info3 structures, here or
 		in CIFSGetDFSRefer BB */
 
@@ -3755,7 +3756,7 @@ out:
  * pointer may be NULL.
  */
 int
-CIFSTCon(unsigned int xid, struct cifs_ses *ses,
+CIFSTCon(const int xid, struct cifs_ses *ses,
 	 const char *tree, struct cifs_tcon *tcon,
 	 const struct nls_table *nls_codepage)
 {
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index ba3071f..1a3f08c 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -432,6 +432,8 @@ struct smb_version_operations smb1_operations = {
 	.negotiate = cifs_negotiate,
 	.sess_setup = CIFS_SessSetup,
 	.logoff = CIFSSMBLogoff,
+	.tree_connect = CIFSTCon,
+	.tree_disconnect = CIFSSMBTDis,
 };
 
 struct smb_version_values smb1_values = {
-- 
1.7.1

Re: [PATCH v3 1/7] CIFS: Introduce S_OP/S_OPV macros for a safe access to ops struct field

[Jeff Layton]

On Tue, 29 May 2012 20:18:58 +0400
Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h  |   18 ++++++++++++++++++
>  fs/cifs/connect.c   |    4 ++--
>  fs/cifs/transport.c |    3 +--
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 20350a9..48d1009 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -367,6 +367,24 @@ struct TCP_Server_Info {
>  #endif
>  };
>  
> +/*
> + * The macro is used for safe access to ops field.
> + *
> + * @server - pointer to the server.
> + * @op - name of ops field.
> + * @miss_code - return code.

How about a little more in these comments? For instance, "return code
when the op is NULL" would be a better description here.

> + * @... - ops field arguments.

"arguments passed to the op"

> + *
> + * Return miss_code if server->ops doesn't have op field. Otherwise - return
> + * servr->ops->op(...).
> + */
> +
> +#define S_OP(server, op, miss_code, ...) \
> +	(server->ops->op ? server->ops->op(__VA_ARGS__) : miss_code)
> +
> +/* Void returned version of S_OP macro */
> +#define S_OPV(server, op, ...) S_OP(server, op, 0, __VA_ARGS__)
> +
>  static inline unsigned int
>  in_flight(struct TCP_Server_Info *server)
>  {
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index ccafded..a849aca 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -1058,13 +1058,13 @@ cifs_demultiplex_thread(void *p)
>  		if (mid_entry != NULL) {
>  			if (!mid_entry->multiRsp || mid_entry->multiEnd)
>  				mid_entry->callback(mid_entry);
> -		} else if (!server->ops->is_oplock_break(buf, server)) {
> +		} else if (!S_OP(server, is_oplock_break, false, buf, server)) {
>  			cERROR(1, "No task to wake, unknown frame received! "
>  				   "NumMids %d", atomic_read(&midCount));
>  			cifs_dump_mem("Received Data is: ", buf,
>  				      HEADER_SIZE(server));
>  #ifdef CONFIG_CIFS_DEBUG2
> -			server->ops->dump_detail(buf);
> +			S_OPV(server, dump_detail, buf);
>  			cifs_dump_mids(server);
>  #endif /* CIFS_DEBUG2 */
>  
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 1b36ffe..81d68f4 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -487,8 +487,7 @@ cifs_sync_mid_result(struct mid_q_entry *mid, struct TCP_Server_Info *server)
>  static inline int
>  send_cancel(struct TCP_Server_Info *server, void *buf, struct mid_q_entry *mid)
>  {
> -	return server->ops->send_cancel ?
> -				server->ops->send_cancel(server, buf, mid) : 0;
> +	return S_OP(server, send_cancel, 0, server, buf, mid);
>  }
>  
>  int

Other than the nit about the comments, this looks fine:

Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v3 2/7] CIFS: Move get_next_mid to ops struct

[Jeff Layton]

On Tue, 29 May 2012 20:18:59 +0400
Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h  |    7 ++++
>  fs/cifs/cifsproto.h |    1 -
>  fs/cifs/cifssmb.c   |    8 ++--
>  fs/cifs/connect.c   |    2 +-
>  fs/cifs/misc.c      |   89 +--------------------------------------------------
>  fs/cifs/smb1ops.c   |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  fs/cifs/transport.c |    2 +-
>  7 files changed, 103 insertions(+), 95 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 48d1009..a660577 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -174,6 +174,7 @@ struct smb_version_operations {
>  	void (*add_credits)(struct TCP_Server_Info *, const unsigned int);
>  	void (*set_credits)(struct TCP_Server_Info *, const int);
>  	int * (*get_credits_field)(struct TCP_Server_Info *);
> +	__u64 (*get_next_mid)(struct TCP_Server_Info *);
>  	/* data offset from read response message */
>  	unsigned int (*read_data_offset)(char *);
>  	/* data length from read response message */
> @@ -417,6 +418,12 @@ set_credits(struct TCP_Server_Info *server, const int val)
>  	server->ops->set_credits(server, val);
>  }
>  
> +static inline __u64
> +get_next_mid(struct TCP_Server_Info *server)
> +{
> +	return server->ops->get_next_mid(server);
> +}
> +

Not sure I see the value in this wrapper, but I'm not too concerned
about it...

>  /*
>   * Macros to allow the TCP_Server_Info->net field and related code to drop out
>   * when CONFIG_NET_NS isn't set.
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 5ec21ec..0a6cbfe 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -114,7 +114,6 @@ extern int small_smb_init_no_tc(const int smb_cmd, const int wct,
>  				void **request_buf);
>  extern int CIFS_SessSetup(unsigned int xid, struct cifs_ses *ses,
>  			     const struct nls_table *nls_cp);
> -extern __u64 GetNextMid(struct TCP_Server_Info *server);
>  extern struct timespec cifs_NTtimeToUnix(__le64 utc_nanoseconds_since_1601);
>  extern u64 cifs_UnixTimeToNT(struct timespec);
>  extern struct timespec cnvrtDosUnixTm(__le16 le_date, __le16 le_time,
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index b5ad716..5b40073 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -268,7 +268,7 @@ small_smb_init_no_tc(const int smb_command, const int wct,
>  		return rc;
>  
>  	buffer = (struct smb_hdr *)*request_buf;
> -	buffer->Mid = GetNextMid(ses->server);
> +	buffer->Mid = get_next_mid(ses->server);
>  	if (ses->capabilities & CAP_UNICODE)
>  		buffer->Flags2 |= SMBFLG2_UNICODE;
>  	if (ses->capabilities & CAP_STATUS32)
> @@ -402,7 +402,7 @@ CIFSSMBNegotiate(unsigned int xid, struct cifs_ses *ses)
>  
>  	cFYI(1, "secFlags 0x%x", secFlags);
>  
> -	pSMB->hdr.Mid = GetNextMid(server);
> +	pSMB->hdr.Mid = get_next_mid(server);
>  	pSMB->hdr.Flags2 |= (SMBFLG2_UNICODE | SMBFLG2_ERR_STATUS);
>  
>  	if ((secFlags & CIFSSEC_MUST_KRB5) == CIFSSEC_MUST_KRB5)
> @@ -782,7 +782,7 @@ CIFSSMBLogoff(const int xid, struct cifs_ses *ses)
>  		return rc;
>  	}
>  
> -	pSMB->hdr.Mid = GetNextMid(ses->server);
> +	pSMB->hdr.Mid = get_next_mid(ses->server);
>  
>  	if (ses->server->sec_mode &
>  		   (SECMODE_SIGN_REQUIRED | SECMODE_SIGN_ENABLED))
> @@ -4762,7 +4762,7 @@ getDFSRetry:
>  
>  	/* server pointer checked in called function,
>  	but should never be null here anyway */
> -	pSMB->hdr.Mid = GetNextMid(ses->server);
> +	pSMB->hdr.Mid = get_next_mid(ses->server);
>  	pSMB->hdr.Tid = ses->ipc_tid;
>  	pSMB->hdr.Uid = ses->Suid;
>  	if (ses->capabilities & CAP_STATUS32)
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index a849aca..9362f90 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -3938,7 +3938,7 @@ CIFSTCon(unsigned int xid, struct cifs_ses *ses,
>  	header_assemble(smb_buffer, SMB_COM_TREE_CONNECT_ANDX,
>  			NULL /*no tid */ , 4 /*wct */ );
>  
> -	smb_buffer->Mid = GetNextMid(ses->server);
> +	smb_buffer->Mid = get_next_mid(ses->server);
>  	smb_buffer->Uid = ses->Suid;
>  	pSMB = (TCONX_REQ *) smb_buffer;
>  	pSMBr = (TCONX_RSP *) smb_buffer_response;
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index e2552d2..557506a 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -212,93 +212,6 @@ cifs_small_buf_release(void *buf_to_free)
>  	return;
>  }
>  
> -/*
> - * Find a free multiplex id (SMB mid). Otherwise there could be
> - * mid collisions which might cause problems, demultiplexing the
> - * wrong response to this request. Multiplex ids could collide if
> - * one of a series requests takes much longer than the others, or
> - * if a very large number of long lived requests (byte range
> - * locks or FindNotify requests) are pending. No more than
> - * 64K-1 requests can be outstanding at one time. If no
> - * mids are available, return zero. A future optimization
> - * could make the combination of mids and uid the key we use
> - * to demultiplex on (rather than mid alone).
> - * In addition to the above check, the cifs demultiplex
> - * code already used the command code as a secondary
> - * check of the frame and if signing is negotiated the
> - * response would be discarded if the mid were the same
> - * but the signature was wrong. Since the mid is not put in the
> - * pending queue until later (when it is about to be dispatched)
> - * we do have to limit the number of outstanding requests
> - * to somewhat less than 64K-1 although it is hard to imagine
> - * so many threads being in the vfs at one time.
> - */
> -__u64 GetNextMid(struct TCP_Server_Info *server)
> -{
> -	__u64 mid = 0;
> -	__u16 last_mid, cur_mid;
> -	bool collision;
> -
> -	spin_lock(&GlobalMid_Lock);
> -
> -	/* mid is 16 bit only for CIFS/SMB */
> -	cur_mid = (__u16)((server->CurrentMid) & 0xffff);
> -	/* we do not want to loop forever */
> -	last_mid = cur_mid;
> -	cur_mid++;
> -
> -	/*
> -	 * This nested loop looks more expensive than it is.
> -	 * In practice the list of pending requests is short,
> -	 * fewer than 50, and the mids are likely to be unique
> -	 * on the first pass through the loop unless some request
> -	 * takes longer than the 64 thousand requests before it
> -	 * (and it would also have to have been a request that
> -	 * did not time out).
> -	 */
> -	while (cur_mid != last_mid) {
> -		struct mid_q_entry *mid_entry;
> -		unsigned int num_mids;
> -
> -		collision = false;
> -		if (cur_mid == 0)
> -			cur_mid++;
> -
> -		num_mids = 0;
> -		list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
> -			++num_mids;
> -			if (mid_entry->mid == cur_mid &&
> -			    mid_entry->mid_state == MID_REQUEST_SUBMITTED) {
> -				/* This mid is in use, try a different one */
> -				collision = true;
> -				break;
> -			}
> -		}
> -
> -		/*
> -		 * if we have more than 32k mids in the list, then something
> -		 * is very wrong. Possibly a local user is trying to DoS the
> -		 * box by issuing long-running calls and SIGKILL'ing them. If
> -		 * we get to 2^16 mids then we're in big trouble as this
> -		 * function could loop forever.
> -		 *
> -		 * Go ahead and assign out the mid in this situation, but force
> -		 * an eventual reconnect to clean out the pending_mid_q.
> -		 */
> -		if (num_mids > 32768)
> -			server->tcpStatus = CifsNeedReconnect;
> -
> -		if (!collision) {
> -			mid = (__u64)cur_mid;
> -			server->CurrentMid = mid;
> -			break;
> -		}
> -		cur_mid++;
> -	}
> -	spin_unlock(&GlobalMid_Lock);
> -	return mid;
> -}
> -
>  /* NB: MID can not be set if treeCon not passed in, in that
>     case it is responsbility of caller to set the mid */
>  void
> @@ -334,7 +247,7 @@ header_assemble(struct smb_hdr *buffer, char smb_command /* command */ ,
>  
>  			/* Uid is not converted */
>  			buffer->Uid = treeCon->ses->Suid;
> -			buffer->Mid = GetNextMid(treeCon->ses->server);
> +			buffer->Mid = get_next_mid(treeCon->ses->server);
>  		}
>  		if (treeCon->Flags & SMB_SHARE_IS_IN_DFS)
>  			buffer->Flags2 |= SMBFLG2_DFS;
> diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
> index d9d615f..6dec38f 100644
> --- a/fs/cifs/smb1ops.c
> +++ b/fs/cifs/smb1ops.c
> @@ -125,6 +125,94 @@ cifs_get_credits_field(struct TCP_Server_Info *server)
>  	return &server->credits;
>  }
>  
> +/*
> + * Find a free multiplex id (SMB mid). Otherwise there could be
> + * mid collisions which might cause problems, demultiplexing the
> + * wrong response to this request. Multiplex ids could collide if
> + * one of a series requests takes much longer than the others, or
> + * if a very large number of long lived requests (byte range
> + * locks or FindNotify requests) are pending. No more than
> + * 64K-1 requests can be outstanding at one time. If no
> + * mids are available, return zero. A future optimization
> + * could make the combination of mids and uid the key we use
> + * to demultiplex on (rather than mid alone).
> + * In addition to the above check, the cifs demultiplex
> + * code already used the command code as a secondary
> + * check of the frame and if signing is negotiated the
> + * response would be discarded if the mid were the same
> + * but the signature was wrong. Since the mid is not put in the
> + * pending queue until later (when it is about to be dispatched)
> + * we do have to limit the number of outstanding requests
> + * to somewhat less than 64K-1 although it is hard to imagine
> + * so many threads being in the vfs at one time.
> + */
> +static __u64
> +cifs_get_next_mid(struct TCP_Server_Info *server)
> +{
> +	__u64 mid = 0;
> +	__u16 last_mid, cur_mid;
> +	bool collision;
> +
> +	spin_lock(&GlobalMid_Lock);
> +
> +	/* mid is 16 bit only for CIFS/SMB */
> +	cur_mid = (__u16)((server->CurrentMid) & 0xffff);
> +	/* we do not want to loop forever */
> +	last_mid = cur_mid;
> +	cur_mid++;
> +
> +	/*
> +	 * This nested loop looks more expensive than it is.
> +	 * In practice the list of pending requests is short,
> +	 * fewer than 50, and the mids are likely to be unique
> +	 * on the first pass through the loop unless some request
> +	 * takes longer than the 64 thousand requests before it
> +	 * (and it would also have to have been a request that
> +	 * did not time out).
> +	 */
> +	while (cur_mid != last_mid) {
> +		struct mid_q_entry *mid_entry;
> +		unsigned int num_mids;
> +
> +		collision = false;
> +		if (cur_mid == 0)
> +			cur_mid++;
> +
> +		num_mids = 0;
> +		list_for_each_entry(mid_entry, &server->pending_mid_q, qhead) {
> +			++num_mids;
> +			if (mid_entry->mid == cur_mid &&
> +			    mid_entry->mid_state == MID_REQUEST_SUBMITTED) {
> +				/* This mid is in use, try a different one */
> +				collision = true;
> +				break;
> +			}
> +		}
> +
> +		/*
> +		 * if we have more than 32k mids in the list, then something
> +		 * is very wrong. Possibly a local user is trying to DoS the
> +		 * box by issuing long-running calls and SIGKILL'ing them. If
> +		 * we get to 2^16 mids then we're in big trouble as this
> +		 * function could loop forever.
> +		 *
> +		 * Go ahead and assign out the mid in this situation, but force
> +		 * an eventual reconnect to clean out the pending_mid_q.
> +		 */
> +		if (num_mids > 32768)
> +			server->tcpStatus = CifsNeedReconnect;
> +
> +		if (!collision) {
> +			mid = (__u64)cur_mid;
> +			server->CurrentMid = mid;
> +			break;
> +		}
> +		cur_mid++;
> +	}
> +	spin_unlock(&GlobalMid_Lock);
> +	return mid;
> +}
> +
>  struct smb_version_operations smb1_operations = {
>  	.send_cancel = send_nt_cancel,
>  	.compare_fids = cifs_compare_fids,
> @@ -133,6 +221,7 @@ struct smb_version_operations smb1_operations = {
>  	.add_credits = cifs_add_credits,
>  	.set_credits = cifs_set_credits,
>  	.get_credits_field = cifs_get_credits_field,
> +	.get_next_mid = cifs_get_next_mid,
>  	.read_data_offset = cifs_read_data_offset,
>  	.read_data_length = cifs_read_data_length,
>  	.map_error = map_smb_to_linux_error,
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 81d68f4..6fdb5be 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -778,7 +778,7 @@ send_lock_cancel(const unsigned int xid, struct cifs_tcon *tcon,
>  
>  	pSMB->LockType = LOCKING_ANDX_CANCEL_LOCK|LOCKING_ANDX_LARGE_FILES;
>  	pSMB->Timeout = 0;
> -	pSMB->hdr.Mid = GetNextMid(ses->server);
> +	pSMB->hdr.Mid = get_next_mid(ses->server);
>  
>  	return SendReceive(xid, ses, in_buf, out_buf,
>  			&bytes_returned, 0);



Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH v3 3/7] CIFS: Move trans2 processing to ops struct

[Jeff Layton]

On Tue, 29 May 2012 20:19:00 +0400
Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> ---
>  fs/cifs/cifsglob.h |    3 +
>  fs/cifs/connect.c  |  160 +------------------------------------------------
>  fs/cifs/smb1ops.c  |  170 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 174 insertions(+), 159 deletions(-)
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index a660577..3b8828b 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -187,6 +187,9 @@ struct smb_version_operations {
>  	/* verify the message */
>  	int (*check_message)(char *, unsigned int);
>  	bool (*is_oplock_break)(char *, struct TCP_Server_Info *);
> +	/* process transaction2 response */
> +	bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
> +			     char *, int);
>  };
>  
>  struct smb_version_values {
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 9362f90..dc96835 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -394,143 +394,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>  	return rc;
>  }
>  
> -/*
> -	return codes:
> -		0 	not a transact2, or all data present
> -		>0 	transact2 with that much data missing
> -		-EINVAL = invalid transact2
> -
> - */
> -static int check2ndT2(char *buf)
> -{
> -	struct smb_hdr *pSMB = (struct smb_hdr *)buf;
> -	struct smb_t2_rsp *pSMBt;
> -	int remaining;
> -	__u16 total_data_size, data_in_this_rsp;
> -
> -	if (pSMB->Command != SMB_COM_TRANSACTION2)
> -		return 0;
> -
> -	/* check for plausible wct, bcc and t2 data and parm sizes */
> -	/* check for parm and data offset going beyond end of smb */
> -	if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */
> -		cFYI(1, "invalid transact2 word count");
> -		return -EINVAL;
> -	}
> -
> -	pSMBt = (struct smb_t2_rsp *)pSMB;
> -
> -	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
> -	data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> -
> -	if (total_data_size == data_in_this_rsp)
> -		return 0;
> -	else if (total_data_size < data_in_this_rsp) {
> -		cFYI(1, "total data %d smaller than data in frame %d",
> -			total_data_size, data_in_this_rsp);
> -		return -EINVAL;
> -	}
> -
> -	remaining = total_data_size - data_in_this_rsp;
> -
> -	cFYI(1, "missing %d bytes from transact2, check next response",
> -		remaining);
> -	if (total_data_size > CIFSMaxBufSize) {
> -		cERROR(1, "TotalDataSize %d is over maximum buffer %d",
> -			total_data_size, CIFSMaxBufSize);
> -		return -EINVAL;
> -	}
> -	return remaining;
> -}
> -
> -static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
> -{
> -	struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf;
> -	struct smb_t2_rsp *pSMBt  = (struct smb_t2_rsp *)target_hdr;
> -	char *data_area_of_tgt;
> -	char *data_area_of_src;
> -	int remaining;
> -	unsigned int byte_count, total_in_tgt;
> -	__u16 tgt_total_cnt, src_total_cnt, total_in_src;
> -
> -	src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount);
> -	tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
> -
> -	if (tgt_total_cnt != src_total_cnt)
> -		cFYI(1, "total data count of primary and secondary t2 differ "
> -			"source=%hu target=%hu", src_total_cnt, tgt_total_cnt);
> -
> -	total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> -
> -	remaining = tgt_total_cnt - total_in_tgt;
> -
> -	if (remaining < 0) {
> -		cFYI(1, "Server sent too much data. tgt_total_cnt=%hu "
> -			"total_in_tgt=%hu", tgt_total_cnt, total_in_tgt);
> -		return -EPROTO;
> -	}
> -
> -	if (remaining == 0) {
> -		/* nothing to do, ignore */
> -		cFYI(1, "no more data remains");
> -		return 0;
> -	}
> -
> -	total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount);
> -	if (remaining < total_in_src)
> -		cFYI(1, "transact2 2nd response contains too much data");
> -
> -	/* find end of first SMB data area */
> -	data_area_of_tgt = (char *)&pSMBt->hdr.Protocol +
> -				get_unaligned_le16(&pSMBt->t2_rsp.DataOffset);
> -
> -	/* validate target area */
> -	data_area_of_src = (char *)&pSMBs->hdr.Protocol +
> -				get_unaligned_le16(&pSMBs->t2_rsp.DataOffset);
> -
> -	data_area_of_tgt += total_in_tgt;
> -
> -	total_in_tgt += total_in_src;
> -	/* is the result too big for the field? */
> -	if (total_in_tgt > USHRT_MAX) {
> -		cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt);
> -		return -EPROTO;
> -	}
> -	put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
> -
> -	/* fix up the BCC */
> -	byte_count = get_bcc(target_hdr);
> -	byte_count += total_in_src;
> -	/* is the result too big for the field? */
> -	if (byte_count > USHRT_MAX) {
> -		cFYI(1, "coalesced BCC too large (%u)", byte_count);
> -		return -EPROTO;
> -	}
> -	put_bcc(byte_count, target_hdr);
> -
> -	byte_count = be32_to_cpu(target_hdr->smb_buf_length);
> -	byte_count += total_in_src;
> -	/* don't allow buffer to overflow */
> -	if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) {
> -		cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count);
> -		return -ENOBUFS;
> -	}
> -	target_hdr->smb_buf_length = cpu_to_be32(byte_count);
> -
> -	/* copy second buffer into end of first buffer */
> -	memcpy(data_area_of_tgt, data_area_of_src, total_in_src);
> -
> -	if (remaining != total_in_src) {
> -		/* more responses to go */
> -		cFYI(1, "waiting for more secondary responses");
> -		return 1;
> -	}
> -
> -	/* we are done */
> -	cFYI(1, "found the last secondary response");
> -	return 0;
> -}
> -
>  static void
>  cifs_echo_request(struct work_struct *work)
>  {
> @@ -803,29 +666,8 @@ static void
>  handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
>  	   char *buf, int malformed)
>  {
> -	if (malformed == 0 && check2ndT2(buf) > 0) {
> -		mid->multiRsp = true;
> -		if (mid->resp_buf) {
> -			/* merge response - fix up 1st*/
> -			malformed = coalesce_t2(buf, mid->resp_buf);
> -			if (malformed > 0)
> -				return;
> -
> -			/* All parts received or packet is malformed. */
> -			mid->multiEnd = true;
> -			return dequeue_mid(mid, malformed);
> -		}
> -		if (!server->large_buf) {
> -			/*FIXME: switch to already allocated largebuf?*/
> -			cERROR(1, "1st trans2 resp needs bigbuf");
> -		} else {
> -			/* Have first buffer */
> -			mid->resp_buf = buf;
> -			mid->large_buf = true;
> -			server->bigbuf = NULL;
> -		}
> +	if (S_OP(server, check_trans2, false, mid, server, buf, malformed))

Blech, do you really think this makes the code clearer? I personally
find this macro ugly, but I suppose it's a matter of taste...

Looks fine otherwise, and I'm not concerned enough about the ugliness
to NAK it on those grounds...

Reviewed-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>

Re: [PATCH v3 3/7] CIFS: Move trans2 processing to ops struct

[Pavel Shilovsky]

2012/5/30 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> On Tue, 29 May 2012 20:19:00 +0400
> Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>
>> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> ---
>>  fs/cifs/cifsglob.h |    3 +
>>  fs/cifs/connect.c  |  160 +------------------------------------------------
>>  fs/cifs/smb1ops.c  |  170 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 174 insertions(+), 159 deletions(-)
>>
>> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
>> index a660577..3b8828b 100644
>> --- a/fs/cifs/cifsglob.h
>> +++ b/fs/cifs/cifsglob.h
>> @@ -187,6 +187,9 @@ struct smb_version_operations {
>>       /* verify the message */
>>       int (*check_message)(char *, unsigned int);
>>       bool (*is_oplock_break)(char *, struct TCP_Server_Info *);
>> +     /* process transaction2 response */
>> +     bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
>> +                          char *, int);
>>  };
>>
>>  struct smb_version_values {
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 9362f90..dc96835 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -394,143 +394,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
>>       return rc;
>>  }
>>
>> -/*
>> -     return codes:
>> -             0       not a transact2, or all data present
>> -             >0      transact2 with that much data missing
>> -             -EINVAL = invalid transact2
>> -
>> - */
>> -static int check2ndT2(char *buf)
>> -{
>> -     struct smb_hdr *pSMB = (struct smb_hdr *)buf;
>> -     struct smb_t2_rsp *pSMBt;
>> -     int remaining;
>> -     __u16 total_data_size, data_in_this_rsp;
>> -
>> -     if (pSMB->Command != SMB_COM_TRANSACTION2)
>> -             return 0;
>> -
>> -     /* check for plausible wct, bcc and t2 data and parm sizes */
>> -     /* check for parm and data offset going beyond end of smb */
>> -     if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */
>> -             cFYI(1, "invalid transact2 word count");
>> -             return -EINVAL;
>> -     }
>> -
>> -     pSMBt = (struct smb_t2_rsp *)pSMB;
>> -
>> -     total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
>> -     data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
>> -
>> -     if (total_data_size == data_in_this_rsp)
>> -             return 0;
>> -     else if (total_data_size < data_in_this_rsp) {
>> -             cFYI(1, "total data %d smaller than data in frame %d",
>> -                     total_data_size, data_in_this_rsp);
>> -             return -EINVAL;
>> -     }
>> -
>> -     remaining = total_data_size - data_in_this_rsp;
>> -
>> -     cFYI(1, "missing %d bytes from transact2, check next response",
>> -             remaining);
>> -     if (total_data_size > CIFSMaxBufSize) {
>> -             cERROR(1, "TotalDataSize %d is over maximum buffer %d",
>> -                     total_data_size, CIFSMaxBufSize);
>> -             return -EINVAL;
>> -     }
>> -     return remaining;
>> -}
>> -
>> -static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
>> -{
>> -     struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf;
>> -     struct smb_t2_rsp *pSMBt  = (struct smb_t2_rsp *)target_hdr;
>> -     char *data_area_of_tgt;
>> -     char *data_area_of_src;
>> -     int remaining;
>> -     unsigned int byte_count, total_in_tgt;
>> -     __u16 tgt_total_cnt, src_total_cnt, total_in_src;
>> -
>> -     src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount);
>> -     tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
>> -
>> -     if (tgt_total_cnt != src_total_cnt)
>> -             cFYI(1, "total data count of primary and secondary t2 differ "
>> -                     "source=%hu target=%hu", src_total_cnt, tgt_total_cnt);
>> -
>> -     total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
>> -
>> -     remaining = tgt_total_cnt - total_in_tgt;
>> -
>> -     if (remaining < 0) {
>> -             cFYI(1, "Server sent too much data. tgt_total_cnt=%hu "
>> -                     "total_in_tgt=%hu", tgt_total_cnt, total_in_tgt);
>> -             return -EPROTO;
>> -     }
>> -
>> -     if (remaining == 0) {
>> -             /* nothing to do, ignore */
>> -             cFYI(1, "no more data remains");
>> -             return 0;
>> -     }
>> -
>> -     total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount);
>> -     if (remaining < total_in_src)
>> -             cFYI(1, "transact2 2nd response contains too much data");
>> -
>> -     /* find end of first SMB data area */
>> -     data_area_of_tgt = (char *)&pSMBt->hdr.Protocol +
>> -                             get_unaligned_le16(&pSMBt->t2_rsp.DataOffset);
>> -
>> -     /* validate target area */
>> -     data_area_of_src = (char *)&pSMBs->hdr.Protocol +
>> -                             get_unaligned_le16(&pSMBs->t2_rsp.DataOffset);
>> -
>> -     data_area_of_tgt += total_in_tgt;
>> -
>> -     total_in_tgt += total_in_src;
>> -     /* is the result too big for the field? */
>> -     if (total_in_tgt > USHRT_MAX) {
>> -             cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt);
>> -             return -EPROTO;
>> -     }
>> -     put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
>> -
>> -     /* fix up the BCC */
>> -     byte_count = get_bcc(target_hdr);
>> -     byte_count += total_in_src;
>> -     /* is the result too big for the field? */
>> -     if (byte_count > USHRT_MAX) {
>> -             cFYI(1, "coalesced BCC too large (%u)", byte_count);
>> -             return -EPROTO;
>> -     }
>> -     put_bcc(byte_count, target_hdr);
>> -
>> -     byte_count = be32_to_cpu(target_hdr->smb_buf_length);
>> -     byte_count += total_in_src;
>> -     /* don't allow buffer to overflow */
>> -     if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) {
>> -             cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count);
>> -             return -ENOBUFS;
>> -     }
>> -     target_hdr->smb_buf_length = cpu_to_be32(byte_count);
>> -
>> -     /* copy second buffer into end of first buffer */
>> -     memcpy(data_area_of_tgt, data_area_of_src, total_in_src);
>> -
>> -     if (remaining != total_in_src) {
>> -             /* more responses to go */
>> -             cFYI(1, "waiting for more secondary responses");
>> -             return 1;
>> -     }
>> -
>> -     /* we are done */
>> -     cFYI(1, "found the last secondary response");
>> -     return 0;
>> -}
>> -
>>  static void
>>  cifs_echo_request(struct work_struct *work)
>>  {
>> @@ -803,29 +666,8 @@ static void
>>  handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
>>          char *buf, int malformed)
>>  {
>> -     if (malformed == 0 && check2ndT2(buf) > 0) {
>> -             mid->multiRsp = true;
>> -             if (mid->resp_buf) {
>> -                     /* merge response - fix up 1st*/
>> -                     malformed = coalesce_t2(buf, mid->resp_buf);
>> -                     if (malformed > 0)
>> -                             return;
>> -
>> -                     /* All parts received or packet is malformed. */
>> -                     mid->multiEnd = true;
>> -                     return dequeue_mid(mid, malformed);
>> -             }
>> -             if (!server->large_buf) {
>> -                     /*FIXME: switch to already allocated largebuf?*/
>> -                     cERROR(1, "1st trans2 resp needs bigbuf");
>> -             } else {
>> -                     /* Have first buffer */
>> -                     mid->resp_buf = buf;
>> -                     mid->large_buf = true;
>> -                     server->bigbuf = NULL;
>> -             }
>> +     if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
>
> Blech, do you really think this makes the code clearer? I personally
> find this macro ugly, but I suppose it's a matter of taste...
>
> Looks fine otherwise, and I'm not concerned enough about the ugliness
> to NAK it on those grounds...
>
> Reviewed-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>

Thanks for the review!

As for the macro itself. Instead of this:

if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
        return;

we will have:

if (server->ops->check_trans2 && server->ops->check_trans2(mid,
server, buf, malformed))
        return;

that looks bigger.

And for non-void functions:

rc = server->ops->negotiate? server->ops->negotiate(server, xid, ses))
: -ENOSYS;

the macro reduces this place to

rc = S_OP(server, negotiate, -ENOSYS, xid, ses);

that also looks more compact.

I am not sure that the name of the macro is ideal - suggestions are welcome!

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v3 3/7] CIFS: Move trans2 processing to ops struct

[Pavel Shilovsky]

2012/5/30 Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>:
> Thanks for the review!
>
> As for the macro itself. Instead of this:
>
> if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
>        return;
>
> we will have:
>
> if (server->ops->check_trans2 && server->ops->check_trans2(mid,
> server, buf, malformed))
>        return;
>
> that looks bigger.
>
> And for non-void functions:
>
> rc = server->ops->negotiate? server->ops->negotiate(server, xid, ses))
> : -ENOSYS;
>
> the macro reduces this place to
>
> rc = S_OP(server, negotiate, -ENOSYS, xid, ses);
>
> that also looks more compact.
>
> I am not sure that the name of the macro is ideal - suggestions are welcome!
>
> --
> Best regards,
> Pavel Shilovsky.

Sorry - email web client split the lines.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH v3 3/7] CIFS: Move trans2 processing to ops struct

[Jeff Layton]

On Wed, 30 May 2012 20:38:15 +0400
Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> 2012/5/30 Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>:
> > On Tue, 29 May 2012 20:19:00 +0400
> > Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >
> >> Signed-off-by: Pavel Shilovsky <pshilovsky-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >> ---
> >>  fs/cifs/cifsglob.h |    3 +
> >>  fs/cifs/connect.c  |  160 +------------------------------------------------
> >>  fs/cifs/smb1ops.c  |  170 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 174 insertions(+), 159 deletions(-)
> >>
> >> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> >> index a660577..3b8828b 100644
> >> --- a/fs/cifs/cifsglob.h
> >> +++ b/fs/cifs/cifsglob.h
> >> @@ -187,6 +187,9 @@ struct smb_version_operations {
> >>       /* verify the message */
> >>       int (*check_message)(char *, unsigned int);
> >>       bool (*is_oplock_break)(char *, struct TCP_Server_Info *);
> >> +     /* process transaction2 response */
> >> +     bool (*check_trans2)(struct mid_q_entry *, struct TCP_Server_Info *,
> >> +                          char *, int);
> >>  };
> >>
> >>  struct smb_version_values {
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 9362f90..dc96835 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -394,143 +394,6 @@ cifs_reconnect(struct TCP_Server_Info *server)
> >>       return rc;
> >>  }
> >>
> >> -/*
> >> -     return codes:
> >> -             0       not a transact2, or all data present
> >> -             >0      transact2 with that much data missing
> >> -             -EINVAL = invalid transact2
> >> -
> >> - */
> >> -static int check2ndT2(char *buf)
> >> -{
> >> -     struct smb_hdr *pSMB = (struct smb_hdr *)buf;
> >> -     struct smb_t2_rsp *pSMBt;
> >> -     int remaining;
> >> -     __u16 total_data_size, data_in_this_rsp;
> >> -
> >> -     if (pSMB->Command != SMB_COM_TRANSACTION2)
> >> -             return 0;
> >> -
> >> -     /* check for plausible wct, bcc and t2 data and parm sizes */
> >> -     /* check for parm and data offset going beyond end of smb */
> >> -     if (pSMB->WordCount != 10) { /* coalesce_t2 depends on this */
> >> -             cFYI(1, "invalid transact2 word count");
> >> -             return -EINVAL;
> >> -     }
> >> -
> >> -     pSMBt = (struct smb_t2_rsp *)pSMB;
> >> -
> >> -     total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
> >> -     data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> >> -
> >> -     if (total_data_size == data_in_this_rsp)
> >> -             return 0;
> >> -     else if (total_data_size < data_in_this_rsp) {
> >> -             cFYI(1, "total data %d smaller than data in frame %d",
> >> -                     total_data_size, data_in_this_rsp);
> >> -             return -EINVAL;
> >> -     }
> >> -
> >> -     remaining = total_data_size - data_in_this_rsp;
> >> -
> >> -     cFYI(1, "missing %d bytes from transact2, check next response",
> >> -             remaining);
> >> -     if (total_data_size > CIFSMaxBufSize) {
> >> -             cERROR(1, "TotalDataSize %d is over maximum buffer %d",
> >> -                     total_data_size, CIFSMaxBufSize);
> >> -             return -EINVAL;
> >> -     }
> >> -     return remaining;
> >> -}
> >> -
> >> -static int coalesce_t2(char *second_buf, struct smb_hdr *target_hdr)
> >> -{
> >> -     struct smb_t2_rsp *pSMBs = (struct smb_t2_rsp *)second_buf;
> >> -     struct smb_t2_rsp *pSMBt  = (struct smb_t2_rsp *)target_hdr;
> >> -     char *data_area_of_tgt;
> >> -     char *data_area_of_src;
> >> -     int remaining;
> >> -     unsigned int byte_count, total_in_tgt;
> >> -     __u16 tgt_total_cnt, src_total_cnt, total_in_src;
> >> -
> >> -     src_total_cnt = get_unaligned_le16(&pSMBs->t2_rsp.TotalDataCount);
> >> -     tgt_total_cnt = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
> >> -
> >> -     if (tgt_total_cnt != src_total_cnt)
> >> -             cFYI(1, "total data count of primary and secondary t2 differ "
> >> -                     "source=%hu target=%hu", src_total_cnt, tgt_total_cnt);
> >> -
> >> -     total_in_tgt = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
> >> -
> >> -     remaining = tgt_total_cnt - total_in_tgt;
> >> -
> >> -     if (remaining < 0) {
> >> -             cFYI(1, "Server sent too much data. tgt_total_cnt=%hu "
> >> -                     "total_in_tgt=%hu", tgt_total_cnt, total_in_tgt);
> >> -             return -EPROTO;
> >> -     }
> >> -
> >> -     if (remaining == 0) {
> >> -             /* nothing to do, ignore */
> >> -             cFYI(1, "no more data remains");
> >> -             return 0;
> >> -     }
> >> -
> >> -     total_in_src = get_unaligned_le16(&pSMBs->t2_rsp.DataCount);
> >> -     if (remaining < total_in_src)
> >> -             cFYI(1, "transact2 2nd response contains too much data");
> >> -
> >> -     /* find end of first SMB data area */
> >> -     data_area_of_tgt = (char *)&pSMBt->hdr.Protocol +
> >> -                             get_unaligned_le16(&pSMBt->t2_rsp.DataOffset);
> >> -
> >> -     /* validate target area */
> >> -     data_area_of_src = (char *)&pSMBs->hdr.Protocol +
> >> -                             get_unaligned_le16(&pSMBs->t2_rsp.DataOffset);
> >> -
> >> -     data_area_of_tgt += total_in_tgt;
> >> -
> >> -     total_in_tgt += total_in_src;
> >> -     /* is the result too big for the field? */
> >> -     if (total_in_tgt > USHRT_MAX) {
> >> -             cFYI(1, "coalesced DataCount too large (%u)", total_in_tgt);
> >> -             return -EPROTO;
> >> -     }
> >> -     put_unaligned_le16(total_in_tgt, &pSMBt->t2_rsp.DataCount);
> >> -
> >> -     /* fix up the BCC */
> >> -     byte_count = get_bcc(target_hdr);
> >> -     byte_count += total_in_src;
> >> -     /* is the result too big for the field? */
> >> -     if (byte_count > USHRT_MAX) {
> >> -             cFYI(1, "coalesced BCC too large (%u)", byte_count);
> >> -             return -EPROTO;
> >> -     }
> >> -     put_bcc(byte_count, target_hdr);
> >> -
> >> -     byte_count = be32_to_cpu(target_hdr->smb_buf_length);
> >> -     byte_count += total_in_src;
> >> -     /* don't allow buffer to overflow */
> >> -     if (byte_count > CIFSMaxBufSize + MAX_CIFS_HDR_SIZE - 4) {
> >> -             cFYI(1, "coalesced BCC exceeds buffer size (%u)", byte_count);
> >> -             return -ENOBUFS;
> >> -     }
> >> -     target_hdr->smb_buf_length = cpu_to_be32(byte_count);
> >> -
> >> -     /* copy second buffer into end of first buffer */
> >> -     memcpy(data_area_of_tgt, data_area_of_src, total_in_src);
> >> -
> >> -     if (remaining != total_in_src) {
> >> -             /* more responses to go */
> >> -             cFYI(1, "waiting for more secondary responses");
> >> -             return 1;
> >> -     }
> >> -
> >> -     /* we are done */
> >> -     cFYI(1, "found the last secondary response");
> >> -     return 0;
> >> -}
> >> -
> >>  static void
> >>  cifs_echo_request(struct work_struct *work)
> >>  {
> >> @@ -803,29 +666,8 @@ static void
> >>  handle_mid(struct mid_q_entry *mid, struct TCP_Server_Info *server,
> >>          char *buf, int malformed)
> >>  {
> >> -     if (malformed == 0 && check2ndT2(buf) > 0) {
> >> -             mid->multiRsp = true;
> >> -             if (mid->resp_buf) {
> >> -                     /* merge response - fix up 1st*/
> >> -                     malformed = coalesce_t2(buf, mid->resp_buf);
> >> -                     if (malformed > 0)
> >> -                             return;
> >> -
> >> -                     /* All parts received or packet is malformed. */
> >> -                     mid->multiEnd = true;
> >> -                     return dequeue_mid(mid, malformed);
> >> -             }
> >> -             if (!server->large_buf) {
> >> -                     /*FIXME: switch to already allocated largebuf?*/
> >> -                     cERROR(1, "1st trans2 resp needs bigbuf");
> >> -             } else {
> >> -                     /* Have first buffer */
> >> -                     mid->resp_buf = buf;
> >> -                     mid->large_buf = true;
> >> -                     server->bigbuf = NULL;
> >> -             }
> >> +     if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
> >
> > Blech, do you really think this makes the code clearer? I personally
> > find this macro ugly, but I suppose it's a matter of taste...
> >
> > Looks fine otherwise, and I'm not concerned enough about the ugliness
> > to NAK it on those grounds...
> >
> > Reviewed-by: Jeff Layton <jlayton-vpEMnDpepFuMZCB2o+C8xQ@public.gmane.org>
> 
> Thanks for the review!
> 
> As for the macro itself. Instead of this:
> 
> if (S_OP(server, check_trans2, false, mid, server, buf, malformed))
>         return;
> 
> we will have:
> 
> if (server->ops->check_trans2 && server->ops->check_trans2(mid,
> server, buf, malformed))
>         return;
> 
> that looks bigger.
> 

Sure, it's more verbose, but at least it's clear where each of the
arguments go...

> And for non-void functions:
> 
> rc = server->ops->negotiate? server->ops->negotiate(server, xid, ses))
> : -ENOSYS;
> 
> the macro reduces this place to
> 
> rc = S_OP(server, negotiate, -ENOSYS, xid, ses);
> 
> that also looks more compact.
> 
> I am not sure that the name of the macro is ideal - suggestions are welcome!
> 

The name doesn't much matter to me, but clarity does...

Regardless though, since you're doing the work here I'm inclined to let
you decide on this point. If you find that clearer, then so be it. I
can live with it as long as it ultimately works...

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>