[Patch v2 1/6] cifs: smbd: Check for iov length on sending the last iov

[Patch v2 1/6] cifs: smbd: Check for iov length on sending the last iov

[Long Li]

From: Long Li <longli@microsoft.com>

When sending the last iov that breaks into smaller buffers to fit the
transfer size, it's necessary to check if this is the last iov.

If this is the latest iov, stop and proceed to send pages.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index 90e673c..b5c6c0d 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -2197,6 +2197,8 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 						goto done;
 				}
 				i++;
+				if (i == rqst->rq_nvec)
+					break;
 			}
 			start = i;
 			buflen = 0;
-- 
2.7.4

[Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

[Long Li]

From: Long Li <longli@microsoft.com>

The data buffer allocated on the stack can't be DMA'ed, and hence can't send
through RDMA via SMB Direct.

Fix this by allocating the request on the heap in smb3_validate_negotiate.

Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects against
downgrade attacks"

Changes in v2:
Removed duplicated code on freeing buffers on function exit.
(Thanks to Parav Pandit <parav@mellanox.com>)

Fixed typo in the patch title.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smb2pdu.c | 57 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 0f044c4..41625e4 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
 
 int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 {
-	int rc = 0;
-	struct validate_negotiate_info_req vneg_inbuf;
+	int ret, rc = -EIO;
+	struct validate_negotiate_info_req *pneg_inbuf;
 	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
 	u32 rsplen;
 	u32 inbuflen; /* max of 4 dialects */
@@ -741,6 +741,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (tcon->ses->server->rdma)
 		return 0;
 #endif
+	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
+	if (!pneg_inbuf)
+		return -ENOMEM;
 
 	/* In SMB3.11 preauth integrity supersedes validate negotiate */
 	if (tcon->ses->server->dialect == SMB311_PROT_ID)
@@ -764,53 +767,53 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
 		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag sent by server\n");
 
-	vneg_inbuf.Capabilities =
+	pneg_inbuf->Capabilities =
 			cpu_to_le32(tcon->ses->server->vals->req_capabilities);
-	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
+	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
 					SMB2_CLIENT_GUID_SIZE);
 
 	if (tcon->ses->sign)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
 	else if (global_secflags & CIFSSEC_MAY_SIGN)
-		vneg_inbuf.SecurityMode =
+		pneg_inbuf->SecurityMode =
 			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
 	else
-		vneg_inbuf.SecurityMode = 0;
+		pneg_inbuf->SecurityMode = 0;
 
 
 	if (strcmp(tcon->ses->server->vals->version_string,
 		SMB3ANY_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(2);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(2);
 		/* structure is big enough for 3 dialects, sending only 2 */
 		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;
 	} else if (strcmp(tcon->ses->server->vals->version_string,
 		SMBDEFAULT_VERSION_STRING) == 0) {
-		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
-		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
-		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
-		vneg_inbuf.DialectCount = cpu_to_le16(3);
+		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
+		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
+		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
+		pneg_inbuf->DialectCount = cpu_to_le16(3);
 		/* structure is big enough for 3 dialects */
 		inbuflen = sizeof(struct validate_negotiate_info_req);
 	} else {
 		/* otherwise specific dialect was requested */
-		vneg_inbuf.Dialects[0] =
+		pneg_inbuf->Dialects[0] =
 			cpu_to_le16(tcon->ses->server->vals->protocol_id);
-		vneg_inbuf.DialectCount = cpu_to_le16(1);
+		pneg_inbuf->DialectCount = cpu_to_le16(1);
 		/* structure is big enough for 3 dialects, sending only 1 */
 		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
 	}
 
-	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
+	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
 		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
-		(char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),
+		(char *)pneg_inbuf, sizeof(struct validate_negotiate_info_req),
 		(char **)&pneg_rsp, &rsplen);
 
-	if (rc != 0) {
-		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
-		return -EIO;
+	if (ret != 0) {
+		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
+		goto out_free_inbuf;
 	}
 
 	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
@@ -820,7 +823,7 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 		/* relax check since Mac returns max bufsize allowed on ioctl */
 		if ((rsplen > CIFSMaxBufSize)
 		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
-			goto err_rsp_free;
+			goto out_free_rsp;
 	}
 
 	/* check validate negotiate info response matches what we got earlier */
@@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 	/* validate negotiate successful */
 	cifs_dbg(FYI, "validate negotiate info successful\n");
-	kfree(pneg_rsp);
-	return 0;
+	rc = 0;
+	goto out_free_rsp;
 
 vneg_out:
 	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
-err_rsp_free:
+out_free_rsp:
 	kfree(pneg_rsp);
-	return -EIO;
+out_free_inbuf:
+	kfree(pneg_inbuf);
+	return rc;
 }
 
 enum securityEnum
-- 
2.7.4

[Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack

[Long Li]

From: Long Li <longli@microsoft.com>

It's not necessary to allocate another iov when going through the buffers
in smbd_send() through RDMA send.

Remove it to reduce stack size.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index b5c6c0d..f575e9a 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -2088,7 +2088,7 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 	int start, i, j;
 	int max_iov_size =
 		info->max_send_size - sizeof(struct smbd_data_transfer);
-	struct kvec iov[SMBDIRECT_MAX_SGE];
+	struct kvec *iov;
 	int rc;
 
 	info->smbd_send_pending++;
@@ -2099,32 +2099,20 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 	}
 
 	/*
-	 * This usually means a configuration error
-	 * We use RDMA read/write for packet size > rdma_readwrite_threshold
-	 * as long as it's properly configured we should never get into this
-	 * situation
-	 */
-	if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
-		log_write(ERR, "maximum send segment %x exceeding %x\n",
-			 rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
-		rc = -EINVAL;
-		goto done;
-	}
-
-	/*
-	 * Remove the RFC1002 length defined in MS-SMB2 section 2.1
-	 * It is used only for TCP transport
+	 * Skip the RFC1002 length defined in MS-SMB2 section 2.1
+	 * It is used only for TCP transport in the iov[0]
 	 * In future we may want to add a transport layer under protocol
 	 * layer so this will only be issued to TCP transport
 	 */
-	iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
-	iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
-	buflen += iov[0].iov_len;
+
+	if (rqst->rq_iov[0].iov_len != 4) {
+		log_write(ERR, "expected the pdu length in 1st iov, but got 0x%lu\n", rqst->rq_iov[0].iov_len);
+		return -EINVAL;
+	}
+	iov = &rqst->rq_iov[1];
 
 	/* total up iov array first */
-	for (i = 1; i < rqst->rq_nvec; i++) {
-		iov[i].iov_base = rqst->rq_iov[i].iov_base;
-		iov[i].iov_len = rqst->rq_iov[i].iov_len;
+	for (i = 0; i < rqst->rq_nvec-1; i++) {
 		buflen += iov[i].iov_len;
 	}
 
@@ -2197,14 +2185,14 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 						goto done;
 				}
 				i++;
-				if (i == rqst->rq_nvec)
+				if (i == rqst->rq_nvec-1)
 					break;
 			}
 			start = i;
 			buflen = 0;
 		} else {
 			i++;
-			if (i == rqst->rq_nvec) {
+			if (i == rqst->rq_nvec-1) {
 				/* send out all remaining vecs */
 				remaining_data_length -= buflen;
 				log_write(INFO,
-- 
2.7.4

[Patch v2 4/6] cifs: smbd: Don't use RDMA read/write when signing is used

[Long Li]

From: Long Li <longli@microsoft.com>

SMB server will not sign data transferred through RDMA read/write. When
signing is used, it's a good idea to have all the data signed.

In this case, use RDMA send/recv for all data transfers. This will degrade
performance as this is not generally configured in RDMA environemnt. So
warn the user on signing and RDMA send/recv.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/cifssmb.c |  3 +++
 fs/cifs/smb2ops.c | 18 ++++++++++++++----
 fs/cifs/smb2pdu.c |  4 ++--
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 6d3e40d..1529a08 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -455,6 +455,9 @@ cifs_enable_signing(struct TCP_Server_Info *server, bool mnt_sign_required)
 		server->sign = true;
 	}
 
+	if (cifs_rdma_enabled(server) && server->sign)
+		cifs_dbg(VFS, "Signing is enabled, and RDMA read/write will be disabled");
+
 	return 0;
 }
 
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 38ebf3f..b76b858 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -252,9 +252,14 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
 	wsize = volume_info->wsize ? volume_info->wsize : CIFS_DEFAULT_IOSIZE;
 	wsize = min_t(unsigned int, wsize, server->max_write);
 #ifdef CONFIG_CIFS_SMB_DIRECT
-	if (server->rdma)
-		wsize = min_t(unsigned int,
+	if (server->rdma) {
+		if (server->sign)
+			wsize = min_t(unsigned int,
+				wsize, server->smbd_conn->max_fragmented_send_size);
+		else
+			wsize = min_t(unsigned int,
 				wsize, server->smbd_conn->max_readwrite_size);
+	}
 #endif
 	if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
 		wsize = min_t(unsigned int, wsize, SMB2_MAX_BUFFER_SIZE);
@@ -272,9 +277,14 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
 	rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
 	rsize = min_t(unsigned int, rsize, server->max_read);
 #ifdef CONFIG_CIFS_SMB_DIRECT
-	if (server->rdma)
-		rsize = min_t(unsigned int,
+	if (server->rdma) {
+		if (server->sign)
+			rsize = min_t(unsigned int,
+				rsize, server->smbd_conn->max_fragmented_recv_size);
+		else
+			rsize = min_t(unsigned int,
 				rsize, server->smbd_conn->max_readwrite_size);
+	}
 #endif
 
 	if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 41625e4..33f612f 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -2595,7 +2595,7 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
 	 * If we want to do a RDMA write, fill in and append
 	 * smbd_buffer_descriptor_v1 to the end of read request
 	 */
-	if (server->rdma && rdata &&
+	if (server->rdma && rdata && !server->sign &&
 		rdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) {
 
 		struct smbd_buffer_descriptor_v1 *v1;
@@ -2973,7 +2973,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
 	 * If we want to do a server RDMA read, fill in and append
 	 * smbd_buffer_descriptor_v1 to the end of write request
 	 */
-	if (server->rdma && wdata->bytes >=
+	if (server->rdma && !server->sign && wdata->bytes >=
 		server->smbd_conn->rdma_readwrite_threshold) {
 
 		struct smbd_buffer_descriptor_v1 *v1;
-- 
2.7.4

[Patch v2 5/6] cifs: smbd: Enable signing with smbdirect

[Long Li]

From: Long Li <longli@microsoft.com>

Now signing is supported with RDMA transport.

Remove the code that disabled it.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/connect.c | 8 --------
 fs/cifs/smb2pdu.c | 4 ----
 2 files changed, 12 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index e8830f0..deef270 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1977,14 +1977,6 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 		goto cifs_parse_mount_err;
 	}
 
-#ifdef CONFIG_CIFS_SMB_DIRECT
-	if (vol->rdma && vol->sign) {
-		cifs_dbg(VFS, "Currently SMB direct doesn't support signing."
-			" This is being fixed\n");
-		goto cifs_parse_mount_err;
-	}
-#endif
-
 #ifndef CONFIG_KEYS
 	/* Muliuser mounts require CONFIG_KEYS support */
 	if (vol->multiuser) {
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 33f612f..3e052a0 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -737,10 +737,6 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
 
 	cifs_dbg(FYI, "validate negotiate\n");
 
-#ifdef CONFIG_CIFS_SMB_DIRECT
-	if (tcon->ses->server->rdma)
-		return 0;
-#endif
 	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
 	if (!pneg_inbuf)
 		return -ENOMEM;
-- 
2.7.4

[Patch v2 6/6] cifs: smbd: Dump SMB packet when configured

[Long Li]

From: Long Li <longli@microsoft.com>

When sending through SMB Direct, also dump the packet in SMB send path.

Also fixed a typo in debug message.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index f575e9a..6ff864a 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -1029,7 +1029,7 @@ static int smbd_post_send(struct smbd_connection *info,
 	for (i = 0; i < request->num_sge; i++) {
 		log_rdma_send(INFO,
 			"rdma_request sge[%d] addr=%llu length=%u\n",
-			i, request->sge[0].addr, request->sge[0].length);
+			i, request->sge[i].addr, request->sge[i].length);
 		ib_dma_sync_single_for_device(
 			info->id->device,
 			request->sge[i].addr,
@@ -2130,6 +2130,10 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 		goto done;
 	}
 
+	cifs_dbg(FYI, "Sending smb (RDMA): smb_len=%u\n", buflen);
+	for (i = 0; i < rqst->rq_nvec-1; i++)
+		dump_smb(iov[i].iov_base, iov[i].iov_len);
+
 	remaining_data_length = buflen;
 
 	log_write(INFO, "rqst->rq_nvec=%d rqst->rq_npages=%d rq_pagesz=%d "
-- 
2.7.4

RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

[Parav Pandit]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Long Li
> Sent: Tuesday, April 17, 2018 2:17 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org; linux-
> rdma@vger.kernel.org
> Cc: longli <longli@microsoft.com>; stable@vger.kernel.org
> Subject: [Patch v2 2/6] cifs: Allocate validate negotiation request through
> kmalloc
> 
> From: Long Li <longli@microsoft.com>
> 
> The data buffer allocated on the stack can't be DMA'ed, and hence can't send
> through RDMA via SMB Direct.
> 
> Fix this by allocating the request on the heap in smb3_validate_negotiate.
> 
> Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects
> against downgrade attacks"
> 

Format is:
Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks")
It should be right above Signed-off signature.

> Changes in v2:
> Removed duplicated code on freeing buffers on function exit.
> (Thanks to Parav Pandit <parav@mellanox.com>)
> 
> Fixed typo in the patch title.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/smb2pdu.c | 57 ++++++++++++++++++++++++++++++------------------------
> -
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 0f044c4..41625e4
> 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses
> *ses)
> 
>  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)  {
> -	int rc = 0;
> -	struct validate_negotiate_info_req vneg_inbuf;
> +	int ret, rc = -EIO;
> +	struct validate_negotiate_info_req *pneg_inbuf;
>  	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
>  	u32 rsplen;
>  	u32 inbuflen; /* max of 4 dialects */
> @@ -741,6 +741,9 @@ int smb3_validate_negotiate(const unsigned int xid,
> struct cifs_tcon *tcon)
>  	if (tcon->ses->server->rdma)
>  		return 0;
>  #endif
> +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
> +	if (!pneg_inbuf)
> +		return -ENOMEM;
> 
>  	/* In SMB3.11 preauth integrity supersedes validate negotiate */
>  	if (tcon->ses->server->dialect == SMB311_PROT_ID) @@ -764,53
> +767,53 @@ int smb3_validate_negotiate(const unsigned int xid, struct
> cifs_tcon *tcon)
>  	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
>  		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag sent
> by server\n");
> 
> -	vneg_inbuf.Capabilities =
> +	pneg_inbuf->Capabilities =
>  			cpu_to_le32(tcon->ses->server->vals-
> >req_capabilities);
> -	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
> +	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
>  					SMB2_CLIENT_GUID_SIZE);
> 
>  	if (tcon->ses->sign)
> -		vneg_inbuf.SecurityMode =
> +		pneg_inbuf->SecurityMode =
>  			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
>  	else if (global_secflags & CIFSSEC_MAY_SIGN)
> -		vneg_inbuf.SecurityMode =
> +		pneg_inbuf->SecurityMode =
>  			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
>  	else
> -		vneg_inbuf.SecurityMode = 0;
> +		pneg_inbuf->SecurityMode = 0;
> 
> 
>  	if (strcmp(tcon->ses->server->vals->version_string,
Please check if strncmp() should be used or not.

>  		SMB3ANY_VERSION_STRING) == 0) {
> -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> -		vneg_inbuf.DialectCount = cpu_to_le16(2);
> +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> +		pneg_inbuf->DialectCount = cpu_to_le16(2);
>  		/* structure is big enough for 3 dialects, sending only 2 */
>  		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;
>  	} else if (strcmp(tcon->ses->server->vals->version_string,
>  		SMBDEFAULT_VERSION_STRING) == 0) {
> -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> -		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> -		vneg_inbuf.DialectCount = cpu_to_le16(3);
> +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> +		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> +		pneg_inbuf->DialectCount = cpu_to_le16(3);
>  		/* structure is big enough for 3 dialects */
>  		inbuflen = sizeof(struct validate_negotiate_info_req);
>  	} else {
>  		/* otherwise specific dialect was requested */
> -		vneg_inbuf.Dialects[0] =
> +		pneg_inbuf->Dialects[0] =
>  			cpu_to_le16(tcon->ses->server->vals->protocol_id);
> -		vneg_inbuf.DialectCount = cpu_to_le16(1);
> +		pneg_inbuf->DialectCount = cpu_to_le16(1);
>  		/* structure is big enough for 3 dialects, sending only 1 */
>  		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
>  	}
> 
> -	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> +	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
>  		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> -		(char *)&vneg_inbuf, sizeof(struct
> validate_negotiate_info_req),
> +		(char *)pneg_inbuf, sizeof(struct validate_negotiate_info_req),

Use sizeof(*pneg_inbuf)

>  		(char **)&pneg_rsp, &rsplen);
> 
> -	if (rc != 0) {
> -		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
> -		return -EIO;
> +	if (ret != 0) {

if (ret) is fine.

> +		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
> +		goto out_free_inbuf;
>  	}
> 
>  	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) { @@ -820,7

if (rsplen != sizeof(*pneg_rsp))

RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

[Long Li]

> Subject: RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through
> kmalloc
> 
> 
> 
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> > owner@vger.kernel.org] On Behalf Of Long Li
> > Sent: Tuesday, April 17, 2018 2:17 PM
> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org; linux-
> > rdma@vger.kernel.org
> > Cc: longli <longli@microsoft.com>; stable@vger.kernel.org
> > Subject: [Patch v2 2/6] cifs: Allocate validate negotiation request
> > through kmalloc
> >
> > From: Long Li <longli@microsoft.com>
> >
> > The data buffer allocated on the stack can't be DMA'ed, and hence
> > can't send through RDMA via SMB Direct.
> >
> > Fix this by allocating the request on the heap in smb3_validate_negotiate.
> >
> > Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects
> > against downgrade attacks"
> >
> 
> Format is:
> Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks") It
> should be right above Signed-off signature.

I will fix up and resend this patch.

How about the rest patches (1, 3-6) in the series? If they don't need any changes, is it okay that I resend this one only?

> 
> > Changes in v2:
> > Removed duplicated code on freeing buffers on function exit.
> > (Thanks to Parav Pandit <parav@mellanox.com>)
> >
> > Fixed typo in the patch title.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/cifs/smb2pdu.c | 57
> > ++++++++++++++++++++++++++++++------------------------
> > -
> >  1 file changed, 31 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> > 0f044c4..41625e4
> > 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct
> > cifs_ses
> > *ses)
> >
> >  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
> {
> > -	int rc = 0;
> > -	struct validate_negotiate_info_req vneg_inbuf;
> > +	int ret, rc = -EIO;
> > +	struct validate_negotiate_info_req *pneg_inbuf;
> >  	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
> >  	u32 rsplen;
> >  	u32 inbuflen; /* max of 4 dialects */ @@ -741,6 +741,9 @@ int
> > smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> > *tcon)
> >  	if (tcon->ses->server->rdma)
> >  		return 0;
> >  #endif
> > +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
> > +	if (!pneg_inbuf)
> > +		return -ENOMEM;
> >
> >  	/* In SMB3.11 preauth integrity supersedes validate negotiate */
> >  	if (tcon->ses->server->dialect == SMB311_PROT_ID) @@ -764,53
> > +767,53 @@ int smb3_validate_negotiate(const unsigned int xid, struct
> > cifs_tcon *tcon)
> >  	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
> >  		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag
> sent by
> > server\n");
> >
> > -	vneg_inbuf.Capabilities =
> > +	pneg_inbuf->Capabilities =
> >  			cpu_to_le32(tcon->ses->server->vals-
> > >req_capabilities);
> > -	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
> > +	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
> >  					SMB2_CLIENT_GUID_SIZE);
> >
> >  	if (tcon->ses->sign)
> > -		vneg_inbuf.SecurityMode =
> > +		pneg_inbuf->SecurityMode =
> >
> 	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
> >  	else if (global_secflags & CIFSSEC_MAY_SIGN)
> > -		vneg_inbuf.SecurityMode =
> > +		pneg_inbuf->SecurityMode =
> >
> 	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
> >  	else
> > -		vneg_inbuf.SecurityMode = 0;
> > +		pneg_inbuf->SecurityMode = 0;
> >
> >
> >  	if (strcmp(tcon->ses->server->vals->version_string,
> Please check if strncmp() should be used or not.
> 
> >  		SMB3ANY_VERSION_STRING) == 0) {
> > -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> > -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> > -		vneg_inbuf.DialectCount = cpu_to_le16(2);
> > +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> > +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> > +		pneg_inbuf->DialectCount = cpu_to_le16(2);
> >  		/* structure is big enough for 3 dialects, sending only 2 */
> >  		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;
> >  	} else if (strcmp(tcon->ses->server->vals->version_string,
> >  		SMBDEFAULT_VERSION_STRING) == 0) {
> > -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> > -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> > -		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> > -		vneg_inbuf.DialectCount = cpu_to_le16(3);
> > +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> > +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> > +		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> > +		pneg_inbuf->DialectCount = cpu_to_le16(3);
> >  		/* structure is big enough for 3 dialects */
> >  		inbuflen = sizeof(struct validate_negotiate_info_req);
> >  	} else {
> >  		/* otherwise specific dialect was requested */
> > -		vneg_inbuf.Dialects[0] =
> > +		pneg_inbuf->Dialects[0] =
> >  			cpu_to_le16(tcon->ses->server->vals->protocol_id);
> > -		vneg_inbuf.DialectCount = cpu_to_le16(1);
> > +		pneg_inbuf->DialectCount = cpu_to_le16(1);
> >  		/* structure is big enough for 3 dialects, sending only 1 */
> >  		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
> >  	}
> >
> > -	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> > +	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> >  		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> > -		(char *)&vneg_inbuf, sizeof(struct
> > validate_negotiate_info_req),
> > +		(char *)pneg_inbuf, sizeof(struct
> validate_negotiate_info_req),
> 
> Use sizeof(*pneg_inbuf)
> 
> >  		(char **)&pneg_rsp, &rsplen);
> >
> > -	if (rc != 0) {
> > -		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
> > -		return -EIO;
> > +	if (ret != 0) {
> 
> if (ret) is fine.
> 
> > +		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
> > +		goto out_free_inbuf;
> >  	}
> >
> >  	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) { @@
> > -820,7
> 
> if (rsplen != sizeof(*pneg_rsp))

Re: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

[Steve French]

On Tue, Apr 17, 2018 at 3:11 PM, Long Li via samba-technical
<samba-technical@lists.samba.org> wrote:
>> Subject: RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through
>> kmalloc
>>
>>
>>
>> > -----Original Message-----
>> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>> > owner@vger.kernel.org] On Behalf Of Long Li
>> > Sent: Tuesday, April 17, 2018 2:17 PM
>> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
>> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org; linux-
>> > rdma@vger.kernel.org
>> > Cc: longli <longli@microsoft.com>; stable@vger.kernel.org
>> > Subject: [Patch v2 2/6] cifs: Allocate validate negotiation request
>> > through kmalloc
>> >
>> > From: Long Li <longli@microsoft.com>
>> >
>> > The data buffer allocated on the stack can't be DMA'ed, and hence
>> > can't send through RDMA via SMB Direct.
>> >
>> > Fix this by allocating the request on the heap in smb3_validate_negotiate.
>> >
>> > Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects
>> > against downgrade attacks"
>> >
>>
>> Format is:
>> Fixes: ff1c038addc4 ("Check SMB3 dialects against downgrade attacks") It
>> should be right above Signed-off signature.
>
> I will fix up and resend this patch.
>
> How about the rest patches (1, 3-6) in the series? If they don't need any changes, is it okay that I resend this one only?

Doesn't matter to me either way - I already merged patch 1 in any case.

>> > Changes in v2:
>> > Removed duplicated code on freeing buffers on function exit.
>> > (Thanks to Parav Pandit <parav@mellanox.com>)



-- 
Thanks,

Steve

RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

[Michael Kelley (EOSG)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Long Li
> Sent: Tuesday, April 17, 2018 12:17 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org; linux-rdma@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>; stable@vger.kernel.org
> Subject: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc
> 
> From: Long Li <longli@microsoft.com>
> 
> The data buffer allocated on the stack can't be DMA'ed, and hence can't send
> through RDMA via SMB Direct.
> 
> Fix this by allocating the request on the heap in smb3_validate_negotiate.
> 
> Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects against
> downgrade attacks"
> 
> Changes in v2:
> Removed duplicated code on freeing buffers on function exit.
> (Thanks to Parav Pandit <parav@mellanox.com>)
> 
> Fixed typo in the patch title.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/smb2pdu.c | 57 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 0f044c4..41625e4 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
> 
>  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon *tcon)
>  {
> -	int rc = 0;
> -	struct validate_negotiate_info_req vneg_inbuf;
> +	int ret, rc = -EIO;
> +	struct validate_negotiate_info_req *pneg_inbuf;
>  	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
>  	u32 rsplen;
>  	u32 inbuflen; /* max of 4 dialects */
> @@ -741,6 +741,9 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> *tcon)
>  	if (tcon->ses->server->rdma)
>  		return 0;
>  #endif
> +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
> +	if (!pneg_inbuf)
> +		return -ENOMEM;

Immediately after the above new code, there are three if statements that can
'return 0' and never free the pneg_inbuf memory.  They should instead set 'rc'
appropriately and 'goto' the out_free_inbuf label.

Michael

> 
>  	/* In SMB3.11 preauth integrity supersedes validate negotiate */
>  	if (tcon->ses->server->dialect == SMB311_PROT_ID)
> @@ -764,53 +767,53 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> *tcon)
>  	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
>  		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag sent by server\n");
> 
> -	vneg_inbuf.Capabilities =
> +	pneg_inbuf->Capabilities =
>  			cpu_to_le32(tcon->ses->server->vals->req_capabilities);
> -	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
> +	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
>  					SMB2_CLIENT_GUID_SIZE);
> 
>  	if (tcon->ses->sign)
> -		vneg_inbuf.SecurityMode =
> +		pneg_inbuf->SecurityMode =
>  			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
>  	else if (global_secflags & CIFSSEC_MAY_SIGN)
> -		vneg_inbuf.SecurityMode =
> +		pneg_inbuf->SecurityMode =
>  			cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
>  	else
> -		vneg_inbuf.SecurityMode = 0;
> +		pneg_inbuf->SecurityMode = 0;
> 
> 
>  	if (strcmp(tcon->ses->server->vals->version_string,
>  		SMB3ANY_VERSION_STRING) == 0) {
> -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> -		vneg_inbuf.DialectCount = cpu_to_le16(2);
> +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> +		pneg_inbuf->DialectCount = cpu_to_le16(2);
>  		/* structure is big enough for 3 dialects, sending only 2 */
>  		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;
>  	} else if (strcmp(tcon->ses->server->vals->version_string,
>  		SMBDEFAULT_VERSION_STRING) == 0) {
> -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> -		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> -		vneg_inbuf.DialectCount = cpu_to_le16(3);
> +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> +		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> +		pneg_inbuf->DialectCount = cpu_to_le16(3);
>  		/* structure is big enough for 3 dialects */
>  		inbuflen = sizeof(struct validate_negotiate_info_req);
>  	} else {
>  		/* otherwise specific dialect was requested */
> -		vneg_inbuf.Dialects[0] =
> +		pneg_inbuf->Dialects[0] =
>  			cpu_to_le16(tcon->ses->server->vals->protocol_id);
> -		vneg_inbuf.DialectCount = cpu_to_le16(1);
> +		pneg_inbuf->DialectCount = cpu_to_le16(1);
>  		/* structure is big enough for 3 dialects, sending only 1 */
>  		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
>  	}
> 
> -	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> +	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
>  		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> -		(char *)&vneg_inbuf, sizeof(struct validate_negotiate_info_req),
> +		(char *)pneg_inbuf, sizeof(struct validate_negotiate_info_req),
>  		(char **)&pneg_rsp, &rsplen);
> 
> -	if (rc != 0) {
> -		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
> -		return -EIO;
> +	if (ret != 0) {
> +		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
> +		goto out_free_inbuf;
>  	}
> 
>  	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) {
> @@ -820,7 +823,7 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> *tcon)
>  		/* relax check since Mac returns max bufsize allowed on ioctl */
>  		if ((rsplen > CIFSMaxBufSize)
>  		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
> -			goto err_rsp_free;
> +			goto out_free_rsp;
>  	}
> 
>  	/* check validate negotiate info response matches what we got earlier */
> @@ -838,14 +841,16 @@ int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> *tcon)
> 
>  	/* validate negotiate successful */
>  	cifs_dbg(FYI, "validate negotiate info successful\n");
> -	kfree(pneg_rsp);
> -	return 0;
> +	rc = 0;
> +	goto out_free_rsp;
> 
>  vneg_out:
>  	cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n");
> -err_rsp_free:
> +out_free_rsp:
>  	kfree(pneg_rsp);
> -	return -EIO;
> +out_free_inbuf:
> +	kfree(pneg_inbuf);
> +	return rc;
>  }
> 
>  enum securityEnum
> --
> 2.7.4

RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through kmalloc

[Long Li]

> Subject: RE: [Patch v2 2/6] cifs: Allocate validate negotiation request through
> kmalloc
> 
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org
> > <linux-kernel-owner@vger.kernel.org> On Behalf Of Long Li
> > Sent: Tuesday, April 17, 2018 12:17 PM
> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org;
> > linux-rdma@vger.kernel.org
> > Cc: Long Li <longli@microsoft.com>; stable@vger.kernel.org
> > Subject: [Patch v2 2/6] cifs: Allocate validate negotiation request
> > through kmalloc
> >
> > From: Long Li <longli@microsoft.com>
> >
> > The data buffer allocated on the stack can't be DMA'ed, and hence
> > can't send through RDMA via SMB Direct.
> >
> > Fix this by allocating the request on the heap in smb3_validate_negotiate.
> >
> > Fixes: ff1c038addc4f205d5f1ede449426c7d316c0eed "Check SMB3 dialects
> > against downgrade attacks"
> >
> > Changes in v2:
> > Removed duplicated code on freeing buffers on function exit.
> > (Thanks to Parav Pandit <parav@mellanox.com>)
> >
> > Fixed typo in the patch title.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/cifs/smb2pdu.c | 57
> > ++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 31 insertions(+), 26 deletions(-)
> >
> > diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index
> > 0f044c4..41625e4 100644
> > --- a/fs/cifs/smb2pdu.c
> > +++ b/fs/cifs/smb2pdu.c
> > @@ -729,8 +729,8 @@ SMB2_negotiate(const unsigned int xid, struct
> > cifs_ses *ses)
> >
> >  int smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> > *tcon)  {
> > -	int rc = 0;
> > -	struct validate_negotiate_info_req vneg_inbuf;
> > +	int ret, rc = -EIO;
> > +	struct validate_negotiate_info_req *pneg_inbuf;
> >  	struct validate_negotiate_info_rsp *pneg_rsp = NULL;
> >  	u32 rsplen;
> >  	u32 inbuflen; /* max of 4 dialects */ @@ -741,6 +741,9 @@ int
> > smb3_validate_negotiate(const unsigned int xid, struct cifs_tcon
> > *tcon)
> >  	if (tcon->ses->server->rdma)
> >  		return 0;
> >  #endif
> > +	pneg_inbuf = kmalloc(sizeof(*pneg_inbuf), GFP_KERNEL);
> > +	if (!pneg_inbuf)
> > +		return -ENOMEM;
> 
> Immediately after the above new code, there are three if statements that
> can 'return 0' and never free the pneg_inbuf memory.  They should instead
> set 'rc'
> appropriately and 'goto' the out_free_inbuf label.

Thanks!

I will move the kmalloc after those statements.

> 
> Michael
> 
> >
> >  	/* In SMB3.11 preauth integrity supersedes validate negotiate */
> >  	if (tcon->ses->server->dialect == SMB311_PROT_ID) @@ -764,53
> +767,53
> > @@ int smb3_validate_negotiate(const unsigned int xid, struct
> > cifs_tcon
> > *tcon)
> >  	if (tcon->ses->session_flags & SMB2_SESSION_FLAG_IS_NULL)
> >  		cifs_dbg(VFS, "Unexpected null user (anonymous) auth flag
> sent by
> > server\n");
> >
> > -	vneg_inbuf.Capabilities =
> > +	pneg_inbuf->Capabilities =
> >  			cpu_to_le32(tcon->ses->server->vals-
> >req_capabilities);
> > -	memcpy(vneg_inbuf.Guid, tcon->ses->server->client_guid,
> > +	memcpy(pneg_inbuf->Guid, tcon->ses->server->client_guid,
> >  					SMB2_CLIENT_GUID_SIZE);
> >
> >  	if (tcon->ses->sign)
> > -		vneg_inbuf.SecurityMode =
> > +		pneg_inbuf->SecurityMode =
> >
> 	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_REQUIRED);
> >  	else if (global_secflags & CIFSSEC_MAY_SIGN)
> > -		vneg_inbuf.SecurityMode =
> > +		pneg_inbuf->SecurityMode =
> >
> 	cpu_to_le16(SMB2_NEGOTIATE_SIGNING_ENABLED);
> >  	else
> > -		vneg_inbuf.SecurityMode = 0;
> > +		pneg_inbuf->SecurityMode = 0;
> >
> >
> >  	if (strcmp(tcon->ses->server->vals->version_string,
> >  		SMB3ANY_VERSION_STRING) == 0) {
> > -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> > -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> > -		vneg_inbuf.DialectCount = cpu_to_le16(2);
> > +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB30_PROT_ID);
> > +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB302_PROT_ID);
> > +		pneg_inbuf->DialectCount = cpu_to_le16(2);
> >  		/* structure is big enough for 3 dialects, sending only 2 */
> >  		inbuflen = sizeof(struct validate_negotiate_info_req) - 2;
> >  	} else if (strcmp(tcon->ses->server->vals->version_string,
> >  		SMBDEFAULT_VERSION_STRING) == 0) {
> > -		vneg_inbuf.Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> > -		vneg_inbuf.Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> > -		vneg_inbuf.Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> > -		vneg_inbuf.DialectCount = cpu_to_le16(3);
> > +		pneg_inbuf->Dialects[0] = cpu_to_le16(SMB21_PROT_ID);
> > +		pneg_inbuf->Dialects[1] = cpu_to_le16(SMB30_PROT_ID);
> > +		pneg_inbuf->Dialects[2] = cpu_to_le16(SMB302_PROT_ID);
> > +		pneg_inbuf->DialectCount = cpu_to_le16(3);
> >  		/* structure is big enough for 3 dialects */
> >  		inbuflen = sizeof(struct validate_negotiate_info_req);
> >  	} else {
> >  		/* otherwise specific dialect was requested */
> > -		vneg_inbuf.Dialects[0] =
> > +		pneg_inbuf->Dialects[0] =
> >  			cpu_to_le16(tcon->ses->server->vals->protocol_id);
> > -		vneg_inbuf.DialectCount = cpu_to_le16(1);
> > +		pneg_inbuf->DialectCount = cpu_to_le16(1);
> >  		/* structure is big enough for 3 dialects, sending only 1 */
> >  		inbuflen = sizeof(struct validate_negotiate_info_req) - 4;
> >  	}
> >
> > -	rc = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> > +	ret = SMB2_ioctl(xid, tcon, NO_FILE_ID, NO_FILE_ID,
> >  		FSCTL_VALIDATE_NEGOTIATE_INFO, true /* is_fsctl */,
> > -		(char *)&vneg_inbuf, sizeof(struct
> validate_negotiate_info_req),
> > +		(char *)pneg_inbuf, sizeof(struct
> validate_negotiate_info_req),
> >  		(char **)&pneg_rsp, &rsplen);
> >
> > -	if (rc != 0) {
> > -		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", rc);
> > -		return -EIO;
> > +	if (ret != 0) {
> > +		cifs_dbg(VFS, "validate protocol negotiate failed: %d\n", ret);
> > +		goto out_free_inbuf;
> >  	}
> >
> >  	if (rsplen != sizeof(struct validate_negotiate_info_rsp)) { @@
> > -820,7 +823,7 @@ int smb3_validate_negotiate(const unsigned int xid,
> > struct cifs_tcon
> > *tcon)
> >  		/* relax check since Mac returns max bufsize allowed on ioctl
> */
> >  		if ((rsplen > CIFSMaxBufSize)
> >  		     || (rsplen < sizeof(struct validate_negotiate_info_rsp)))
> > -			goto err_rsp_free;
> > +			goto out_free_rsp;
> >  	}
> >
> >  	/* check validate negotiate info response matches what we got
> > earlier */ @@ -838,14 +841,16 @@ int smb3_validate_negotiate(const
> > unsigned int xid, struct cifs_tcon
> > *tcon)
> >
> >  	/* validate negotiate successful */
> >  	cifs_dbg(FYI, "validate negotiate info successful\n");
> > -	kfree(pneg_rsp);
> > -	return 0;
> > +	rc = 0;
> > +	goto out_free_rsp;
> >
> >  vneg_out:
> >  	cifs_dbg(VFS, "protocol revalidation - security settings
> > mismatch\n");
> > -err_rsp_free:
> > +out_free_rsp:
> >  	kfree(pneg_rsp);
> > -	return -EIO;
> > +out_free_inbuf:
> > +	kfree(pneg_inbuf);
> > +	return rc;
> >  }
> >
> >  enum securityEnum
> > --
> > 2.7.4

RE: [Patch v2 1/6] cifs: smbd: Check for iov length on sending the last iov

[Michael Kelley (EOSG)]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-owner@vger.kernel.org> On Behalf
> Of Long Li
> Sent: Tuesday, April 17, 2018 12:17 PM
> To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org; samba-
> technical@lists.samba.org; linux-kernel@vger.kernel.org; linux-rdma@vger.kernel.org
> Cc: Long Li <longli@microsoft.com>; stable@vger.kernel.org
> Subject: [Patch v2 1/6] cifs: smbd: Check for iov length on sending the last iov
> 
> From: Long Li <longli@microsoft.com>
> 
> When sending the last iov that breaks into smaller buffers to fit the
> transfer size, it's necessary to check if this is the last iov.
> 
> If this is the latest iov, stop and proceed to send pages.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org

Reviewed-by: Michael Kelley <mikelley@microsoft.com>

But a question unrelated to this change arose during my review:  At the
beginning and end of smbd_send(), the field smbd_send_pending is
incremented and decremented, respectively.   The increment/decrement
are not done as atomic operations.  Is this code guaranteed to be single
threaded?  If not, the count could become corrupted, and
smbd_destroy_rdma_work(), which waits for the count to become zero,
could hang.  A similar question applies to smbd_recv_pending in smbd_recv().

> ---
>  fs/cifs/smbdirect.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index 90e673c..b5c6c0d 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2197,6 +2197,8 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>  						goto done;
>  				}
>  				i++;
> +				if (i == rqst->rq_nvec)
> +					break;
>  			}
>  			start = i;
>  			buflen = 0;
> --
> 2.7.4

Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack

