[PATCH 1/6] nvme-auth: allocate authentication buffer only during transaction

[Sagi Grimberg]

On 11/2/22 09:52, Hannes Reinecke wrote:
> The authentication buffer is only used during the authentication
> transaction, so no need to keep it around.

Patch is fine, but why do we need the chap context dynamically
allocated? It is always the number of queues and never ever changes.

> 
> Signed-off-by: Hannes Reinecke <hare at suse.de>
> ---
>   drivers/nvme/host/auth.c | 49 +++++++++++++++++++---------------------
>   1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index 3b63aa155beb..b68fb2c764f6 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -667,8 +667,6 @@ static void __nvme_auth_reset(struct nvme_dhchap_queue_context *chap)
>   	kfree_sensitive(chap->sess_key);
>   	chap->sess_key = NULL;
>   	chap->sess_key_len = 0;
> -	chap->status = 0;
> -	chap->error = 0;
>   	chap->s1 = 0;
>   	chap->s2 = 0;
>   	chap->transaction = 0;
> @@ -687,7 +685,6 @@ static void __nvme_auth_free(struct nvme_dhchap_queue_context *chap)
>   	kfree_sensitive(chap->host_key);
>   	kfree_sensitive(chap->sess_key);
>   	kfree_sensitive(chap->host_response);
> -	kfree(chap->buf);
>   	kfree(chap);
>   }
>   
> @@ -700,6 +697,19 @@ static void __nvme_auth_work(struct work_struct *work)
>   	int ret = 0;
>   
>   	chap->transaction = ctrl->transaction++;
> +	chap->status = 0;
> +	chap->error = 0;
> +
> +	/*
> +	 * Allocate a large enough buffer for the entire negotiation:
> +	 * 4k should be enough to ffdhe8192.
> +	 */
> +	chap->buf_size = 4096;
> +	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
> +	if (!chap->buf) {
> +		chap->error = -ENOMEM;
> +		return;
> +	}
>   
>   	/* DH-HMAC-CHAP Step 1: send negotiate */
>   	dev_dbg(ctrl->device, "%s: qid %d send negotiate\n",
> @@ -707,13 +717,13 @@ static void __nvme_auth_work(struct work_struct *work)
>   	ret = nvme_auth_set_dhchap_negotiate_data(ctrl, chap);
>   	if (ret < 0) {
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   	tl = ret;
>   	ret = nvme_auth_submit(ctrl, chap->qid, chap->buf, tl, true);
>   	if (ret) {
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   
>   	/* DH-HMAC-CHAP Step 2: receive challenge */
> @@ -727,14 +737,14 @@ static void __nvme_auth_work(struct work_struct *work)
>   			 "qid %d failed to receive challenge, %s %d\n",
>   			 chap->qid, ret < 0 ? "error" : "nvme status", ret);
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   	ret = nvme_auth_receive_validate(ctrl, chap->qid, chap->buf, chap->transaction,
>   					 NVME_AUTH_DHCHAP_MESSAGE_CHALLENGE);
>   	if (ret) {
>   		chap->status = ret;
>   		chap->error = NVME_SC_AUTH_REQUIRED;
> -		return;
> +		goto out_free;
>   	}
>   
>   	ret = nvme_auth_process_dhchap_challenge(ctrl, chap);
> @@ -790,7 +800,7 @@ static void __nvme_auth_work(struct work_struct *work)
>   			 "qid %d failed to receive success1, %s %d\n",
>   			 chap->qid, ret < 0 ? "error" : "nvme status", ret);
>   		chap->error = ret;
> -		return;
> +		goto out_free;
>   	}
>   	ret = nvme_auth_receive_validate(ctrl, chap->qid,
>   					 chap->buf, chap->transaction,
> @@ -798,7 +808,7 @@ static void __nvme_auth_work(struct work_struct *work)
>   	if (ret) {
>   		chap->status = ret;
>   		chap->error = NVME_SC_AUTH_REQUIRED;
> -		return;
> +		goto out_free;
>   	}
>   
>   	if (ctrl->ctrl_key) {
> @@ -828,10 +838,7 @@ static void __nvme_auth_work(struct work_struct *work)
>   		if (ret)
>   			chap->error = ret;
>   	}
> -	if (!ret) {
> -		chap->error = 0;
> -		return;
> -	}
> +	goto out_free;
>   
>   fail2:
>   	dev_dbg(ctrl->device, "%s: qid %d send failure2, status %x\n",
> @@ -844,6 +851,9 @@ static void __nvme_auth_work(struct work_struct *work)
>   	 */
>   	if (ret && !chap->error)
>   		chap->error = ret;
> +out_free:
> +	kfree(chap->buf);
> +	chap->buf = NULL;
>   }
>   
>   int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
> @@ -863,7 +873,6 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
>   	mutex_lock(&ctrl->dhchap_auth_mutex);
>   	/* Check if the context is already queued */
>   	list_for_each_entry(chap, &ctrl->dhchap_auth_list, entry) {
> -		WARN_ON(!chap->buf);
>   		if (chap->qid == qid) {
>   			dev_dbg(ctrl->device, "qid %d: re-using context\n", qid);
>   			mutex_unlock(&ctrl->dhchap_auth_mutex);
> @@ -881,18 +890,6 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
>   	chap->qid = (qid == NVME_QID_ANY) ? 0 : qid;
>   	chap->ctrl = ctrl;
>   
> -	/*
> -	 * Allocate a large enough buffer for the entire negotiation:
> -	 * 4k should be enough to ffdhe8192.
> -	 */
> -	chap->buf_size = 4096;
> -	chap->buf = kzalloc(chap->buf_size, GFP_KERNEL);
> -	if (!chap->buf) {
> -		mutex_unlock(&ctrl->dhchap_auth_mutex);
> -		kfree(chap);
> -		return -ENOMEM;
> -	}
> -
>   	INIT_WORK(&chap->auth_work, __nvme_auth_work);
>   	list_add(&chap->entry, &ctrl->dhchap_auth_list);
>   	mutex_unlock(&ctrl->dhchap_auth_mutex);