On 04/03/2025 18:21, Harald Freudenberger wrote:
> Rework the memory usage of the cca 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.
>
> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
See my comments below.
> ---
> drivers/s390/crypto/pkey_cca.c | 25 +++++++++++--------------
> drivers/s390/crypto/zcrypt_ccamisc.c | 18 ++++--------------
> drivers/s390/crypto/zcrypt_ccamisc.h | 12 +++++-------
> 3 files changed, 20 insertions(+), 35 deletions(-)
>
[...]
> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.c b/drivers/s390/crypto/zcrypt_ccamisc.c
> index 65b4cdb9b478..d3b093dcdf30 100644
> --- a/drivers/s390/crypto/zcrypt_ccamisc.c
> +++ b/drivers/s390/crypto/zcrypt_ccamisc.c
[...]
> @@ -2006,17 +1999,14 @@ int cca_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;
In my opinion, the -ENODEV return value can be completely dropped. For the caller it should be sufficient to check for nr_apqns == 0.
> } else {
> - /* no re-allocation, simple return the _apqns array */
> - *apqns = _apqns;
> *nr_apqns = _nr_apqns;
Please update *nr_apqns unconditionally.
> rc = 0;
> }
> diff --git a/drivers/s390/crypto/zcrypt_ccamisc.h b/drivers/s390/crypto/zcrypt_ccamisc.h
> index 273edf2bb036..bed647a42eb2 100644
> --- a/drivers/s390/crypto/zcrypt_ccamisc.h
> +++ b/drivers/s390/crypto/zcrypt_ccamisc.h
> @@ -229,14 +229,12 @@ int cca_findcard(const u8 *key, u16 *pcardnr, u16 *pdomain, int verify);
> * cur_mkvp or old_mkvp values of the apqn are used.
> * The mktype determines which set of master keys to use:
> * 0 = AES_MK_SET - AES MK set, 1 = APKA MK_SET - APKA MK set
> - * 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.
Why not just return nr_apqns == 0 to indicate, that no matching device has been found? Anyhow, even if you would stay with the -ENODEV return value, *nr_apqns should be updated in any case.
> */
> -int cca_findcard2(u32 **apqns, u32 *nr_apqns, u16 cardnr, u16 domain,
> +int cca_findcard2(u32 *apqns, u32 *nr_apqns, u16 cardnr, u16 domain,
> int minhwtype, int mktype, u64 cur_mkvp, u64 old_mkvp,
> int verify);
>
--
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@xxxxxxxxxxxxx
