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

[PATCH V2 0/4] 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 of 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 :-

commit 01cff945c026f1e245ba6401f7df2336ddbae11d (origin/nvme-6.5)
Author: Chaitanya Kulkarni <kch@nvidia.com>
Date:   Fri May 19 02:40:52 2023 -0700

    nvme-fcloop: no need to return from void function
    
    Remove return at the end of void function.

-ck

- Changes in V2:-

* add reviewe tags
* remove use of conditional operators in 3rd patch
* add a patch to fix the nvmet_sq->dhchap_step type fix from int -> u16

Chaitanya Kulkarni (4):
  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
  nvmet-auth: use correct type for status variable

 drivers/nvme/target/fabrics-cmd-auth.c | 108 +++++++++++--------------
 drivers/nvme/target/nvmet.h            |   2 +-
 2 files changed, 48 insertions(+), 62 deletions(-)

nvme (nvme-6.5) # gitlog -4
6692f4da1048 (HEAD -> nvme-6.5) nvmet-auth: use correct type for status variable
16cb863ed3b1 nvmet-auth: use helper for auth send/recv cmd prep
d980c9183a99 nvmet_auth: use common helper for buffer alloc
104b23f8d911 nvmet-auth: use common helper to check secp/spsp
nvme (nvme-6.5) # git reset HEAD~4 --hard
HEAD is now at 01cff945c026 nvme-fcloop: no need to return from void function
nvme (nvme-6.5) # git am p/nvmet-auth-code-cleanup/*patch; git am --skip
Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To record the empty patch as an empty commit, run "git am --allow-empty".
To restore the original branch and stop patching, run "git am --abort".
Applying: nvmet-auth: use common helper to check secp/spsp
Applying: nvmet_auth: use common helper for buffer alloc
Applying: nvmet-auth: use helper for auth send/recv cmd prep
Applying: nvmet-auth: use correct type for status variable
nvme (nvme-6.5) # ./compile_nvme.sh 
+ modprobe -r nvme-fabrics
+ modprobe -r nvme_loop
+ modprobe -r nvmet
+ modprobe -r nvme
+ sleep 1
+ modprobe -r nvme-core
+ lsmod
+ grep nvme
+ sleep 1
+ git diff
++ nproc
+ make -j 48 M=drivers/nvme/ modules
  CC [M]  drivers/nvme/target/core.o
  CC [M]  drivers/nvme/target/configfs.o
  CC [M]  drivers/nvme/target/admin-cmd.o
  CC [M]  drivers/nvme/target/fabrics-cmd.o
  CC [M]  drivers/nvme/target/discovery.o
  CC [M]  drivers/nvme/target/io-cmd-file.o
  CC [M]  drivers/nvme/target/io-cmd-bdev.o
  CC [M]  drivers/nvme/target/passthru.o
  CC [M]  drivers/nvme/target/zns.o
  CC [M]  drivers/nvme/target/fabrics-cmd-auth.o
  CC [M]  drivers/nvme/target/auth.o
  CC [M]  drivers/nvme/target/trace.o
  CC [M]  drivers/nvme/target/loop.o
  CC [M]  drivers/nvme/target/rdma.o
  CC [M]  drivers/nvme/target/fc.o
  CC [M]  drivers/nvme/target/fcloop.o
  CC [M]  drivers/nvme/target/tcp.o
  LD [M]  drivers/nvme/target/nvme-loop.o
  LD [M]  drivers/nvme/target/nvme-fcloop.o
  LD [M]  drivers/nvme/target/nvmet.o
  LD [M]  drivers/nvme/target/nvmet-fc.o
  LD [M]  drivers/nvme/target/nvmet-tcp.o
  LD [M]  drivers/nvme/target/nvmet-rdma.o
  MODPOST drivers/nvme/Module.symvers
  LD [M]  drivers/nvme/target/nvmet.ko
  LD [M]  drivers/nvme/target/nvme-loop.ko
  LD [M]  drivers/nvme/target/nvmet-rdma.ko
  LD [M]  drivers/nvme/target/nvmet-fc.ko
  LD [M]  drivers/nvme/target/nvme-fcloop.ko
  LD [M]  drivers/nvme/target/nvmet-tcp.ko
+ HOST=drivers/nvme/host
+ TARGET=drivers/nvme/target
++ uname -r
+ HOST_DEST=/lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/host/
++ uname -r
+ TARGET_DEST=/lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/target/
+ cp drivers/nvme/host/nvme-core.ko drivers/nvme/host/nvme-fabrics.ko drivers/nvme/host/nvme-fc.ko drivers/nvme/host/nvme.ko drivers/nvme/host/nvme-rdma.ko drivers/nvme/host/nvme-tcp.ko /lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/host//
+ cp drivers/nvme/target/nvme-fcloop.ko drivers/nvme/target/nvme-loop.ko drivers/nvme/target/nvmet-fc.ko drivers/nvme/target/nvmet.ko drivers/nvme/target/nvmet-rdma.ko drivers/nvme/target/nvmet-tcp.ko /lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/target//
+ ls -lrth /lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/host/ /lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/target//
/lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/host/:
total 7.8M
-rw-r--r--. 1 root root 3.8M Jun  4 23:21 nvme-core.ko
-rw-r--r--. 1 root root 493K Jun  4 23:21 nvme-fabrics.ko
-rw-r--r--. 1 root root 981K Jun  4 23:21 nvme-fc.ko
-rw-r--r--. 1 root root 785K Jun  4 23:21 nvme.ko
-rw-r--r--. 1 root root 929K Jun  4 23:21 nvme-rdma.ko
-rw-r--r--. 1 root root 906K Jun  4 23:21 nvme-tcp.ko

/lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/target//:
total 7.4M
-rw-r--r--. 1 root root 537K Jun  4 23:21 nvme-fcloop.ko
-rw-r--r--. 1 root root 476K Jun  4 23:21 nvme-loop.ko
-rw-r--r--. 1 root root 805K Jun  4 23:21 nvmet-fc.ko
-rw-r--r--. 1 root root 4.0M Jun  4 23:21 nvmet.ko
-rw-r--r--. 1 root root 899K Jun  4 23:21 nvmet-rdma.ko
-rw-r--r--. 1 root root 761K Jun  4 23:21 nvmet-tcp.ko
+ sync
+ sync
+ sync
+ modprobe nvme
+ echo 'Press enter to continue ...'
Press enter to continue ...
################nvme_trtype=loop############
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime  20.311s  ...  20.535s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.080s  ...  10.080s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.465s  ...  1.470s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.797s  ...  1.784s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.060s  ...  0.061s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.034s  ...  0.035s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.468s  ...  1.468s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.441s  ...  1.436s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  94.861s  ...  86.838s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  81.316s  ...  71.517s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime    ...  77.886s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  70.922s  ...  77.348s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.239s  ...  4.570s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.763s  ...  3.792s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime  12.382s  ...  12.757s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime  12.338s  ...  12.283s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.440s  ...  1.434s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.451s  ...  1.445s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.430s  ...  1.421s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.434s  ...  1.434s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.756s  ...  1.756s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.449s  ...  1.453s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.414s  ...  1.435s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.431s  ...  1.426s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.435s  ...  1.417s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.438s  ...  1.438s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.433s  ...  1.429s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.579s  ...  1.561s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.211s  ...  0.209s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  3.968s  ...  3.925s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.013s  ...  0.013s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.909s  ...  7.858s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.741s  ...  0.766s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  4.897s  ...  4.920s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  3.126s  ...  6.955s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.854s  ...  1.797s
nvme/045 (Test re-authentication)                            [passed]
    runtime  4.087s  ...  4.039s
nvme/047 (test different queue types for fabric transports)  [not run]
    nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
    nvme_trtype=loop is not supported in this test
################nvme_trtype=tcp############
nvme/002 (create many subsystems and test discovery)         [not run]
    runtime  20.535s  ...  
    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.080s  ...  10.096s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.470s  ...  1.150s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.784s  ...  1.193s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.061s  ...  0.060s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.035s  ...  0.035s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.468s  ...  1.153s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.436s  ...  1.133s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  86.838s  ...  94.475s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  71.517s  ...  65.550s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  77.886s  ...  88.897s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  77.348s  ...  62.977s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.570s  ...  4.069s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.792s  ...  3.540s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [not run]
    runtime  12.757s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/017 (create/delete many file-ns and test discovery)     [not run]
    runtime  12.283s  ...  
    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.434s  ...  1.134s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.445s  ...  1.158s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.421s  ...  1.116s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.434s  ...  1.136s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.756s  ...  1.171s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.453s  ...  1.142s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.435s  ...  1.138s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.426s  ...  1.123s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.417s  ...  1.117s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.438s  ...  1.147s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.429s  ...  1.121s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.561s  ...  1.259s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.209s  ...  0.121s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  3.925s  ...  0.815s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.013s  ...  0.017s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.858s  ...  7.171s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.766s  ...  0.452s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  4.920s  ...  2.822s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  6.955s  ...  0.691s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.797s  ...  1.216s
nvme/045 (Test re-authentication)                            [passed]
    runtime  4.039s  ...  3.810s
nvme/047 (test different queue types for fabric transports)  [passed]
    runtime    ...  1.832s
nvme/048 (Test queue count changes on reconnect)             [passed]
    runtime    ...  5.251s
blktests (master) # 


-- 
2.40.0

[PATCH V2 1/4] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 586458f765f1..42594e086de7 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 V2 2/4] 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>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 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 42594e086de7..778961e231a3 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 V2 3/4] 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 | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 778961e231a3..d331c22ed26e 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -41,6 +41,15 @@ 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 status = nvmet_auth_check_secp_spsp(req);
+
+	if (status)
+		return status;
+	return 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 +225,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 +446,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

[PATCH V2 4/4] nvmet-auth: use correct type for status variable

[Chaitanya Kulkarni]