[Steve French]

Didn't see any obvious problems, but can you fix the checkpatch
warnings and resend to the list (I am more concerned about the last
two warnings rather than the first one).

$ scripts/checkpatch.pl 0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch
WARNING: line over 80 characters
#60: FILE: fs/cifs/smbdirect.c:2106:
+        log_write(ERR, "expected the pdu length in 1st iov, but got
0x%lu\n", rqst->rq_iov[0].iov_len);

ERROR: Prefixing 0x with decimal output is defective
#60: FILE: fs/cifs/smbdirect.c:2106:
+        log_write(ERR, "expected the pdu length in 1st iov, but got
0x%lu\n", rqst->rq_iov[0].iov_len);

WARNING: braces {} are not necessary for single statement blocks
#69: FILE: fs/cifs/smbdirect.c:2112:
+    for (i = 0; i < rqst->rq_nvec-1; i++) {
         buflen += iov[i].iov_len;
     }

total: 1 errors, 2 warnings, 65 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch has style
problems, please review.

On Tue, Apr 17, 2018 at 2:17 PM, Long Li <longli@linuxonhyperv.com> wrote:
> From: Long Li <longli@microsoft.com>
>
> It's not necessary to allocate another iov when going through the buffers
> in smbd_send() through RDMA send.
>
> Remove it to reduce stack size.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
>  1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index b5c6c0d..f575e9a 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -2088,7 +2088,7 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>         int start, i, j;
>         int max_iov_size =
>                 info->max_send_size - sizeof(struct smbd_data_transfer);
> -       struct kvec iov[SMBDIRECT_MAX_SGE];
> +       struct kvec *iov;
>         int rc;
>
>         info->smbd_send_pending++;
> @@ -2099,32 +2099,20 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>         }
>
>         /*
> -        * This usually means a configuration error
> -        * We use RDMA read/write for packet size > rdma_readwrite_threshold
> -        * as long as it's properly configured we should never get into this
> -        * situation
> -        */
> -       if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
> -               log_write(ERR, "maximum send segment %x exceeding %x\n",
> -                        rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
> -               rc = -EINVAL;
> -               goto done;
> -       }
> -
> -       /*
> -        * Remove the RFC1002 length defined in MS-SMB2 section 2.1
> -        * It is used only for TCP transport
> +        * Skip the RFC1002 length defined in MS-SMB2 section 2.1
> +        * It is used only for TCP transport in the iov[0]
>          * In future we may want to add a transport layer under protocol
>          * layer so this will only be issued to TCP transport
>          */
> -       iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
> -       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
> -       buflen += iov[0].iov_len;
> +
> +       if (rqst->rq_iov[0].iov_len != 4) {
> +               log_write(ERR, "expected the pdu length in 1st iov, but got 0x%lu\n", rqst->rq_iov[0].iov_len);
> +               return -EINVAL;
> +       }
> +       iov = &rqst->rq_iov[1];
>
>         /* total up iov array first */
> -       for (i = 1; i < rqst->rq_nvec; i++) {
> -               iov[i].iov_base = rqst->rq_iov[i].iov_base;
> -               iov[i].iov_len = rqst->rq_iov[i].iov_len;
> +       for (i = 0; i < rqst->rq_nvec-1; i++) {
>                 buflen += iov[i].iov_len;
>         }
>
> @@ -2197,14 +2185,14 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
>                                                 goto done;
>                                 }
>                                 i++;
> -                               if (i == rqst->rq_nvec)
> +                               if (i == rqst->rq_nvec-1)
>                                         break;
>                         }
>                         start = i;
>                         buflen = 0;
>                 } else {
>                         i++;
> -                       if (i == rqst->rq_nvec) {
> +                       if (i == rqst->rq_nvec-1) {
>                                 /* send out all remaining vecs */
>                                 remaining_data_length -= buflen;
>                                 log_write(INFO,
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

Re: [Patch v2 4/6] cifs: smbd: Don't use RDMA read/write when signing is used

[Steve French]

merged into cifs-2.6.git for-next

On Tue, Apr 17, 2018 at 2:17 PM, Long Li <longli@linuxonhyperv.com> wrote:
> From: Long Li <longli@microsoft.com>
>
> SMB server will not sign data transferred through RDMA read/write. When
> signing is used, it's a good idea to have all the data signed.
>
> In this case, use RDMA send/recv for all data transfers. This will degrade
> performance as this is not generally configured in RDMA environemnt. So
> warn the user on signing and RDMA send/recv.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> Cc: stable@vger.kernel.org
> ---
>  fs/cifs/cifssmb.c |  3 +++
>  fs/cifs/smb2ops.c | 18 ++++++++++++++----
>  fs/cifs/smb2pdu.c |  4 ++--
>  3 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 6d3e40d..1529a08 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -455,6 +455,9 @@ cifs_enable_signing(struct TCP_Server_Info *server, bool mnt_sign_required)
>                 server->sign = true;
>         }
>
> +       if (cifs_rdma_enabled(server) && server->sign)
> +               cifs_dbg(VFS, "Signing is enabled, and RDMA read/write will be disabled");
> +
>         return 0;
>  }
>
> diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
> index 38ebf3f..b76b858 100644
> --- a/fs/cifs/smb2ops.c
> +++ b/fs/cifs/smb2ops.c
> @@ -252,9 +252,14 @@ smb2_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>         wsize = volume_info->wsize ? volume_info->wsize : CIFS_DEFAULT_IOSIZE;
>         wsize = min_t(unsigned int, wsize, server->max_write);
>  #ifdef CONFIG_CIFS_SMB_DIRECT
> -       if (server->rdma)
> -               wsize = min_t(unsigned int,
> +       if (server->rdma) {
> +               if (server->sign)
> +                       wsize = min_t(unsigned int,
> +                               wsize, server->smbd_conn->max_fragmented_send_size);
> +               else
> +                       wsize = min_t(unsigned int,
>                                 wsize, server->smbd_conn->max_readwrite_size);
> +       }
>  #endif
>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
>                 wsize = min_t(unsigned int, wsize, SMB2_MAX_BUFFER_SIZE);
> @@ -272,9 +277,14 @@ smb2_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *volume_info)
>         rsize = volume_info->rsize ? volume_info->rsize : CIFS_DEFAULT_IOSIZE;
>         rsize = min_t(unsigned int, rsize, server->max_read);
>  #ifdef CONFIG_CIFS_SMB_DIRECT
> -       if (server->rdma)
> -               rsize = min_t(unsigned int,
> +       if (server->rdma) {
> +               if (server->sign)
> +                       rsize = min_t(unsigned int,
> +                               rsize, server->smbd_conn->max_fragmented_recv_size);
> +               else
> +                       rsize = min_t(unsigned int,
>                                 rsize, server->smbd_conn->max_readwrite_size);
> +       }
>  #endif
>
>         if (!(server->capabilities & SMB2_GLOBAL_CAP_LARGE_MTU))
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 41625e4..33f612f 100644
> --- a/fs/cifs/smb2pdu.c
> +++ b/fs/cifs/smb2pdu.c
> @@ -2595,7 +2595,7 @@ smb2_new_read_req(void **buf, unsigned int *total_len,
>          * If we want to do a RDMA write, fill in and append
>          * smbd_buffer_descriptor_v1 to the end of read request
>          */
> -       if (server->rdma && rdata &&
> +       if (server->rdma && rdata && !server->sign &&
>                 rdata->bytes >= server->smbd_conn->rdma_readwrite_threshold) {
>
>                 struct smbd_buffer_descriptor_v1 *v1;
> @@ -2973,7 +2973,7 @@ smb2_async_writev(struct cifs_writedata *wdata,
>          * If we want to do a server RDMA read, fill in and append
>          * smbd_buffer_descriptor_v1 to the end of write request
>          */
> -       if (server->rdma && wdata->bytes >=
> +       if (server->rdma && !server->sign && wdata->bytes >=
>                 server->smbd_conn->rdma_readwrite_threshold) {
>
>                 struct smbd_buffer_descriptor_v1 *v1;
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks,

Steve

RE: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack

[Long Li]

> Subject: Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack
> 
> Didn't see any obvious problems, but can you fix the checkpatch warnings
> and resend to the list (I am more concerned about the last two warnings
> rather than the first one).

Yes, I will fix it and resend.

> 
> $ scripts/checkpatch.pl 0001-cifs-smbd-Avoid-allocating-iov-on-the-
> stack.patch
> WARNING: line over 80 characters
> #60: FILE: fs/cifs/smbdirect.c:2106:
> +        log_write(ERR, "expected the pdu length in 1st iov, but got
> 0x%lu\n", rqst->rq_iov[0].iov_len);
> 
> ERROR: Prefixing 0x with decimal output is defective
> #60: FILE: fs/cifs/smbdirect.c:2106:
> +        log_write(ERR, "expected the pdu length in 1st iov, but got
> 0x%lu\n", rqst->rq_iov[0].iov_len);
> 
> WARNING: braces {} are not necessary for single statement blocks
> #69: FILE: fs/cifs/smbdirect.c:2112:
> +    for (i = 0; i < rqst->rq_nvec-1; i++) {
>          buflen += iov[i].iov_len;
>      }
> 
> total: 1 errors, 2 warnings, 65 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> 0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch has style problems,
> please review.
> 
> On Tue, Apr 17, 2018 at 2:17 PM, Long Li <longli@linuxonhyperv.com> wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > It's not necessary to allocate another iov when going through the
> > buffers in smbd_send() through RDMA send.
> >
> > Remove it to reduce stack size.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
> >  1 file changed, 12 insertions(+), 24 deletions(-)
> >
> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
> > b5c6c0d..f575e9a 100644
> > --- a/fs/cifs/smbdirect.c
> > +++ b/fs/cifs/smbdirect.c
> > @@ -2088,7 +2088,7 @@ int smbd_send(struct smbd_connection *info,
> struct smb_rqst *rqst)
> >         int start, i, j;
> >         int max_iov_size =
> >                 info->max_send_size - sizeof(struct smbd_data_transfer);
> > -       struct kvec iov[SMBDIRECT_MAX_SGE];
> > +       struct kvec *iov;
> >         int rc;
> >
> >         info->smbd_send_pending++;
> > @@ -2099,32 +2099,20 @@ int smbd_send(struct smbd_connection *info,
> struct smb_rqst *rqst)
> >         }
> >
> >         /*
> > -        * This usually means a configuration error
> > -        * We use RDMA read/write for packet size >
> rdma_readwrite_threshold
> > -        * as long as it's properly configured we should never get into this
> > -        * situation
> > -        */
> > -       if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
> > -               log_write(ERR, "maximum send segment %x exceeding %x\n",
> > -                        rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
> > -               rc = -EINVAL;
> > -               goto done;
> > -       }
> > -
> > -       /*
> > -        * Remove the RFC1002 length defined in MS-SMB2 section 2.1
> > -        * It is used only for TCP transport
> > +        * Skip the RFC1002 length defined in MS-SMB2 section 2.1
> > +        * It is used only for TCP transport in the iov[0]
> >          * In future we may want to add a transport layer under protocol
> >          * layer so this will only be issued to TCP transport
> >          */
> > -       iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
> > -       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
> > -       buflen += iov[0].iov_len;
> > +
> > +       if (rqst->rq_iov[0].iov_len != 4) {
> > +               log_write(ERR, "expected the pdu length in 1st iov, but got
> 0x%lu\n", rqst->rq_iov[0].iov_len);
> > +               return -EINVAL;
> > +       }
> > +       iov = &rqst->rq_iov[1];
> >
> >         /* total up iov array first */
> > -       for (i = 1; i < rqst->rq_nvec; i++) {
> > -               iov[i].iov_base = rqst->rq_iov[i].iov_base;
> > -               iov[i].iov_len = rqst->rq_iov[i].iov_len;
> > +       for (i = 0; i < rqst->rq_nvec-1; i++) {
> >                 buflen += iov[i].iov_len;
> >         }
> >
> > @@ -2197,14 +2185,14 @@ int smbd_send(struct smbd_connection *info,
> struct smb_rqst *rqst)
> >                                                 goto done;
> >                                 }
> >                                 i++;
> > -                               if (i == rqst->rq_nvec)
> > +                               if (i == rqst->rq_nvec-1)
> >                                         break;
> >                         }
> >                         start = i;
> >                         buflen = 0;
> >                 } else {
> >                         i++;
> > -                       if (i == rqst->rq_nvec) {
> > +                       if (i == rqst->rq_nvec-1) {
> >                                 /* send out all remaining vecs */
> >                                 remaining_data_length -= buflen;
> >                                 log_write(INFO,
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs"
> > in the body of a message to majordomo@vger.kernel.org More
> majordomo
> > info at
> >
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
> > ernel.org%2Fmajordomo-
> info.html&data=02%7C01%7Clongli%40microsoft.com%
> >
> 7C23fbdbb0598a497f624708d5a92f6435%7C72f988bf86f141af91ab2d7cd011d
> b47%
> >
> 7C1%7C0%7C636600943380887988&sdata=imh9cWDQQEXqxHdUrCuEykgiLfKs
> UAl20jB
> > yPOS7FrI%3D&reserved=0
> 
> 
> 
> --
> Thanks,
> 
> Steve
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the
> body of a message to majordomo@vger.kernel.org More majordomo info at
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html&data=02%7C01%7Clongli%40microsoft.com%7C23fbdbb0598a497f
> 624708d5a92f6435%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63
> 6600943380887988&sdata=imh9cWDQQEXqxHdUrCuEykgiLfKsUAl20jByPOS7F
> rI%3D&reserved=0

Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 6595 bytes --]

I am ok with the other style warning - so I just fixed up the one
error (see attached) and merged into cifs-2.6.git for-next

That leaves two more from your series to review (and for others to
review).  Let's just focus on those two.


On Mon, Apr 23, 2018 at 12:33 PM, Long Li <longli@microsoft.com> wrote:
>> Subject: Re: [Patch v2 3/6] cifs: smbd: Avoid allocating iov on the stack
>>
>> Didn't see any obvious problems, but can you fix the checkpatch warnings
>> and resend to the list (I am more concerned about the last two warnings
>> rather than the first one).
>
> Yes, I will fix it and resend.
>
>>
>> $ scripts/checkpatch.pl 0001-cifs-smbd-Avoid-allocating-iov-on-the-
>> stack.patch
>> WARNING: line over 80 characters
>> #60: FILE: fs/cifs/smbdirect.c:2106:
>> +        log_write(ERR, "expected the pdu length in 1st iov, but got
>> 0x%lu\n", rqst->rq_iov[0].iov_len);
>>
>> ERROR: Prefixing 0x with decimal output is defective
>> #60: FILE: fs/cifs/smbdirect.c:2106:
>> +        log_write(ERR, "expected the pdu length in 1st iov, but got
>> 0x%lu\n", rqst->rq_iov[0].iov_len);
>>
>> WARNING: braces {} are not necessary for single statement blocks
>> #69: FILE: fs/cifs/smbdirect.c:2112:
>> +    for (i = 0; i < rqst->rq_nvec-1; i++) {
>>          buflen += iov[i].iov_len;
>>      }
>>
>> total: 1 errors, 2 warnings, 65 lines checked
>>
>> NOTE: For some of the reported defects, checkpatch may be able to
>>       mechanically convert to the typical style using --fix or --fix-inplace.
>>
>> 0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch has style problems,
>> please review.
>>
>> On Tue, Apr 17, 2018 at 2:17 PM, Long Li <longli@linuxonhyperv.com> wrote:
>> > From: Long Li <longli@microsoft.com>
>> >
>> > It's not necessary to allocate another iov when going through the
>> > buffers in smbd_send() through RDMA send.
>> >
>> > Remove it to reduce stack size.
>> >
>> > Signed-off-by: Long Li <longli@microsoft.com>
>> > Cc: stable@vger.kernel.org
>> > ---
>> >  fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
>> >  1 file changed, 12 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
>> > b5c6c0d..f575e9a 100644
>> > --- a/fs/cifs/smbdirect.c
>> > +++ b/fs/cifs/smbdirect.c
>> > @@ -2088,7 +2088,7 @@ int smbd_send(struct smbd_connection *info,
>> struct smb_rqst *rqst)
>> >         int start, i, j;
>> >         int max_iov_size =
>> >                 info->max_send_size - sizeof(struct smbd_data_transfer);
>> > -       struct kvec iov[SMBDIRECT_MAX_SGE];
>> > +       struct kvec *iov;
>> >         int rc;
>> >
>> >         info->smbd_send_pending++;
>> > @@ -2099,32 +2099,20 @@ int smbd_send(struct smbd_connection *info,
>> struct smb_rqst *rqst)
>> >         }
>> >
>> >         /*
>> > -        * This usually means a configuration error
>> > -        * We use RDMA read/write for packet size >
>> rdma_readwrite_threshold
>> > -        * as long as it's properly configured we should never get into this
>> > -        * situation
>> > -        */
>> > -       if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
>> > -               log_write(ERR, "maximum send segment %x exceeding %x\n",
>> > -                        rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
>> > -               rc = -EINVAL;
>> > -               goto done;
>> > -       }
>> > -
>> > -       /*
>> > -        * Remove the RFC1002 length defined in MS-SMB2 section 2.1
>> > -        * It is used only for TCP transport
>> > +        * Skip the RFC1002 length defined in MS-SMB2 section 2.1
>> > +        * It is used only for TCP transport in the iov[0]
>> >          * In future we may want to add a transport layer under protocol
>> >          * layer so this will only be issued to TCP transport
>> >          */
>> > -       iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
>> > -       iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
>> > -       buflen += iov[0].iov_len;
>> > +
>> > +       if (rqst->rq_iov[0].iov_len != 4) {
>> > +               log_write(ERR, "expected the pdu length in 1st iov, but got
>> 0x%lu\n", rqst->rq_iov[0].iov_len);
>> > +               return -EINVAL;
>> > +       }
>> > +       iov = &rqst->rq_iov[1];
>> >
>> >         /* total up iov array first */
>> > -       for (i = 1; i < rqst->rq_nvec; i++) {
>> > -               iov[i].iov_base = rqst->rq_iov[i].iov_base;
>> > -               iov[i].iov_len = rqst->rq_iov[i].iov_len;
>> > +       for (i = 0; i < rqst->rq_nvec-1; i++) {
>> >                 buflen += iov[i].iov_len;
>> >         }
>> >
>> > @@ -2197,14 +2185,14 @@ int smbd_send(struct smbd_connection *info,
>> struct smb_rqst *rqst)
>> >                                                 goto done;
>> >                                 }
>> >                                 i++;
>> > -                               if (i == rqst->rq_nvec)
>> > +                               if (i == rqst->rq_nvec-1)
>> >                                         break;
>> >                         }
>> >                         start = i;
>> >                         buflen = 0;
>> >                 } else {
>> >                         i++;
>> > -                       if (i == rqst->rq_nvec) {
>> > +                       if (i == rqst->rq_nvec-1) {
>> >                                 /* send out all remaining vecs */
>> >                                 remaining_data_length -= buflen;
>> >                                 log_write(INFO,
>> > --
>> > 2.7.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-cifs"
>> > in the body of a message to majordomo@vger.kernel.org More
>> majordomo
>> > info at
>> >
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
>> > ernel.org%2Fmajordomo-
>> info.html&data=02%7C01%7Clongli%40microsoft.com%
>> >
>> 7C23fbdbb0598a497f624708d5a92f6435%7C72f988bf86f141af91ab2d7cd011d
>> b47%
>> >
>> 7C1%7C0%7C636600943380887988&sdata=imh9cWDQQEXqxHdUrCuEykgiLfKs
>> UAl20jB
>> > yPOS7FrI%3D&reserved=0
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the
>> body of a message to majordomo@vger.kernel.org More majordomo info at
>> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
>> rnel.org%2Fmajordomo-
>> info.html&data=02%7C01%7Clongli%40microsoft.com%7C23fbdbb0598a497f
>> 624708d5a92f6435%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63
>> 6600943380887988&sdata=imh9cWDQQEXqxHdUrCuEykgiLfKsUAl20jByPOS7F
>> rI%3D&reserved=0



-- 
Thanks,

Steve

[-- Attachment #2: 0001-cifs-smbd-Avoid-allocating-iov-on-the-stack.patch --]
[-- Type: text/x-patch, Size: 2847 bytes --]

From 553018657de265a7c999f4fd12864fd0646319a1 Mon Sep 17 00:00:00 2001
From: Long Li <longli@microsoft.com>
Date: Tue, 17 Apr 2018 12:17:07 -0700
Subject: [PATCH] cifs: smbd: Avoid allocating iov on the stack

It's not necessary to allocate another iov when going through the buffers
in smbd_send() through RDMA send.

Remove it to reduce stack size.

Signed-off-by: Long Li <longli@microsoft.com>
Cc: stable@vger.kernel.org
---
 fs/cifs/smbdirect.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index 87817ddcc096..24cea63e17f5 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -2086,7 +2086,7 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 	int start, i, j;
 	int max_iov_size =
 		info->max_send_size - sizeof(struct smbd_data_transfer);
-	struct kvec iov[SMBDIRECT_MAX_SGE];
+	struct kvec *iov;
 	int rc;
 
 	info->smbd_send_pending++;
@@ -2096,32 +2096,20 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 	}
 
 	/*
-	 * This usually means a configuration error
-	 * We use RDMA read/write for packet size > rdma_readwrite_threshold
-	 * as long as it's properly configured we should never get into this
-	 * situation
-	 */
-	if (rqst->rq_nvec + rqst->rq_npages > SMBDIRECT_MAX_SGE) {
-		log_write(ERR, "maximum send segment %x exceeding %x\n",
-			 rqst->rq_nvec + rqst->rq_npages, SMBDIRECT_MAX_SGE);
-		rc = -EINVAL;
-		goto done;
-	}
-
-	/*
-	 * Remove the RFC1002 length defined in MS-SMB2 section 2.1
-	 * It is used only for TCP transport
+	 * Skip the RFC1002 length defined in MS-SMB2 section 2.1
+	 * It is used only for TCP transport in the iov[0]
 	 * In future we may want to add a transport layer under protocol
 	 * layer so this will only be issued to TCP transport
 	 */
-	iov[0].iov_base = (char *)rqst->rq_iov[0].iov_base + 4;
-	iov[0].iov_len = rqst->rq_iov[0].iov_len - 4;
-	buflen += iov[0].iov_len;
+
+	if (rqst->rq_iov[0].iov_len != 4) {
+		log_write(ERR, "expected the pdu length in 1st iov, but got %lu\n", rqst->rq_iov[0].iov_len);
+		return -EINVAL;
+	}
+	iov = &rqst->rq_iov[1];
 
 	/* total up iov array first */
-	for (i = 1; i < rqst->rq_nvec; i++) {
-		iov[i].iov_base = rqst->rq_iov[i].iov_base;
-		iov[i].iov_len = rqst->rq_iov[i].iov_len;
+	for (i = 0; i < rqst->rq_nvec-1; i++) {
 		buflen += iov[i].iov_len;
 	}
 
@@ -2198,14 +2186,14 @@ int smbd_send(struct smbd_connection *info, struct smb_rqst *rqst)
 						goto done;
 				}
 				i++;
-				if (i == rqst->rq_nvec)
+				if (i == rqst->rq_nvec-1)
 					break;
 			}
 			start = i;
 			buflen = 0;
 		} else {
 			i++;
-			if (i == rqst->rq_nvec) {
+			if (i == rqst->rq_nvec-1) {
 				/* send out all remaining vecs */
 				remaining_data_length -= buflen;
 				log_write(INFO,
-- 
2.14.1

RE: [Patch v2 1/6] cifs: smbd: Check for iov length on sending the last iov

[Long Li]

> Subject: RE: [Patch v2 1/6] cifs: smbd: Check for iov length on sending the last
> iov
> 
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org
> > <linux-kernel-owner@vger.kernel.org> On Behalf Of Long Li
> > Sent: Tuesday, April 17, 2018 12:17 PM
> > To: Steve French <sfrench@samba.org>; linux-cifs@vger.kernel.org;
> > samba- technical@lists.samba.org; linux-kernel@vger.kernel.org;
> > linux-rdma@vger.kernel.org
> > Cc: Long Li <longli@microsoft.com>; stable@vger.kernel.org
> > Subject: [Patch v2 1/6] cifs: smbd: Check for iov length on sending
> > the last iov
> >
> > From: Long Li <longli@microsoft.com>
> >
> > When sending the last iov that breaks into smaller buffers to fit the
> > transfer size, it's necessary to check if this is the last iov.
> >
> > If this is the latest iov, stop and proceed to send pages.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > Cc: stable@vger.kernel.org
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> 
> But a question unrelated to this change arose during my review:  At the
> beginning and end of smbd_send(), the field smbd_send_pending is
> incremented and decremented, respectively.   The increment/decrement
> are not done as atomic operations.  Is this code guaranteed to be single
> threaded?  If not, the count could become corrupted, and
> smbd_destroy_rdma_work(), which waits for the count to become zero,
> could hang.  A similar question applies to smbd_recv_pending in smbd_recv().

It is safe. The transport sending path is locked by TCP_Server_Info->srv_mutex.

The transport receiving path is single threaded.

> 
> > ---
> >  fs/cifs/smbdirect.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c index
> > 90e673c..b5c6c0d 100644
> > --- a/fs/cifs/smbdirect.c
> > +++ b/fs/cifs/smbdirect.c
> > @@ -2197,6 +2197,8 @@ int smbd_send(struct smbd_connection *info,
> struct smb_rqst *rqst)
> >  						goto done;
> >  				}
> >  				i++;
> > +				if (i == rqst->rq_nvec)
> > +					break;
> >  			}
> >  			start = i;
> >  			buflen = 0;
> > --
> > 2.7.4