Hi Mikulas,

thanks for your patch. See below.

On Mon, Jun 01, 2020 at 06:03:33PM +0200, Mikulas Patocka wrote:
> Using dm-crypt with QAT result in deadlocks.
> 
> If a crypto routine returns -EBUSY, it is expected that the crypto driver
> have already queued the request and the crypto API user should assume that
> this request was processed, but it should stop sending more requests. When
> an -EBUSY request is processed, the crypto driver calls the callback with
> the error code -EINPROGRESS - this means that the request is still being
> processed (i.e. the user should wait for another callback), but the user
> can start sending more requests now.
> 
> The QAT driver misunderstood this logic, it return -EBUSY when the queue
> was full and didn't queue the request - the request was lost and it
> resulted in a deadlock.
> 
> This patch fixes busy state handling - if the queue is at least 15/16
> full, we return -EBUSY to signal to the user that no more requests should
> be sent. We remember that we returned -EBUSY (the variable backed_off) and
> if we finish the request, we return -EINPROGRESS to indicate that the user
> can send more requests.
> 
> Signed-off-by: Mikulas Patocka <mpato...@redhat.com>
> Cc: sta...@vger.kernel.org
> 
> Index: linux-2.6/drivers/crypto/qat/qat_common/qat_algs.c
> ===================================================================
> --- linux-2.6.orig/drivers/crypto/qat/qat_common/qat_algs.c
> +++ linux-2.6/drivers/crypto/qat/qat_common/qat_algs.c
> @@ -826,6 +826,9 @@ static void qat_aead_alg_callback(struct
>       qat_alg_free_bufl(inst, qat_req);
>       if (unlikely(qat_res != ICP_QAT_FW_COMN_STATUS_FLAG_OK))
>               res = -EBADMSG;
> +
> +     if (qat_req->backed_off)
> +             areq->base.complete(&areq->base, -EINPROGRESS);
>       areq->base.complete(&areq->base, res);
>  }
>  
> @@ -847,6 +850,8 @@ static void qat_skcipher_alg_callback(st
>       dma_free_coherent(dev, AES_BLOCK_SIZE, qat_req->iv,
>                         qat_req->iv_paddr);
>  
> +     if (qat_req->backed_off)
> +             sreq->base.complete(&sreq->base, -EINPROGRESS);
>       sreq->base.complete(&sreq->base, res);
>  }
>  
> @@ -869,7 +874,7 @@ static int qat_alg_aead_dec(struct aead_
>       struct icp_qat_fw_la_auth_req_params *auth_param;
>       struct icp_qat_fw_la_bulk_req *msg;
>       int digst_size = crypto_aead_authsize(aead_tfm);
> -     int ret, ctr = 0;
> +     int ret, backed_off;
>  
>       ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req);
>       if (unlikely(ret))
> @@ -890,15 +895,16 @@ static int qat_alg_aead_dec(struct aead_
>       auth_param = (void *)((uint8_t *)cipher_param + sizeof(*cipher_param));
>       auth_param->auth_off = 0;
>       auth_param->auth_len = areq->assoclen + cipher_param->cipher_length;
> -     do {
> -             ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
> -     } while (ret == -EAGAIN && ctr++ < 10);
>  
> +     qat_req->backed_off = backed_off = 
> adf_should_back_off(ctx->inst->sym_tx);
> +again:
> +     ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
>       if (ret == -EAGAIN) {
> -             qat_alg_free_bufl(ctx->inst, qat_req);
> -             return -EBUSY;
> +             qat_req->backed_off = backed_off = 1;
> +             cpu_relax();
> +             goto again;
>       }
I am a bit concerned about this potential infinite loop.
If an error occurred on the device and the queue is full, we will be
stuck here forever.
Should we just retry a number of times and then fail?
Or, should we just move to the crypto-engine?

> -     return -EINPROGRESS;
> +     return backed_off ? -EBUSY : -EINPROGRESS;
>  }
>  
>  static int qat_alg_aead_enc(struct aead_request *areq)
> @@ -911,7 +917,7 @@ static int qat_alg_aead_enc(struct aead_
>       struct icp_qat_fw_la_auth_req_params *auth_param;
>       struct icp_qat_fw_la_bulk_req *msg;
>       uint8_t *iv = areq->iv;
> -     int ret, ctr = 0;
> +     int ret, backed_off;
>  
>       ret = qat_alg_sgl_to_bufl(ctx->inst, areq->src, areq->dst, qat_req);
>       if (unlikely(ret))
> @@ -935,15 +941,15 @@ static int qat_alg_aead_enc(struct aead_
>       auth_param->auth_off = 0;
>       auth_param->auth_len = areq->assoclen + areq->cryptlen;
>  
> -     do {
> -             ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
> -     } while (ret == -EAGAIN && ctr++ < 10);
> -
> +     qat_req->backed_off = backed_off = 
> adf_should_back_off(ctx->inst->sym_tx);
checkpatch: line over 80 characters - same in every place
adf_should_back_off is used.

> +again:
> +     ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
>       if (ret == -EAGAIN) {
> -             qat_alg_free_bufl(ctx->inst, qat_req);
> -             return -EBUSY;
> +             qat_req->backed_off = backed_off = 1;
> +             cpu_relax();
> +             goto again;
>       }
> -     return -EINPROGRESS;
> +     return backed_off ? -EBUSY : -EINPROGRESS;
>  }
>  
>  static int qat_alg_skcipher_rekey(struct qat_alg_skcipher_ctx *ctx,
> @@ -1051,7 +1057,7 @@ static int qat_alg_skcipher_encrypt(stru
>       struct icp_qat_fw_la_cipher_req_params *cipher_param;
>       struct icp_qat_fw_la_bulk_req *msg;
>       struct device *dev = &GET_DEV(ctx->inst->accel_dev);
> -     int ret, ctr = 0;
> +     int ret, backed_off;
>  
>       if (req->cryptlen == 0)
>               return 0;
> @@ -1081,17 +1087,16 @@ static int qat_alg_skcipher_encrypt(stru
>       cipher_param->cipher_offset = 0;
>       cipher_param->u.s.cipher_IV_ptr = qat_req->iv_paddr;
>       memcpy(qat_req->iv, req->iv, AES_BLOCK_SIZE);
> -     do {
> -             ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
> -     } while (ret == -EAGAIN && ctr++ < 10);
>  
> +     qat_req->backed_off = backed_off = 
> adf_should_back_off(ctx->inst->sym_tx);
> +again:
> +     ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
>       if (ret == -EAGAIN) {
> -             qat_alg_free_bufl(ctx->inst, qat_req);
> -             dma_free_coherent(dev, AES_BLOCK_SIZE, qat_req->iv,
> -                               qat_req->iv_paddr);
> -             return -EBUSY;
> +             qat_req->backed_off = backed_off = 1;
> +             cpu_relax();
> +             goto again;
>       }
> -     return -EINPROGRESS;
> +     return backed_off ? -EBUSY : -EINPROGRESS;
>  }
>  
>  static int qat_alg_skcipher_blk_encrypt(struct skcipher_request *req)
> @@ -1111,7 +1116,7 @@ static int qat_alg_skcipher_decrypt(stru
>       struct icp_qat_fw_la_cipher_req_params *cipher_param;
>       struct icp_qat_fw_la_bulk_req *msg;
>       struct device *dev = &GET_DEV(ctx->inst->accel_dev);
> -     int ret, ctr = 0;
> +     int ret, backed_off;
>  
>       if (req->cryptlen == 0)
>               return 0;
> @@ -1141,17 +1146,16 @@ static int qat_alg_skcipher_decrypt(stru
>       cipher_param->cipher_offset = 0;
>       cipher_param->u.s.cipher_IV_ptr = qat_req->iv_paddr;
>       memcpy(qat_req->iv, req->iv, AES_BLOCK_SIZE);
> -     do {
> -             ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
> -     } while (ret == -EAGAIN && ctr++ < 10);
>  
> +     qat_req->backed_off = backed_off = 
> adf_should_back_off(ctx->inst->sym_tx);
> +again:
> +     ret = adf_send_message(ctx->inst->sym_tx, (uint32_t *)msg);
>       if (ret == -EAGAIN) {
> -             qat_alg_free_bufl(ctx->inst, qat_req);
> -             dma_free_coherent(dev, AES_BLOCK_SIZE, qat_req->iv,
> -                               qat_req->iv_paddr);
> -             return -EBUSY;
> +             qat_req->backed_off = backed_off = 1;
> +             cpu_relax();
> +             goto again;
>       }
> -     return -EINPROGRESS;
> +     return backed_off ? -EBUSY : -EINPROGRESS;
>  }
>  
>  static int qat_alg_skcipher_blk_decrypt(struct skcipher_request *req)
> Index: linux-2.6/drivers/crypto/qat/qat_common/adf_transport.c
> ===================================================================
> --- linux-2.6.orig/drivers/crypto/qat/qat_common/adf_transport.c
> +++ linux-2.6/drivers/crypto/qat/qat_common/adf_transport.c
> @@ -114,10 +114,19 @@ static void adf_disable_ring_irq(struct
>       WRITE_CSR_INT_COL_EN(bank->csr_addr, bank->bank_number, bank->irq_mask);
>  }
>  
> +bool adf_should_back_off(struct adf_etr_ring_data *ring)
> +{
> +     return atomic_read(ring->inflights) > 
> ADF_MAX_INFLIGHTS(ring->ring_size, ring->msg_size) * 15 / 16;
How did you came up with 15/16?
checkpatch: WARNING: line over 80 characters

> +}
> +
>  int adf_send_message(struct adf_etr_ring_data *ring, uint32_t *msg)
>  {
> -     if (atomic_add_return(1, ring->inflights) >
> -         ADF_MAX_INFLIGHTS(ring->ring_size, ring->msg_size)) {
> +     int limit = ADF_MAX_INFLIGHTS(ring->ring_size, ring->msg_size);
> +
> +     if (atomic_read(ring->inflights) >= limit)
> +             return -EAGAIN;
Can this be removed and leave only the condition below?
Am I missing something here?
> +
> +     if (atomic_add_return(1, ring->inflights) > limit) {
>               atomic_dec(ring->inflights);
>               return -EAGAIN;
>       }
> Index: linux-2.6/drivers/crypto/qat/qat_common/adf_transport.h
> ===================================================================
> --- linux-2.6.orig/drivers/crypto/qat/qat_common/adf_transport.h
> +++ linux-2.6/drivers/crypto/qat/qat_common/adf_transport.h
> @@ -58,6 +58,7 @@ int adf_create_ring(struct adf_accel_dev
>                   const char *ring_name, adf_callback_fn callback,
>                   int poll_mode, struct adf_etr_ring_data **ring_ptr);
>  
> +bool adf_should_back_off(struct adf_etr_ring_data *ring);
>  int adf_send_message(struct adf_etr_ring_data *ring, uint32_t *msg);
>  void adf_remove_ring(struct adf_etr_ring_data *ring);
>  #endif
> Index: linux-2.6/drivers/crypto/qat/qat_common/qat_crypto.h
> ===================================================================
> --- linux-2.6.orig/drivers/crypto/qat/qat_common/qat_crypto.h
> +++ linux-2.6/drivers/crypto/qat/qat_common/qat_crypto.h
> @@ -90,6 +90,7 @@ struct qat_crypto_request {
>                  struct qat_crypto_request *req);
>       void *iv;
>       dma_addr_t iv_paddr;
> +     int backed_off;
>  };
>  
>  #endif
> 
Regards,

-- 
Giovanni

Reply via email to