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

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

[Chaitanya Kulkarni]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 15998 bytes --]

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 V3:-

* remove wrapper on the top of wrapper (Hannes)
* fix commit message in following patch 
  "nvmet-auth: use correct type for status variable" (Hannes)

- 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 (3):
  nvmet-auth: use common helper to check secp/spsp
  nvmet_auth: use common helper for buffer alloc
  nvmet-auth: use correct type for status variable

 drivers/nvme/target/fabrics-cmd-auth.c | 89 ++++++++++++--------------
 drivers/nvme/target/nvmet.h            |  2 +-
 2 files changed, 41 insertions(+), 50 deletions(-)

nvme (nvme-6.5) # git log -3
commit a30ce4b8bcfa0fa0485cc2afb0252400e7623052 (HEAD -> nvme-6.5)
Author: Chaitanya Kulkarni <kch@nvidia.com>
Date:   Sat Jun 3 22:59:12 2023 -0700

    nvmet-auth: use correct type for status variable
    
    The dhchap_step member of structure nvmet_sq holds the 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 struct.
    
    Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>

commit a5e26a6060a64a3104875bcd7074b15049460f78
Author: Chaitanya Kulkarni <kch@nvidia.com>
Date:   Mon Jun 5 01:46:30 2023 -0700

    nvmet_auth: use common helper for buffer alloc
    
    Add a common helper to factor out buffer allocation in
    nvmet_execute_auth_send() and nvmet_execute_auth_receive() and call it
    from nvmet_auth_common_prep() once we done with the secp/spsp0/spsp1
    check.
    
    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>

commit 974ed1267f19f27ee9fc6c2d194b46cb39fb13a0
Author: Chaitanya Kulkarni <kch@nvidia.com>
Date:   Sat May 20 22:36:25 2023 -0700

    nvmet-auth: use common helper to check secp/spsp
    
    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>
nvme (nvme-6.5) # ./compile_nvme.sh 
+ umount /mnt/nvme0n1
+ clear_dmesg
./compile_nvme.sh: line 3: clear_dmesg: command not found
umount: /mnt/nvme0n1: no mount point specified.
+ ./delete.sh
+ NQN=testnqn
+ nvme disconnect -n testnqn
NQN:testnqn disconnected 0 controller(s)

