Re: [PATCH 2/2] nvme-auth: use xarray instead of linked list

[Christoph Hellwig <hch@lst.de>]

On Fri, Oct 28, 2022 at 03:50:27PM +0200, Hannes Reinecke wrote:
> The current design of holding the chap context is slightly awkward,
> as the context is allocated on demand, and we have to lock the list
> when looking up contexts as we wouldn't know if the context is
> allocated.
> 
> This patch moves the allocation out of the chap context before starting
> authentication and stores it into an xarray. With that we can do
> away with the lock and access the context directly via the queue number.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/nvme/host/auth.c | 116 ++++++++++++++++++++++-----------------
>  drivers/nvme/host/nvme.h |   3 +-
>  2 files changed, 66 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
> index b68fb2c764f6..7b974bd0fa64 100644
> --- a/drivers/nvme/host/auth.c
> +++ b/drivers/nvme/host/auth.c
> @@ -72,10 +72,12 @@ static int nvme_auth_submit(struct nvme_ctrl *ctrl, int qid,
>  				     0, flags, nvme_max_retries);
>  	if (ret > 0)
>  		dev_warn(ctrl->device,
> -			"qid %d auth_send failed with status %d\n", qid, ret);
> +			"qid %d auth_%s failed with status %d\n",
> +			 qid, auth_send ? "send" : "recv", ret);
>  	else if (ret < 0)
>  		dev_err(ctrl->device,
> -			"qid %d auth_send failed with error %d\n", qid, ret);
> +			"qid %d auth_%s failed with error %d\n",
> +			qid, auth_send ? "send" : "recv", ret);
>  	return ret;

This looks like an unrelated message fixup.

> @@ -870,29 +872,42 @@ int nvme_auth_negotiate(struct nvme_ctrl *ctrl, int qid)
>  		return -ENOKEY;
>  	}
>  
> -	mutex_lock(&ctrl->dhchap_auth_mutex);

> +	if (qid == NVME_QID_ANY)
> +		qid = 0;

I can't see how NVME_QID_ANY would ever be passed here.

> +	chap = xa_load(&ctrl->dhchap_auth_xa, qid);
>  	if (!chap) {
> +		int ret;
> +
> +		chap = kzalloc(sizeof(*chap), GFP_KERNEL);
> +		if (!chap) {
> +			dev_warn(ctrl->device,
> +				 "qid %d: error allocation authentication", qid);
> +			return -ENOMEM;
> +		}
> +		chap->qid = qid;
> +		chap->ctrl = ctrl;
>  
> +		INIT_WORK(&chap->auth_work, __nvme_auth_work);
> +		ret = xa_insert(&ctrl->dhchap_auth_xa, qid, chap, GFP_KERNEL);

GFP_NOFS?

Also xa_insert can fail with -EBUSY here if someone concurrently inserted
an entry now that there is no locking.

> +	} else {
> +		if (chap->qid != qid) {
> +			dev_warn(ctrl->device,
> +				 "qid %d: authentication qid mismatch (%d)!",
> +				 chap->qid, qid);
> +			chap = xa_erase(&ctrl->dhchap_auth_xa, qid);
> +			__nvme_auth_free(chap);
> +			return -ENOENT;
> +		}

How can the qid not match given that the lookup is by qid?

> +		flush_work(&chap->auth_work);
> +		__nvme_auth_reset(chap);
> +	}

What protects us against someone concurrently freeing the entry here?

> @@ -901,33 +916,35 @@ EXPORT_SYMBOL_GPL(nvme_auth_negotiate);
>  int nvme_auth_wait(struct nvme_ctrl *ctrl, int qid)
>  {
>  	struct nvme_dhchap_queue_context *chap;
>  
> +	if (qid == NVME_QID_ANY)
> +		qid = 0;

I can't see how NVME_QID_ANY gets passed here ever.


> +	chap = xa_load(&ctrl->dhchap_auth_xa, qid);
> +	if (!chap) {
> +		dev_warn(ctrl->device,
> +			 "qid %d: authentication not initialized!",
> +			 qid);
> +		return -ENOENT;
> +	} else if (chap->qid != qid) {

No need for an return after an else.  But I can't see how the qid
makes sense here.  But once again, what protects the entry from
being freed?

> @@ -947,7 +964,7 @@ static void nvme_dhchap_auth_work(struct work_struct *work)
>  	ret = nvme_auth_wait(ctrl, 0);
>  	if (ret) {
>  		dev_warn(ctrl->device,
> -			 "qid 0: authentication failed\n");
> +			 "qid 0: authentication failed with %d\n", ret);

This looks like an unrelated message cleanup.