linux-kernel - Re: [PATCH v2 10/16] crypto: qce - Add support for lock aquire,lock release api.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <n625hyjcbiidnlskzlubrmrflguwyurq5rp4l2hsnqf2g2wzik@ftz4wvvifft5>
Date: Fri, 16 Aug 2024 11:38:22 -0500
From: Bjorn Andersson <andersson@...nel.org>
To: Md Sadre Alam <quic_mdalam@...cinc.com>
Cc: vkoul@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, konradybcio@...nel.org, thara.gopinath@...il.com, 
	herbert@...dor.apana.org.au, davem@...emloft.net, gustavoars@...nel.org, 
	u.kleine-koenig@...gutronix.de, kees@...nel.org, agross@...nel.org, 
	linux-arm-msm@...r.kernel.org, dmaengine@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org, quic_srichara@...cinc.com, 
	quic_varada@...cinc.com, quic_utiwari@...cinc.com
Subject: Re: [PATCH v2 10/16] crypto: qce - Add support for lock aquire,lock
 release api.

On Thu, Aug 15, 2024 at 02:27:19PM GMT, Md Sadre Alam wrote:
> Add support for lock acquire and lock release api.
> When multiple EE's(Execution Environment) want to access
> CE5 then there will be race condition b/w multiple EE's.
> 
> Since each EE's having their dedicated BAM pipe, BAM allows
> Locking and Unlocking on BAM pipe. So if one EE's requesting
> for CE5 access then that EE's first has to LOCK the BAM pipe
> while setting LOCK bit on command descriptor and then access
> it. After finishing the request EE's has to UNLOCK the BAM pipe
> so in this way we race condition will not happen.

Does the lock/unlock need to happen on a dummy access before and after
the actual sequence? Is it not sufficient to lock/unlock on the first
and last operation?

Please squash this with the previous commit, if kept as explicit
operations, please squash it with the previous patch that introduces the
flags.

> 
> Added these two API qce_bam_acquire_lock() and qce_bam_release_lock()
> for the same.
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@...cinc.com>
> ---
> 
> Change in [v2]
> 
> * No chnage
> 
> Change in [v1]
> 
> * Added initial support for lock_acquire and lock_release
>   api.
> 
>  drivers/crypto/qce/common.c | 36 ++++++++++++++++++++++++++++++++++++
>  drivers/crypto/qce/core.h   |  2 ++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/crypto/qce/common.c b/drivers/crypto/qce/common.c
> index ff96f6ba1fc5..a8eaffe41101 100644
> --- a/drivers/crypto/qce/common.c
> +++ b/drivers/crypto/qce/common.c
> @@ -617,3 +617,39 @@ void qce_get_version(struct qce_device *qce, u32 *major, u32 *minor, u32 *step)
>  	*minor = (val & CORE_MINOR_REV_MASK) >> CORE_MINOR_REV_SHIFT;
>  	*step = (val & CORE_STEP_REV_MASK) >> CORE_STEP_REV_SHIFT;
>  }
> +
> +int qce_bam_acquire_lock(struct qce_device *qce)
> +{
> +	int ret;
> +
> +	qce_clear_bam_transaction(qce);

It's not entirely obvious that a "lock" operation will invalidate any
pending operations.

> +
> +	/* This is just a dummy write to acquire lock on bam pipe */
> +	qce_write_reg_dma(qce, REG_AUTH_SEG_CFG, 0, 1);
> +
> +	ret = qce_submit_cmd_desc(qce, QCE_DMA_DESC_FLAG_LOCK);
> +	if (ret) {
> +		dev_err(qce->dev, "Error in Locking cmd descriptor\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int qce_bam_release_lock(struct qce_device *qce)

What would be a reasonable response from the caller if this release
operation returns a failure? How do you expect it to recover?

> +{
> +	int ret;
> +
> +	qce_clear_bam_transaction(qce);
> +

In particularly not on "unlock".

Regards,
Bjorn

> +	/* This just dummy write to release lock on bam pipe*/
> +	qce_write_reg_dma(qce, REG_AUTH_SEG_CFG, 0, 1);
> +
> +	ret = qce_submit_cmd_desc(qce, QCE_DMA_DESC_FLAG_UNLOCK);
> +	if (ret) {
> +		dev_err(qce->dev, "Error in Un-Locking cmd descriptor\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/crypto/qce/core.h b/drivers/crypto/qce/core.h
> index bf28dedd1509..d01d810b60ad 100644
> --- a/drivers/crypto/qce/core.h
> +++ b/drivers/crypto/qce/core.h
> @@ -68,4 +68,6 @@ int qce_read_reg_dma(struct qce_device *qce, unsigned int offset, void *buff,
>  void qce_clear_bam_transaction(struct qce_device *qce);
>  int qce_submit_cmd_desc(struct qce_device *qce, unsigned long flags);
>  struct qce_bam_transaction *qce_alloc_bam_txn(struct qce_dma_data *dma);
> +int qce_bam_acquire_lock(struct qce_device *qce);
> +int qce_bam_release_lock(struct qce_device *qce);
>  #endif /* _CORE_H_ */
> -- 
> 2.34.1
>