Re: [PATCH v2 11/20] s390/zcrypt/pkey: Rework ep11 findcard() implementation and callers

Holger Dengler
2025-03-20T08:30:49Z

On 04/03/2025 18:21, Harald Freudenberger wrote:
> Rework the memory usage of the ep11 findcard() implementation:
> - findcard does not allocate memory for the list of apqns
> any more.
> - the callers are now responsible to provide an array of
> apqns to store the matching apqns into.

See my comments below.

>
> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> ---
> drivers/s390/crypto/pkey_ep11.c | 19 ++++++++-----------
> drivers/s390/crypto/zcrypt_ep11misc.c | 18 ++++--------------
> drivers/s390/crypto/zcrypt_ep11misc.h | 12 +++++-------
> 3 files changed, 17 insertions(+), 32 deletions(-)
>
[...]
> diff --git a/drivers/s390/crypto/zcrypt_ep11misc.c b/drivers/s390/crypto/zcrypt_ep11misc.c
> index 04153b476168..6005ef79c001 100644
> --- a/drivers/s390/crypto/zcrypt_ep11misc.c
> +++ b/drivers/s390/crypto/zcrypt_ep11misc.c
[...]
> @@ -1678,17 +1671,14 @@ int ep11_findcard2(u32 **apqns, u32 *nr_apqns, u16 cardnr, u16 domain,
> continue;
> }
> /* apqn passed all filtering criterons, add to the array */
> - if (_nr_apqns < 256)
> - _apqns[_nr_apqns++] = (((u16)card) << 16) | ((u16)dom);
> + if (_nr_apqns < *nr_apqns)
> + apqns[_nr_apqns++] = (((u16)card) << 16) | ((u16)dom);
> }
>
> /* nothing found ? */
> if (!_nr_apqns) {
> - kfree(_apqns);
> rc = -ENODEV;
> } else {
> - /* no re-allocation, simple return the _apqns array */
> - *apqns = _apqns;

Please update *apqns unconditionally.

> *nr_apqns = _nr_apqns;
> rc = 0;
> }
> diff --git a/drivers/s390/crypto/zcrypt_ep11misc.h b/drivers/s390/crypto/zcrypt_ep11misc.h
> index a4b98eca8431..cbd615547bc2 100644
> --- a/drivers/s390/crypto/zcrypt_ep11misc.h
> +++ b/drivers/s390/crypto/zcrypt_ep11misc.h
> @@ -136,14 +136,12 @@ int ep11_clr2keyblob(u16 cardnr, u16 domain, u32 keybitsize, u32 keygenflags,
> * key for this domain. When a wkvp is given there will always be a re-fetch
> * of the domain info for the potential apqn - so this triggers an request
> * reply to each apqn eligible.
> - * The array of apqn entries is allocated with kmalloc and returned in *apqns;
> - * the number of apqns stored into the list is returned in *nr_apqns. One apqn
> - * entry is simple a 32 bit value with 16 bit cardnr and 16 bit domain nr and
> - * may be casted to struct pkey_apqn. The return value is either 0 for success
> - * or a negative errno value. If no apqn meeting the criteria is found,
> - * -ENODEV is returned.
> + * The caller should set *nr_apqns to the nr of elements available in *apqns.
> + * On return *nr_apqns is then updated with the nr of apqns filled into *apqns.
> + * The return value is either 0 for success or a negative errno value.
> + * If no apqn meeting the criteria is found, -ENODEV is returned.

As mentioned in my comment to patch 10/20: please update nr_apqns in any case. If -ENODEV should be dropped for cca, than also drop it here.

> */
> -int ep11_findcard2(u32 **apqns, u32 *nr_apqns, u16 cardnr, u16 domain,
> +int ep11_findcard2(u32 *apqns, u32 *nr_apqns, u16 cardnr, u16 domain,
> int minhwtype, int minapi, const u8 *wkvp);
>
> /*

--
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@linux.ibm.com