Re: [PATCH 4/4] crypto: hisilicon/zip - adapt to use crypto engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] crypto: hisilicon/zip - req_id exhaustion deadlock in
  hisi_zip_do_comp()
- [Medium] crypto: hisilicon/zip: hisi_zip_do_acomp() clobbers -EBUSY on
  backlog
--

commit e6135edb946689a8be0edb23655def633c6b3f9a
Author: Chenghai Huang <huangchenghai2@huawei.com>

crypto: hisilicon/zip - adapt to use crypto engine

This commit migrates the HiSilicon ZIP acomp driver to the crypto_engine
framework so requests that hit a full hardware queue are queued and retried
instead of failing immediately.

> diff --git a/drivers/crypto/hisilicon/zip/zip_crypto.c b/drivers/crypto/hisilicon/zip/zip_crypto.c
> index 70adde049b535..84951bc74a96e 100644
> --- a/drivers/crypto/hisilicon/zip/zip_crypto.c
> +++ b/drivers/crypto/hisilicon/zip/zip_crypto.c

[ ... ]

> @@ -333,68 +345,111 @@ static void hisi_zip_acomp_cb(struct hisi_qp *qp, void *data)

[ ... ]

> +static int hisi_zip_do_comp(struct hisi_zip_req *req)
> +{
> +	struct hisi_qp *qp = req->qp_ctx->qp;
> +	struct device *dev = &qp->qm->pdev->dev;
> +	int ret;
> +
> +	if (!req->buf_ready) {
> +		ret = hisi_zip_create_req(req);
> +		if (unlikely(ret))
> +			return -ENOSPC;
> +
> +		ret = hisi_zip_map_req_buffers(req);
> +		if (unlikely(ret))
> +			goto remove_req;
> +	}
> +
> +	ret = hisi_zip_do_work(req);
> +	if (ret == -EINPROGRESS)
> +		return -EINPROGRESS;
> +
> +	if (ret == -EBUSY)
> +		return -ENOSPC;

[Severity: High]
Can hisi_zip_do_comp() deadlock qp_ctx->engine when a request with
req->buf_ready == false sits at the head of engine->queue while all
req_q->size bits in req_q->req_bitmap are held by req->buf_ready == true
requests queued behind it?

req_q->req_bitmap has sq_depth bits, while qm_get_avail_sqe() caps the
hardware SQ at sq_depth - 1 in-flight requests.  When hisi_qp_send() returns
-EBUSY, hisi_zip_do_comp() returns -ENOSPC with req->buf_ready == true and
keeps req->req_id allocated while the request is parked in engine->queue.
Because hisi_zip_do_acomp() runs hisi_zip_do_comp() before
crypto_transfer_acomp_request_to_engine(), and qm_poll_req_cb() runs
hisi_zip_acomp_cb() (freeing req_id) before atomic_dec(&qp->qp_status.used),
new submissions can claim freed req_bitmap bits and queue behind an unmapped
request:

CPU0                              CPU1 (QM completion / new submitters)
hisi_zip_do_acomp(req_b)
  /* sq_depth - 1 in flight,
   * 1 parked with req_id held:
   * req_bitmap is full
   */
  hisi_zip_do_comp(req_b)
    hisi_zip_create_req(req_b)
      -> -EBUSY (-ENOSPC,
         buf_ready == false)
  crypto_transfer_acomp_request_to_engine(req_b)
    /* req_b at head of engine->queue */
                                  qm_poll_req_cb()
                                    hisi_zip_acomp_cb()
                                      hisi_zip_remove_req()
                                    /* before atomic_dec(&qp_status.used),
                                     * new hisi_zip_do_acomp() calls
                                     * allocate the freed req_ids, get
                                     * -EBUSY from hisi_qp_send(), and
                                     * queue behind req_b with
                                     * buf_ready == true
                                     */
kworker:
crypto_pump_requests()
  hisi_zip_do_one_request(req_b)
    hisi_zip_create_req(req_b)
      -> -ENOSPC
  crypto_enqueue_request_head(req_b)
  kthread_queue_work(pump_requests)  <-- spins forever

Once all sq_depth bits in req_bitmap are held by requests queued behind
req_b and the hardware SQ drains, req_b can never allocate a req_id and the
requests behind it can never be dequeued to release theirs, wedging the
queue and spinning engine->kworker at 100% CPU.

[ ... ]

> +static int hisi_zip_do_acomp(struct acomp_req *acomp_req, u8 alg_type)
> +{
> +	struct hisi_zip_ctx *ctx = crypto_tfm_ctx(acomp_req->base.tfm);
> +	struct hisi_zip_qp_ctx *qp_ctx = &ctx->qp_ctx[alg_type];
> +	struct hisi_zip_req *req = acomp_request_ctx(acomp_req);
> +	int ret;
> +
> +	if (unlikely(!acomp_req->src || !acomp_req->slen ||
> +		     !acomp_req->dst || !acomp_req->dlen))
> +		return -EINVAL;
> +
> +	req->req = acomp_req;
> +	req->alg_type = alg_type;
> +	req->qp_ctx = qp_ctx;
> +	req->buf_ready = false;
> +
> +	if (ctx->fallback)
> +		return hisi_zip_fallback_do_work(acomp_req,
> +						 req->alg_type == HZIP_ALG_TYPE_DECOMP);
> +
> +	ret = hisi_zip_do_comp(req);
> +	if (ret != -ENOSPC)
> +		return ret;
> +
> +	ret = crypto_transfer_acomp_request_to_engine(qp_ctx->engine, acomp_req);
> +	if (ret != -EINPROGRESS && ret != -EBUSY) {
> +		hisi_zip_release_req_resource(req);
> +		return ret;
> +	}
> +
> +	return -EINPROGRESS;
> +}

[Severity: Medium]
Should hisi_zip_do_acomp() return ret instead of -EINPROGRESS at the end of
the function?

When engine->queue is full and CRYPTO_TFM_REQ_MAY_BACKLOG is set,
crypto_transfer_acomp_request_to_engine() records the request on
engine->queue.backlog and returns -EBUSY.  Returning -EINPROGRESS hides the
backlog signal from callers and still triggers the extra
crypto_request_complete(backlog, -EINPROGRESS) notification from
crypto_pump_requests() that callers only expect after receiving -EBUSY.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109073556.2517351-1-huangchenghai2@huawei.com?part=4