[PATCH 0/3] nvmet-auth: auth send / receive cleanup

[PATCH 0/3] nvmet-auth: auth send / receive cleanup

[Chaitanya Kulkarni]

Hi,

nvmet_execute_auth_send() and nvmet_exeucte_auth_receive() share a lot
of common functionality such as :-

1. Checking secp/spsp values and its error handling.
2. Initializing transfer buffer len and its error handling.
2. Allocating transfer buffer and its error handling.

This code is repeated in both the functions.

Add common helpers with very small restructring code to remove
duplication of above functionality in the nvmet_exeucte_auth_receive()
and nvmet_execute_auth_send(), it also makes code easy to read as both
the functions are doing substantial work.

Please note that this series is generated on the top of this :-

[PATCH 1/2] nvmet-auth: remove unnecessary break after goto
http://lists.infradead.org/pipermail/linux-nvme/2023-May/039950.html

Below are the blktets nvme results on nvme-loop and nvme-tcp transport.

-ck

Chaitanya Kulkarni (3):
  nvmet-auth: use common helper to check secp/spsp
  nvmet_auth: use common helper for buffer alloc
  nvmet-auth: use helper for auth send/recv cmd prep

 drivers/nvme/target/fabrics-cmd-auth.c | 106 +++++++++++--------------
 1 file changed, 45 insertions(+), 61 deletions(-)

blktests (master) # sh  test-nvme-memleack.sh 
################nvme_trtype=loop############
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime    ...  43.965s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.194s  ...  10.171s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.321s  ...  1.732s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.421s  ...  2.118s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.134s  ...  0.131s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.092s  ...  0.083s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.351s  ...  1.712s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.306s  ...  1.742s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  88.563s  ...  84.996s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  88.483s  ...  70.556s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  93.537s  ...  77.927s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  89.064s  ...  71.515s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  5.629s  ...  6.141s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.993s  ...  4.797s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime    ...  20.513s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime    ...  20.563s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.315s  ...  1.741s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.324s  ...  1.718s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.293s  ...  1.721s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.307s  ...  1.668s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.376s  ...  2.147s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.318s  ...  1.718s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.284s  ...  1.699s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.285s  ...  1.695s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.274s  ...  1.659s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.290s  ...  1.688s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.289s  ...  1.716s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.616s  ...  2.001s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.298s  ...  0.420s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  1.484s  ...  5.448s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.046s  ...  0.042s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.416s  ...  8.235s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  1.164s  ...  1.638s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  7.461s  ...  10.101s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  1.342s  ...  4.516s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.537s  ...  2.351s
nvme/045 (Test re-authentication)                            [passed]
    runtime  6.356s  ...  4.993s
nvme/047 (test different queue types for fabric transports)  [not run]
    runtime  2.281s  ...  
    nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
    runtime  5.627s  ...  
    nvme_trtype=loop is not supported in this test
################nvme_trtype=tcp############
nvme/002 (create many subsystems and test discovery)         [not run]
    runtime  43.965s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.171s  ...  10.189s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.732s  ...  1.331s
nvme/005 (reset local loopback target)                       [passed]
    runtime  2.118s  ...  1.432s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.131s  ...  0.134s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.083s  ...  0.092s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.712s  ...  1.345s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.742s  ...  1.315s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  84.996s  ...  78.021s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  70.556s  ...  94.072s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  77.927s  ...  70.347s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  71.515s  ...  92.875s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  6.141s  ...  5.665s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  4.797s  ...  3.903s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [not run]
    runtime  20.513s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/017 (create/delete many file-ns and test discovery)     [not run]
    runtime  20.563s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.741s  ...  1.307s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.718s  ...  1.350s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.721s  ...  1.315s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.668s  ...  1.316s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  2.147s  ...  1.386s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.718s  ...  1.336s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.699s  ...  1.304s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.695s  ...  1.320s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.659s  ...  1.297s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.688s  ...  1.307s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.716s  ...  1.322s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  2.001s  ...  1.671s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.420s  ...  0.341s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  5.448s  ...  1.622s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.042s  ...  0.049s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  8.235s  ...  7.458s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  1.638s  ...  1.242s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  10.101s  ...  7.807s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  4.516s  ...  1.496s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  2.351s  ...  1.552s
nvme/045 (Test re-authentication)                            [passed]
    runtime  4.993s  ...  5.859s
nvme/047 (test different queue types for fabric transports)  [passed]
    runtime    ...  2.365s
nvme/048 (Test queue count changes on reconnect)             [passed]
    runtime    ...  5.649s
blktests (master) # 