In structructure nvmet_sq dhchap_step member is responsible to hold
following values :-

        NVME_AUTH_DHCHAP_FAILURE_FAILED                 = 0x01,
        NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE             = 0x02,
        NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH        = 0x03,
        NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE          = 0x04,
        NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE       = 0x05,
        NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD      = 0x06,
        NVME_AUTH_DHCHAP_FAILURE_INCORRECT_MESSAGE      = 0x07,

These values can never be negative, hence change int type of
dhchap_step to u16 in the nvmet_sq declaration.

Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/target/nvmet.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 6cf723bc664e..66d8673c3ebf 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -113,7 +113,7 @@ struct nvmet_sq {
 	struct delayed_work	auth_expired_work;
 	u16			dhchap_tid;
 	u16			dhchap_status;
-	int			dhchap_step;
+	u16			dhchap_step;
 	u8			*dhchap_c1;
 	u8			*dhchap_c2;
 	u32			dhchap_s1;
-- 
2.40.0

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

[Hannes Reinecke]

On 6/5/23 08:44, Chaitanya Kulkarni wrote:
> 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/target/fabrics-cmd-auth.c | 60 +++++++++++---------------
>   1 file changed, 24 insertions(+), 36 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Hannes Reinecke]

On 6/5/23 08:44, Chaitanya Kulkarni wrote:
> 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>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/nvme/target/fabrics-cmd-auth.c | 43 ++++++++++++--------------
>   1 file changed, 19 insertions(+), 24 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Hannes Reinecke]

On 6/5/23 08:44, Chaitanya Kulkarni wrote:
> 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 | 19 +++++++++++--------
>   1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
> index 778961e231a3..d331c22ed26e 100644
> --- a/drivers/nvme/target/fabrics-cmd-auth.c
> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
> @@ -41,6 +41,15 @@ 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 status = nvmet_auth_check_secp_spsp(req);
> +
> +	if (status)
> +		return status;
> +	return nvmet_auth_alloc_transfer_buffer(req, buf, len);
> +}
> +
A wrapper for a wrapper?
Please move nvmet_auch_check_secp_spsp() tion 
nvmet_auth_alloc_tranfer_buffer().

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH V2 4/4] nvmet-auth: use correct type for status variable

[Hannes Reinecke]

On 6/5/23 08:44, Chaitanya Kulkarni wrote:
> In structructure nvmet_sq dhchap_step member is responsible to hold
Spelling.
Also please drop the 'is responsible for' to just read:

'The dhchap_step member of structure nvmet_sq holds the following values:'

> following values :-
> 
>          NVME_AUTH_DHCHAP_FAILURE_FAILED                 = 0x01,
>          NVME_AUTH_DHCHAP_FAILURE_NOT_USABLE             = 0x02,
>          NVME_AUTH_DHCHAP_FAILURE_CONCAT_MISMATCH        = 0x03,
>          NVME_AUTH_DHCHAP_FAILURE_HASH_UNUSABLE          = 0x04,
>          NVME_AUTH_DHCHAP_FAILURE_DHGROUP_UNUSABLE       = 0x05,
>          NVME_AUTH_DHCHAP_FAILURE_INCORRECT_PAYLOAD      = 0x06,
>          NVME_AUTH_DHCHAP_FAILURE_INCORRECT_MESSAGE      = 0x07,
> 
> These values can never be negative, hence change int type of
> dhchap_step to u16 in the nvmet_sq declaration.
> 
> Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
> ---
>   drivers/nvme/target/nvmet.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
Otherwise the patch is okay.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Chaitanya Kulkarni]

On 6/5/23 00:46, Hannes Reinecke wrote:
> On 6/5/23 08:44, Chaitanya Kulkarni wrote:
>> 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 | 19 +++++++++++--------
>>   1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/nvme/target/fabrics-cmd-auth.c 
>> b/drivers/nvme/target/fabrics-cmd-auth.c
>> index 778961e231a3..d331c22ed26e 100644
>> --- a/drivers/nvme/target/fabrics-cmd-auth.c
>> +++ b/drivers/nvme/target/fabrics-cmd-auth.c
>> @@ -41,6 +41,15 @@ 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 status = nvmet_auth_check_secp_spsp(req);
>> +
>> +    if (status)
>> +        return status;
>> +    return nvmet_auth_alloc_transfer_buffer(req, buf, len);
>> +}
>> +
> A wrapper for a wrapper?
> Please move nvmet_auch_check_secp_spsp() tion 
> nvmet_auth_alloc_tranfer_buffer().
>
> Cheers,
>
> Hannes

checking secp and spsp and allocating memory are two different things,
let me restructure this series to remove this wrapper...

-ck

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

[Christoph Hellwig]

On Sun, Jun 04, 2023 at 11:44:33PM -0700, Chaitanya Kulkarni wrote:
> 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().

While the patch looks correct, I can't say I really like.  Having a
"common prep" function for just two unrelated things doesn't exactly
improve readability.  And it also doesn't remove all that much code.

Re: [PATCH V2 4/4] nvmet-auth: use correct type for status variable

[Christoph Hellwig]

Looks good:

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