Re: [PATCH v7 07/25] s390/zcrypt: Introduce cprb mempool for ep11 misc functions

[Heiko Carstens <hca@linux.ibm.com>]

On Wed, Apr 23, 2025 at 12:15:16PM +0200, Harald Freudenberger wrote:
> Introduce a cprb mempool for the zcrypt ep11 misc functions
> (zcrypt_ep11misc.*) do some preparation rework to support
> a do-not-allocate path through some zcrypt ep11 misc functions.
> 
> The mempool is controlled by the zcrypt module parameter
> "mempool_threshold" which shall control the minimal amount
> of memory items for CCA and EP11.
> 
> The mempool shall support "mempool_threshold" requests/replies
> in parallel which means for EP11 to hold a send and receive
> buffer memory per request. Each of this cprb space items is
> limited to 8 KB. So by default the mempool consumes
>   5 * 2 * 8KB = 80KB
> 
> If the mempool is depleted upon one ep11 misc functions is
> called with the ZCRYPT_XFLAG_NOMEMALLOC xflag set, the function
> will fail with -ENOMEM and the caller is responsible for taking
> further actions.
> 
> This is only part of an rework to support a new xflag
> ZCRYPT_XFLAG_NOMEMALLOC but not yet complete.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> ---
>  drivers/s390/crypto/zcrypt_api.c      |   6 ++
>  drivers/s390/crypto/zcrypt_ep11misc.c | 149 +++++++++++++++-----------
>  drivers/s390/crypto/zcrypt_ep11misc.h |   1 +
>  3 files changed, 95 insertions(+), 61 deletions(-)

clang says:

    CC [M]  drivers/s390/crypto/zcrypt_ep11misc.o
      drivers/s390/crypto/zcrypt_ep11misc.c:1041:6: warning: variable 'rep_pl_size' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
       1041 |         if (!req)
            |             ^~~~
      drivers/s390/crypto/zcrypt_ep11misc.c:1121:20: note: uninitialized use occurs here
       1121 |         free_cprbmem(rep, rep_pl_size, true, xflags);
            |                           ^~~~~~~~~~~
      drivers/s390/crypto/zcrypt_ep11misc.c:1041:2: note: remove the 'if' if its condition is always false
       1041 |         if (!req)
            |         ^~~~~~~~~
       1042 |                 goto out;
            |                 ~~~~~~~~
      drivers/s390/crypto/zcrypt_ep11misc.c:1028:33: note: initialize the variable 'rep_pl_size' to silence this warning
       1028 |         size_t req_pl_size, rep_pl_size;
            |                                        ^
            |                                         = 0
      1 warning generated.

> @@ -1095,9 +1117,8 @@ static int ep11_cryptsingle(u16 card, u16 domain,
>  	*outbufsize = n;
>  
>  out:
> -	kfree(req);
> -	kfree(rep);
> -	kfree(urb);
> +	free_cprbmem(req, req_pl_size, true, xflags);
> +	free_cprbmem(rep, rep_pl_size, true, xflags);
>  	return rc;
>  }

Which is this hunk.

In addition:

> @@ -1161,7 +1183,7 @@ static int _ep11_unwrapkey(u16 card, u16 domain,
>  	req_pl_size = sizeof(struct uw_req_pl) + (iv ? 16 : 0)
>  		+ ASN1TAGLEN(keksize) + ASN1TAGLEN(0)
>  		+ ASN1TAGLEN(pinblob_size) + ASN1TAGLEN(enckeysize);
> -	req = alloc_cprb(req_pl_size);
> +	req = alloc_cprbmem(req_pl_size, xflags);
>  	if (!req)
>  		goto out;
>  	req_pl = (struct uw_req_pl *)(((u8 *)req) + sizeof(*req));
> @@ -1197,22 +1219,19 @@ static int _ep11_unwrapkey(u16 card, u16 domain,
>  	p += asn1tag_write(p, 0x04, enckey, enckeysize);
>  
>  	/* reply cprb and payload */
> -	rep = alloc_cprb(sizeof(struct uw_rep_pl));
> +	rep = alloc_cprbmem(sizeof(struct uw_rep_pl), xflags);
>  	if (!rep)
>  		goto out;
>  	rep_pl = (struct uw_rep_pl *)(((u8 *)rep) + sizeof(*rep));
>  
>  	/* urb and target */
> -	urb = kmalloc(sizeof(*urb), GFP_KERNEL);
> -	if (!urb)
> -		goto out;
>  	target.ap_id = card;
>  	target.dom_id = domain;
> -	prep_urb(urb, &target, 1,
> +	prep_urb(&urb, &target, 1,
>  		 req, sizeof(*req) + req_pl_size,
>  		 rep, sizeof(*rep) + sizeof(*rep_pl));
>  
> -	rc = zcrypt_send_ep11_cprb(urb, 0);
> +	rc = zcrypt_send_ep11_cprb(&urb, xflags);
>  	if (rc) {
>  		ZCRYPT_DBF_ERR("%s zcrypt_send_ep11_cprb(card=%d dom=%d) failed, rc=%d\n",
>  			       __func__, (int)card, (int)domain, rc);
> @@ -1245,9 +1264,8 @@ static int _ep11_unwrapkey(u16 card, u16 domain,
>  	*keybufsize = rep_pl->data_len;
>  
>  out:
> -	kfree(req);
> -	kfree(rep);
> -	kfree(urb);
> +	free_cprbmem(req, req_pl_size, true, xflags);

This would lead to a memset() with destination address 0 if alloc_cprbmem()
fails.

> @@ -1319,15 +1337,16 @@ static int _ep11_wrapkey(u16 card, u16 domain,
>  	} __packed * rep_pl;
>  	struct ep11_cprb *req = NULL, *rep = NULL;
>  	struct ep11_target_dev target;
> -	struct ep11_urb *urb = NULL;
> +	struct ep11_urb urb;
>  	size_t req_pl_size;
>  	int api, rc = -ENOMEM;
>  	u8 *p;
> +	const u32 xflags = 0;
>  
>  	/* request cprb and payload */
>  	req_pl_size = sizeof(struct wk_req_pl) + (iv ? 16 : 0)
>  		+ ASN1TAGLEN(keysize) + 4;
> -	req = alloc_cprb(req_pl_size);
> +	req = alloc_cprbmem(req_pl_size, xflags);
>  	if (!req)
>  		goto out;
...
> @@ -1405,9 +1421,8 @@ static int _ep11_wrapkey(u16 card, u16 domain,
>  	*datasize = rep_pl->data_len;
>  
>  out:
> -	kfree(req);
> -	kfree(rep);
> -	kfree(urb);
> +	free_cprbmem(req, req_pl_size, true, xflags);

Same here.

Please check all error paths in all patches and resubmit.
I'm going to revert the whole series for now. Thanks!