Re: [PATCH v4 6/7] s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to crypto engine

On 2026-08-19 11:37, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Thread-local memory mapping (kmap_local_page) is leaked
> across an asynchronous boundary, leading to an invalid cross-thread
> kunmap_local.
> - [High] Double completion of asynchronous requests on error paths in
> `phmac_do_one_request`.
> - [High] Persistent hardware busy state causes the driver to complete
> the request with `-EBUSY`, which the crypto API swallows, leading to
> an infinite wait.
> --
> 
> --- Patch [6]: [PATCH v4 6/7] s390/crypto: Fix handling of EBUSY in
> PHMAC when req is pushed to crypto engine ---
> commit 85264caf4d41b4adaeaed2f15efd8b287915969c
> Author: Harald Freudenberger <[email protected]>
> 
> s390/crypto: Fix handling of EBUSY in PHMAC when req is pushed to 
> crypto engine
> 
> This commit fixes the handling of the -EBUSY return code when a request 
> is
> transferred to the crypto engine via 
> crypto_transfer_hash_request_to_engine().
> It adds -EBUSY as a successful transfer indicator alongside 
> -EINPROGRESS,
> preventing premature cleanup of the request context.
> 
>> diff --git a/arch/s390/crypto/phmac_s390.c 
>> b/arch/s390/crypto/phmac_s390.c
>> index 03ca33ffe6ccf..30c8a59af1a4e 100644
>> --- a/arch/s390/crypto/phmac_s390.c
>> +++ b/arch/s390/crypto/phmac_s390.c
> 
> [ ... ]
> 
>> @@ -629,12 +632,15 @@ static int phmac_update(struct ahash_request 
>> *req)
>>  		req_ctx->async_op = OP_UPDATE;
>>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, 
>> req);
>> -		if (rc != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
> 
> [Severity: Critical]
> This is a pre-existing issue, but does setting cleanup = false here 
> leak a
> thread-local memory mapping across an asynchronous boundary?
> 
> When the submitter thread maps the scatterlist page using 
> kmap_local_page()
> via hwh_prepare() calling into crypto_hash_walk_first(), the map is 
> strictly
> thread-local. If the request is pushed to the engine and the submitter 
> thread
> returns, the local memory remains mapped in its thread context.
> 
> Later, the crypto_engine kworker thread processes the queued request in
> arch/s390/crypto/phmac_s390.c:phmac_do_one_request(), calling
> crypto_hash_walk_done(), which invokes kunmap_local() cross-thread. Can 
> this
> cause a kernel BUG or panic by violating the thread-locality 
> requirements of
> kmap_local_page()?
> 

This is not exactly true. On s390 CONFIG_HIGHMEM is NOT set.

 From /ext/git/s390-linux/include/crypto/scatterwalk.h:

   static inline void scatterwalk_map(struct scatter_walk *walk)
   {
      if (IS_ENABLED(CONFIG_HIGHMEM)) {
          // Uses kmap_local_page() - THREAD-LOCAL
          addr = kmap_local_page(page) + offset;
      } else {
          // Direct mapping - NO thread restrictions
          addr = page_address(base_page) + offset;
      }
   }

So there is no thread restriction on s390 and paes and phmac are both 
only s390.

>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
>> 
>> -	if (rc != -EINPROGRESS) {
>> -		hwh_advance(hwh, rc);
>> +	if (cleanup) {
>> +		if (hwh->walkbytes > 0)
>> +			hwh_advance(hwh, rc);
>>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>>  	}
>> 
> 
> [ ... ]
> 
>> @@ -667,12 +674,14 @@ static int phmac_final(struct ahash_request 
>> *req)
>>  		req_ctx->async_op = OP_FINAL;
>>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, 
>> req);
>> -		if (rc != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
> 
> [Severity: High]
> This is a pre-existing issue, but does the driver properly translate 
> persistent
> -EBUSY hardware errors before completion?
> 
> If the hardware persistently returns -EBUSY when deriving a protected 
> key in
> arch/s390/crypto/phmac_s390.c:convert_key(), it exhausts its retries 
> and
> returns -EBUSY. This propagates to phmac_do_one_request(), which 
> explicitly
> finalizes the request:
> 
>     crypto_finalize_hash_request(engine, req, rc);
> 
> Since the crypto API's completion wrapper ahash_op_done() explicitly 
> swallows
> -EBUSY because it treats it as a queueing status, completing a request 
> with
> -EBUSY prevents the final completion callback from firing. Could this 
> cause
> callers using crypto_wait_req() to block forever?
> 
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
>> 
>>  out:
>> -	if (rc != -EINPROGRESS)
>> +	if (cleanup)
>>  		memzero_explicit(kmac_ctx, sizeof(*kmac_ctx));
>>  	pr_debug("rc=%d\n", rc);
>>  	return rc;
> 
> [ ... ]
> 
>> @@ -716,15 +726,17 @@ static int phmac_finup(struct ahash_request 
>> *req)
>>  		/* req->async_op has been set to either OP_FINUP or OP_FINAL */
>>  		atomic_inc(&tfm_ctx->via_engine_ctr);
>>  		rc = crypto_transfer_hash_request_to_engine(phmac_crypto_engine, 
>> req);
>> -		if (rc != -EINPROGRESS)
>> +		if (rc == -EINPROGRESS || rc == -EBUSY)
>> +			cleanup = false;
>> +		else
>>  			atomic_dec(&tfm_ctx->via_engine_ctr);
>>  	}
> 
> [Severity: High]
> This is a pre-existing issue, but is there a risk of double completion 
> on error
> paths in arch/s390/crypto/phmac_s390.c:phmac_do_one_request()?
> 
> If hardware execution fails and returns an error like -EIO,
> phmac_do_one_request() manually completes the request:
> 
>     crypto_finalize_hash_request(engine, req, rc);
>     local_bh_enable();
>     return rc;
> 
> By returning the negative rc back to the crypto_engine core, and 
> because the
> engine allocates with retry_support=true, the core in 
> crypto_pump_requests()
> will treat this as a failure it must handle. Does this trigger an 
> automatic
> fallback crypto_request_complete() that invokes the user's completion 
> callback
> a second time?