-- 
2.40.0

[PATCH 1/3] nvmet-auth: use common helper to check secp/spsp

[Chaitanya Kulkarni]

Add a common helper to factor out secp/spsp values check in
nvmet_execute_auth_send() and nvmet_execute_auth_receive().

No functional change in this patch.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/fabrics-cmd-auth.c | 60 +++++++++++---------------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 6c9a1ce6068d..6ad322b3d0a9 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -12,6 +12,23 @@
 #include <crypto/kpp.h>
 #include "nvmet.h"
 
+static u16 nvmet_auth_check_secp_spsp(struct nvmet_req *req)
+{
+	if (req->cmd->auth_send.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
+		req->error_loc = offsetof(struct nvmf_auth_send_command, secp);
+		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
+	if (req->cmd->auth_send.spsp0 != 0x01) {
+		req->error_loc = offsetof(struct nvmf_auth_send_command, spsp0);
+		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
+	if (req->cmd->auth_send.spsp1 != 0x01) {
+		req->error_loc = offsetof(struct nvmf_auth_send_command, spsp1);
+		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
+	return NVME_SC_SUCCESS;
+}
+
 static void nvmet_auth_expired_work(struct work_struct *work)
 {
 	struct nvmet_sq *sq = container_of(to_delayed_work(work),
@@ -185,26 +202,12 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 	struct nvmf_auth_dhchap_success2_data *data;
 	void *d;
 	u32 tl;
-	u16 status = 0;
+	u16 status;
 
-	if (req->cmd->auth_send.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_send_command, secp);
-		goto done;
-	}
-	if (req->cmd->auth_send.spsp0 != 0x01) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_send_command, spsp0);
-		goto done;
-	}
-	if (req->cmd->auth_send.spsp1 != 0x01) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_send_command, spsp1);
+	status = nvmet_auth_check_secp_spsp(req);
+	if (status)
 		goto done;
-	}
+
 	tl = le32_to_cpu(req->cmd->auth_send.tl);
 	if (!tl) {
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
@@ -432,26 +435,11 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
 	struct nvmet_ctrl *ctrl = req->sq->ctrl;
 	void *d;
 	u32 al;
-	u16 status = 0;
+	u16 status;
 
-	if (req->cmd->auth_receive.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_receive_command, secp);
-		goto done;
-	}
-	if (req->cmd->auth_receive.spsp0 != 0x01) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_receive_command, spsp0);
-		goto done;
-	}
-	if (req->cmd->auth_receive.spsp1 != 0x01) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_receive_command, spsp1);
+	status = nvmet_auth_check_secp_spsp(req);
+	if (status)
 		goto done;
-	}
 	al = le32_to_cpu(req->cmd->auth_receive.al);
 	if (!al) {
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-- 
2.40.0

[PATCH 2/3] nvmet_auth: use common helper for buffer alloc

[Chaitanya Kulkarni]

Add a common helper to factor out buffer allocation in
nvmet_execute_auth_send() and nvmet_execute_auth_receive().

Only functional change in this patch is transfer buffer allocation is
moved before nvmet_check_transfer_len() and it is freed if when
nvmet_check_transfer_len() fails. But similar allocation and free is
used in error unwind path in nvme code and it is not in fast path, so
it shuold be fine.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/fabrics-cmd-auth.c | 43 ++++++++++++--------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 6ad322b3d0a9..ad8e914ae56f 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -29,6 +29,18 @@ static u16 nvmet_auth_check_secp_spsp(struct nvmet_req *req)
 	return NVME_SC_SUCCESS;
 }
 
