Re: [PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Bypassing `ctrblk_lock` in softirq context forces AES-CTR to process data one block (16 bytes) at a time, severely degrading performance for softirq crypto users like IPsec.
--

--- Patch [3]: [PATCH v3 3/3] s390/crypto: Rewrite AES ctr mode to be prepared for context analysis ---
commit 7a2551a9e3023dcb2323f560164fee63c00a3278
Author: Harald Freudenberger <[email protected]>

s390/crypto: Rewrite AES ctr mode to be prepared for context analysis

This commit rewrites the AES CTR implementation to resolve a clang
thread-safety warning regarding the ctrblk_lock mutex. It splits the
cryptographic operations into __ctr_aes_crypt() and checks in_task()
before attempting mutex_trylock() to avoid locking in softirq context.

> diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
> index 10561aa687c78..8295cb3fc56c2 100644
> --- a/arch/s390/crypto/aes_s390.c
> +++ b/arch/s390/crypto/aes_s390.c
> @@ -562,46 +562,62 @@ static unsigned int __ctrblk_init(u8 *ctrptr, u8 *iv, unsigned int nbytes)
[ ... ]
> +static int __ctr_aes_crypt(struct s390_aes_ctx *sctx,
> +			   struct skcipher_walk *walk, bool locked)
> +{
> +	unsigned int n, nbytes;
> +	int ret = 0;
> +	u8 *ctrptr;
> +
> +	while (!ret && ((nbytes = walk->nbytes) >= AES_BLOCK_SIZE)) {
> +		n = AES_BLOCK_SIZE;
> +		if (nbytes >= 2 * AES_BLOCK_SIZE && locked)
> +			n = __ctrblk_init(ctrblk, walk->iv, nbytes);
> +		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk->iv;

[Severity: High]
Since locked is false for softirq contexts, does this code bypass
__ctrblk_init() and clamp n to AES_BLOCK_SIZE (16 bytes)?

This causes the loop to only process 16 bytes per iteration instead of
handling the payload in bulk.

[ ... ]
>  static int ctr_aes_crypt(struct skcipher_request *req)
>  {
[ ... ]
>  	ret = skcipher_walk_virt(&walk, req, false);
> -	while (!ret && ((nbytes = walk.nbytes) >= AES_BLOCK_SIZE)) {
> -		n = AES_BLOCK_SIZE;
> +	if (ret)
> +		return ret;
>  
> -		if (nbytes >= 2*AES_BLOCK_SIZE && locked)
> -			n = __ctrblk_init(ctrblk, walk.iv, nbytes);
> -		ctrptr = (n > AES_BLOCK_SIZE) ? ctrblk : walk.iv;
> -		cpacf_kmctr(sctx->fc, sctx->key, walk.dst.virt.addr,
> -			    walk.src.virt.addr, n, ctrptr);
> -		if (ctrptr == ctrblk)
> -			memcpy(walk.iv, ctrptr + n - AES_BLOCK_SIZE,
> -			       AES_BLOCK_SIZE);
> -		crypto_inc(walk.iv, AES_BLOCK_SIZE);
> -		ret = skcipher_walk_done(&walk, nbytes - n);
> -	}
> -	if (locked)
> +	if (in_task() && mutex_trylock(&ctrblk_lock)) {

[Severity: High]
Does this in_task() check unconditionally prevent softirq contexts from
acquiring the mutex?

Network crypto frameworks (e.g. IPsec) typically execute AES-CTR 
operations within softirq contexts (NET_RX_SOFTIRQ, NET_TX_SOFTIRQ), where 
in_task() evaluates to false. This forces all such softirq users into the
fallback path calling __ctr_aes_crypt(sctx, &walk, false).

By forcing softirq contexts to process data 16 bytes at a time, does this
introduce a massive performance regression for network crypto workloads?

> +		/* process context and mutex acquired */
> +		ret = __ctr_aes_crypt(sctx, &walk, true);
>  		mutex_unlock(&ctrblk_lock);
> +	} else {
> +		ret = __ctr_aes_crypt(sctx, &walk, false);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3