-----Original Message----- From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- owner@vger.kernel.org] On Behalf Of Long Li Sent: Monday, April 16, 2018 7:49 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; stable@vger.kernel.org Cc: longli longli@microsoft.com Subject: [PATCH 2/6] cifs: Allocate validate negoation 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.

Please provide Fixes ("12-letters commit id") "commit string" which introduced this issue and it is getting fixed here, so that other can apply this fix to older versions.

Signed-off-by: Long Li longli@microsoft.com

fs/cifs/smb2pdu.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 0f044c4..abbefe2 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -730,7 +730,7 @@ 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;

• struct validate_negotiate_info_req *pneg_inbuf;

[..]

rc = 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);

• kfree(pneg_inbuf);
  

  return -EIO;

Instead of duplicating code here, please jump to err_rsp_free label. Kfree() takes care to not panic for NULL pointer. Or for clarity define new label.

}

@@ -838,12 +842,14 @@ 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_inbuf); kfree(pneg_rsp); return 0;

vneg_out: cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n"); err_rsp_free:

• kfree(pneg_inbuf); kfree(pneg_rsp); return -EIO;

}

2.7.4

-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Mon, Apr 16, 2018 at 05:49:14PM -0700, Long Li wrote:

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.

Signed-off-by: Long Li longli@microsoft.com

fs/cifs/smb2pdu.c | 38 ++++++++++++++++++++++---------------- 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 0f044c4..abbefe2 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -730,7 +730,7 @@ 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;

• 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,52 +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, 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);

• kfree(pneg_inbuf);
  

  return -EIO; }

@@ -838,12 +842,14 @@ 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_inbuf); kfree(pneg_rsp); return 0;

vneg_out: cifs_dbg(VFS, "protocol revalidation - security settings mismatch\n"); err_rsp_free:

• kfree(pneg_inbuf); kfree(pneg_rsp); return -EIO;

}

2.7.4

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.

</formletter>

On Mon, Apr 16, 2018 at 05:49:15PM -0700, Long Li 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

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

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.

</formletter>

On Mon, Apr 16, 2018 at 05:49:16PM -0700, Long Li 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

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 abbefe2..6759035 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2596,7 +2596,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; @@ -2974,7 +2974,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

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.

</formletter>

On Mon, Apr 16, 2018 at 05:49:17PM -0700, Long Li wrote:

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

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 6759035..d71ce51 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

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.

</formletter>

On Mon, Apr 16, 2018 at 05:49:18PM -0700, Long Li wrote:

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

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

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.

</formletter>

Subject: Re: [PATCH 1/6] cifs: smbd: Check for iov length on sending the last iov

On Mon, Apr 16, 2018 at 05:49:13PM -0700, Long Li wrote:

...

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

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

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:

https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. kernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fstable-kernel- rules.html&data=02%7C01%7Clongli%40microsoft.com%7Cec2fc0284244483b 25bf08d5a434f6dc%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636 595469807590402&sdata=YXqnaTFgRyUyN1ubhCcyblT2ni%2F%2BCowPYJSFje 6PuCk%3D&reserved=0 for how to do this properly.

</formletter>

Will do. Thank you.

-- 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%7Cec2fc0284244483b2 5bf08d5a434f6dc%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6365 95469807590402&sdata=uu9VQ%2BHscmeFJH6kQEf39G2a7Y8M9hMmvBI2s9 T1DXs%3D&reserved=0