+static u16 nvmet_auth_alloc_transfer_buffer(struct nvmet_req *req, void **buf,
+					    u32 *len)
+{
+	*len = le32_to_cpu(req->cmd->auth_receive.al);
+	if (!*len) {
+		req->error_loc = offsetof(struct nvmf_auth_receive_command, al);
+		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
+	*buf = kmalloc(*len, GFP_KERNEL);
+	return *buf ? NVME_SC_SUCCESS : NVME_SC_INTERNAL;
+}
+
 static void nvmet_auth_expired_work(struct work_struct *work)
 {
 	struct nvmet_sq *sq = container_of(to_delayed_work(work),
@@ -207,25 +219,16 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 	status = nvmet_auth_check_secp_spsp(req);
 	if (status)
 		goto done;
-
-	tl = le32_to_cpu(req->cmd->auth_send.tl);
-	if (!tl) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_send_command, tl);
+	status = nvmet_auth_alloc_transfer_buffer(req, &d, &tl);
+	if (status)
 		goto done;
-	}
+
 	if (!nvmet_check_transfer_len(req, tl)) {
 		pr_debug("%s: transfer length mismatch (%u)\n", __func__, tl);
+		kfree(d);
 		return;
 	}
 
-	d = kmalloc(tl, GFP_KERNEL);
-	if (!d) {
-		status = NVME_SC_INTERNAL;
-		goto done;
-	}
-
 	status = nvmet_copy_from_sgl(req, 0, d, tl);
 	if (status)
 		goto done_kfree;
@@ -440,23 +443,15 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
 	status = nvmet_auth_check_secp_spsp(req);
 	if (status)
 		goto done;
-	al = le32_to_cpu(req->cmd->auth_receive.al);
-	if (!al) {
-		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-		req->error_loc =
-			offsetof(struct nvmf_auth_receive_command, al);
+	status = nvmet_auth_alloc_transfer_buffer(req, &d, &al);
+	if (status)
 		goto done;
-	}
 	if (!nvmet_check_transfer_len(req, al)) {
 		pr_debug("%s: transfer length mismatch (%u)\n", __func__, al);
+		kfree(d);
 		return;
 	}
 
-	d = kmalloc(al, GFP_KERNEL);
-	if (!d) {
-		status = NVME_SC_INTERNAL;
-		goto done;
-	}
 	pr_debug("%s: ctrl %d qid %d step %x\n", __func__,
 		 ctrl->cntlid, req->sq->qid, req->sq->dhchap_step);
 	switch (req->sq->dhchap_step) {
-- 
2.40.0

[PATCH 3/3] nvmet-auth: use helper for auth send/recv cmd prep

[Chaitanya Kulkarni]

Add a common helper to factor out secp/spsp values check and transfer
buffer allocation in nvmet_execute_auth_send() and
nvmet_execute_auth_receive().

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/fabrics-cmd-auth.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index ad8e914ae56f..8200265734e1 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -41,6 +41,13 @@ static u16 nvmet_auth_alloc_transfer_buffer(struct nvmet_req *req, void **buf,
 	return *buf ? NVME_SC_SUCCESS : NVME_SC_INTERNAL;
 }
 
+static u16 nvmet_auth_common_prep(struct nvmet_req *req, void **buf, u32 *len)
+{
+	u16 sts = nvmet_auth_check_secp_spsp(req);
+
+	return sts ? sts : nvmet_auth_alloc_transfer_buffer(req, buf, len);
+}
+
 static void nvmet_auth_expired_work(struct work_struct *work)
 {
 	struct nvmet_sq *sq = container_of(to_delayed_work(work),
@@ -216,10 +223,7 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 	u32 tl;
 	u16 status;
 
-	status = nvmet_auth_check_secp_spsp(req);
-	if (status)
-		goto done;
-	status = nvmet_auth_alloc_transfer_buffer(req, &d, &tl);
+	status = nvmet_auth_common_prep(req, &d, &tl);
 	if (status)
 		goto done;
 
@@ -440,10 +444,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
 	u32 al;
 	u16 status;
 
-	status = nvmet_auth_check_secp_spsp(req);
-	if (status)
-		goto done;
-	status = nvmet_auth_alloc_transfer_buffer(req, &d, &al);
+	status = nvmet_auth_common_prep(req, &d, &al);
 	if (status)
 		goto done;
 	if (!nvmet_check_transfer_len(req, al)) {
-- 
2.40.0

Re: [PATCH 1/3] nvmet-auth: use common helper to check secp/spsp

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] nvmet_auth: use common helper for buffer alloc

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/3] nvmet-auth: use helper for auth send/recv cmd prep

[Christoph Hellwig]

> +	u16 sts = nvmet_auth_check_secp_spsp(req);
> +
> +	return sts ? sts : nvmet_auth_alloc_transfer_buffer(req, buf, len);

I'd much prefer just spelling things out here as usual:

	if (sts)
		return sts;
	return nvmet_auth_alloc_transfer_buffer(req, buf, len);

Re: [PATCH 3/3] nvmet-auth: use helper for auth send/recv cmd prep

[Chaitanya Kulkarni]

On 5/22/2023 11:56 PM, Christoph Hellwig wrote:
>> +	u16 sts = nvmet_auth_check_secp_spsp(req);
>> +
>> +	return sts ? sts : nvmet_auth_alloc_transfer_buffer(req, buf, len);
> 
> I'd much prefer just spelling things out here as usual:
> 
> 	if (sts)
> 		return sts;
> 	return nvmet_auth_alloc_transfer_buffer(req, buf, len);
> 

okay, will add above changes in V2.

-ck