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

Harald Freudenberger
2025-03-25T13:02:25Z

On 2025-03-19 18:58, Holger Dengler wrote:
> 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@linux.ibm.com>
>
> 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);
>>

I changed this to unconditionally always update *nr_apqns.
But I'd like to stay with the -ENODEV in case 0 apqns have been found.
There is code using this returncode (which then also would need
reworked).