real	0m0.003s
user	0m0.002s
sys	0m0.001s
+ rm -fr '/sys/kernel/config/nvmet/ports/1/subsystems/*'
+ rmdir /sys/kernel/config/nvmet/ports/1
rmdir: failed to remove '/sys/kernel/config/nvmet/ports/1': No such file or directory
+ for subsys in /sys/kernel/config/nvmet/subsystems/*
+ for ns in ${subsys}/namespaces/*
+ echo 0
./delete.sh: line 14: /sys/kernel/config/nvmet/subsystems/*/namespaces/*/enable: No such file or directory
+ rmdir '/sys/kernel/config/nvmet/subsystems/*/namespaces/*'
rmdir: failed to remove '/sys/kernel/config/nvmet/subsystems/*/namespaces/*': No such file or directory
+ rmdir '/sys/kernel/config/nvmet/subsystems/*'
rmdir: failed to remove '/sys/kernel/config/nvmet/subsystems/*': No such file or directory
+ rmdir 'config/nullb/nullb*'
rmdir: failed to remove 'config/nullb/nullb*': No such file or directory
+ umount /mnt/nvme0n1
umount: /mnt/nvme0n1: no mount point specified.
+ umount /mnt/backend
umount: /mnt/backend: not mounted.
+ modprobe -r nvme_loop
+ modprobe -r nvme_fabrics
+ modprobe -r nvmet
+ modprobe -r nvme
+ modprobe -r null_blk
+ tree /sys/kernel/config
/sys/kernel/config
└── pci_ep
    ├── controllers
    └── functions

3 directories, 0 files
+ sleep 1
+ modprobe -r nvme-core
+ lsmod
+ grep nvme
+ sleep 1
+ git diff
++ nproc
+ make -j 48 M=drivers/nvme/ modules
+ 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  5 01:51 nvme-core.ko
-rw-r--r--. 1 root root 493K Jun  5 01:51 nvme-fabrics.ko
-rw-r--r--. 1 root root 981K Jun  5 01:51 nvme-fc.ko
-rw-r--r--. 1 root root 785K Jun  5 01:51 nvme.ko
-rw-r--r--. 1 root root 929K Jun  5 01:51 nvme-rdma.ko
-rw-r--r--. 1 root root 906K Jun  5 01:51 nvme-tcp.ko

/lib/modules/6.4.0-rc2nvme+/kernel/drivers/nvme/target//:
total 7.4M
-rw-r--r--. 1 root root 537K Jun  5 01:51 nvme-fcloop.ko
-rw-r--r--. 1 root root 476K Jun  5 01:51 nvme-loop.ko
-rw-r--r--. 1 root root 805K Jun  5 01:51 nvmet-fc.ko
-rw-r--r--. 1 root root 4.0M Jun  5 01:51 nvmet.ko
-rw-r--r--. 1 root root 899K Jun  5 01:51 nvmet-rdma.ko
-rw-r--r--. 1 root root 761K Jun  5 01:51 nvmet-tcp.ko
+ sync
+ sync
+ sync
+ modprobe nvme
+ echo 'Press enter to continue ...'
Press enter to continue ...
nvme (nvme-6.5) # cdblktests 
(failed reverse-i-search)`./sh test': ^Csend-email.sh 
blktests (master) # sh test-nvme.sh 
blktests (master) # sh test-nvme.sh 
################nvme_trtype=loop############
nvme/002 (create many subsystems and test discovery)         [passed]
    runtime    ...  19.549s
nvme/003 (test if we're sending keep-alives to a discovery controller) [passed]
    runtime  10.096s  ...  10.088s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.150s  ...  1.439s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.193s  ...  1.787s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.060s  ...  0.063s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.035s  ...  0.036s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.153s  ...  1.454s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.133s  ...  1.458s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  94.475s  ...  97.494s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  65.550s  ...  79.488s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  88.897s  ...  83.403s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  62.977s  ...  72.704s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.069s  ...  4.257s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.540s  ...  3.751s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [passed]
    runtime    ...  12.843s
nvme/017 (create/delete many file-ns and test discovery)     [passed]
    runtime    ...  12.687s
nvme/018 (unit test NVMe-oF out of range access on a file backend) [passed]
    runtime  1.134s  ...  1.435s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.158s  ...  1.443s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.116s  ...  1.430s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.136s  ...  1.426s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.171s  ...  1.782s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.142s  ...  1.462s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.138s  ...  1.425s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.123s  ...  1.432s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.117s  ...  1.421s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.147s  ...  1.443s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.121s  ...  1.444s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.259s  ...  1.569s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.121s  ...  0.202s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  0.815s  ...  3.975s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.017s  ...  0.014s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.171s  ...  7.871s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.452s  ...  0.756s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  2.822s  ...  4.909s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  0.691s  ...  6.975s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.216s  ...  1.824s
nvme/045 (Test re-authentication)                            [passed]
    runtime  3.810s  ...  4.061s
nvme/047 (test different queue types for fabric transports)  [not run]
    runtime  1.832s  ...  
    nvme_trtype=loop is not supported in this test
nvme/048 (Test queue count changes on reconnect)             [not run]
    runtime  5.251s  ...  
    nvme_trtype=loop is not supported in this test
################nvme_trtype=tcp############
nvme/002 (create many subsystems and test discovery)         [not run]
    runtime  19.549s  ...  
    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.088s  ...  10.087s
nvme/004 (test nvme and nvmet UUID NS descriptors)           [passed]
    runtime  1.439s  ...  1.128s
nvme/005 (reset local loopback target)                       [passed]
    runtime  1.787s  ...  1.227s
nvme/006 (create an NVMeOF target with a block device-backed ns) [passed]
    runtime  0.063s  ...  0.056s
nvme/007 (create an NVMeOF target with a file-backed ns)     [passed]
    runtime  0.036s  ...  0.038s
nvme/008 (create an NVMeOF host with a block device-backed ns) [passed]
    runtime  1.454s  ...  1.141s
nvme/009 (create an NVMeOF host with a file-backed ns)       [passed]
    runtime  1.458s  ...  1.118s
nvme/010 (run data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  97.494s  ...  101.964s
nvme/011 (run data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  79.488s  ...  68.096s
nvme/012 (run mkfs and data verification fio job on NVMeOF block device-backed ns) [passed]
    runtime  83.403s  ...  88.510s
nvme/013 (run mkfs and data verification fio job on NVMeOF file-backed ns) [passed]
    runtime  72.704s  ...  71.458s
nvme/014 (flush a NVMeOF block device-backed ns)             [passed]
    runtime  4.257s  ...  3.951s
nvme/015 (unit test for NVMe flush for file backed ns)       [passed]
    runtime  3.751s  ...  3.439s
nvme/016 (create/delete many NVMeOF block device-backed ns and test discovery) [not run]
    runtime  12.843s  ...  
    nvme_trtype=tcp is not supported in this test
nvme/017 (create/delete many file-ns and test discovery)     [not run]
    runtime  12.687s  ...  
    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.435s  ...  1.127s
nvme/019 (test NVMe DSM Discard command on NVMeOF block-device ns) [passed]
    runtime  1.443s  ...  1.132s
nvme/020 (test NVMe DSM Discard command on NVMeOF file-backed ns) [passed]
    runtime  1.430s  ...  1.118s
nvme/021 (test NVMe list command on NVMeOF file-backed ns)   [passed]
    runtime  1.426s  ...  1.121s
nvme/022 (test NVMe reset command on NVMeOF file-backed ns)  [passed]
    runtime  1.782s  ...  1.170s
nvme/023 (test NVMe smart-log command on NVMeOF block-device ns) [passed]
    runtime  1.462s  ...  1.133s
nvme/024 (test NVMe smart-log command on NVMeOF file-backed ns) [passed]
    runtime  1.425s  ...  1.116s
nvme/025 (test NVMe effects-log command on NVMeOF file-backed ns) [passed]
    runtime  1.432s  ...  1.112s
nvme/026 (test NVMe ns-descs command on NVMeOF file-backed ns) [passed]
    runtime  1.421s  ...  1.114s
nvme/027 (test NVMe ns-rescan command on NVMeOF file-backed ns) [passed]
    runtime  1.443s  ...  1.121s
nvme/028 (test NVMe list-subsys command on NVMeOF file-backed ns) [passed]
    runtime  1.444s  ...  1.103s
nvme/029 (test userspace IO via nvme-cli read/write interface) [passed]
    runtime  1.569s  ...  1.253s
nvme/030 (ensure the discovery generation counter is updated appropriately) [passed]
    runtime  0.202s  ...  0.122s
nvme/031 (test deletion of NVMeOF controllers immediately after setup) [passed]
    runtime  3.975s  ...  0.809s
nvme/038 (test deletion of NVMeOF subsystem without enabling) [passed]
    runtime  0.014s  ...  0.015s
nvme/040 (test nvme fabrics controller reset/disconnect operation during I/O) [passed]
    runtime  7.871s  ...  7.169s
nvme/041 (Create authenticated connections)                  [passed]
    runtime  0.756s  ...  0.423s
nvme/042 (Test dhchap key types for authenticated connections) [passed]
    runtime  4.909s  ...  2.758s
nvme/043 (Test hash and DH group variations for authenticated connections) [passed]
    runtime  6.975s  ...  0.701s
nvme/044 (Test bi-directional authentication)                [passed]
    runtime  1.824s  ...  1.235s
nvme/045 (Test re-authentication)                            [passed]
    runtime  4.061s  ...  3.822s
nvme/047 (test different queue types for fabric transports)  [passed]
    runtime    ...  1.803s
nvme/048 (Test queue count changes on reconnect)             [passed]
    runtime    ...  6.233s
-- 
2.40.0

[PATCH V3 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 586458f765f1..847aa12d2915 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_common_prep(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_common_prep(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_common_prep(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 V3 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() and call it
from nvmet_auth_common_prep() once we done with the secp/spsp0/spsp1
check.

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 | 37 ++++++++++++++------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/drivers/nvme/target/fabrics-cmd-auth.c b/drivers/nvme/target/fabrics-cmd-auth.c
index 847aa12d2915..dbcae93bd25c 100644
--- a/drivers/nvme/target/fabrics-cmd-auth.c
+++ b/drivers/nvme/target/fabrics-cmd-auth.c
@@ -12,7 +12,20 @@
 #include <crypto/kpp.h>
 #include "nvmet.h"
 
-static u16 nvmet_auth_common_prep(struct nvmet_req *req)
+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 u16 nvmet_auth_common_prep(struct nvmet_req *req, void **buf,
+				  u32 *len)
 {
 	if (req->cmd->auth_send.secp != NVME_AUTH_DHCHAP_PROTOCOL_IDENTIFIER) {
 		req->error_loc = offsetof(struct nvmf_auth_send_command, secp);
@@ -26,7 +39,8 @@ static u16 nvmet_auth_common_prep(struct nvmet_req *req)
 		req->error_loc = offsetof(struct nvmf_auth_send_command, spsp1);
 		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 	}
-	return NVME_SC_SUCCESS;
+
+	return nvmet_auth_alloc_transfer_buffer(req, buf, len);
 }
 
 static void nvmet_auth_expired_work(struct work_struct *work)
@@ -204,28 +218,16 @@ void nvmet_execute_auth_send(struct nvmet_req *req)
 	u32 tl;
 	u16 status;
 
-	status = nvmet_auth_common_prep(req);
+	status = nvmet_auth_common_prep(req, &d, &tl);
 	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);
-		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;
@@ -437,7 +439,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
 	u32 al;
 	u16 status;
 
-	status = nvmet_auth_common_prep(req);
+	status = nvmet_auth_common_prep(req, &d, &al);
 	if (status)
 		goto done;
 	al = le32_to_cpu(req->cmd->auth_receive.al);
@@ -449,6 +451,7 @@ void nvmet_execute_auth_receive(struct nvmet_req *req)
 	}
 	if (!nvmet_check_transfer_len(req, al)) {
 		pr_debug("%s: transfer length mismatch (%u)\n", __func__, al);
+		kfree(d);
 		return;
 	}
 
-- 
2.40.0

[PATCH V3 3/3] nvmet-auth: use correct type for status variable

[Chaitanya Kulkarni]

The dhchap_step member of structure nvmet_sq holds the 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 struct.

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 V3 0/3] nvmet-auth: auth send / receive cleanup

[Chaitanya Kulkarni]

On 6/5/23 02:19, Chaitanya Kulkarni wrote:
> 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
>
>

Please disregard this, I'll send V4, apologies for noise..

-ck

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

[Sagi Grimberg]

OK,

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

[Sagi Grimberg]

On 6/5/23 12:19, Chaitanya Kulkarni wrote:
> Add a common helper to factor out buffer allocation in
> nvmet_execute_auth_send() and nvmet_execute_auth_receive() and call it
> from nvmet_auth_common_prep() once we done with the secp/spsp0/spsp1
> check.
> 
> 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.

This is asking for a future memory leak. It kinda makes sense to
allocate after we run sanity checks on the request...

Re: [PATCH V3 3/3] nvmet-auth: use correct type for status variable

[Sagi Grimberg]

On 6/5/23 12:19, Chaitanya Kulkarni wrote:
> The dhchap_step member of structure nvmet_sq holds the 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,

No, these are the failure reason codes. You probably meant
NVME_AUTH_DHCHAP_MESSAGE_xxx ?

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

[Sagi Grimberg]

> On 6/5/23 02:19, Chaitanya Kulkarni wrote:
>> 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
>>
>>
> 
> Please disregard this, I'll send V4, apologies for noise..

:/

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

[Max Gurtovoy]

On 05/06/2023 12:19, 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>
> ---
>   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..847aa12d2915 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_common_prep(struct nvmet_req *req)

The naming of the function is misleading a bit.
Function name should reflect better the logic of it.
This function is not preparing anything but just checking and validating 
fields.

maybe call it nvmet_auth_security_protocol_validate(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_common_prep(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_common_prep(req);
> +	if (status)
>   		goto done;
> -	}
>   	al = le32_to_cpu(req->cmd->auth_receive.al);
>   	if (!al) {
>   		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;

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

[Max Gurtovoy]

On 06/06/2023 1:02, Sagi Grimberg wrote:
> 
> 
> On 6/5/23 12:19, Chaitanya Kulkarni wrote:
>> Add a common helper to factor out buffer allocation in
>> nvmet_execute_auth_send() and nvmet_execute_auth_receive() and call it
>> from nvmet_auth_common_prep() once we done with the secp/spsp0/spsp1
>> check.
>>
>> 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.
> 
> This is asking for a future memory leak. It kinda makes sense to
> allocate after we run sanity checks on the request...
> 

Also I don't understand why do we need helper function for malloc ?
Allocating using nvmet_auth_alloc_transfer_buffer() and freeing using 
free() is just make the driver maintenance harder.

Another problem in this patch is that the len in the recv buffer is 
le32_to_cpu(req->cmd->auth_receive.al) and in the send is 
le32_to_cpu(req->cmd->auth_send.tl) so it's logically wrong to unify 
them. Same for error_loc.

maybe just don't include this patch 2/3 in V4 ?