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

[Harald Freudenberger <freude@linux.ibm.com>]

On 2025-03-20 09:30, Holger Dengler wrote:
> 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);
>> 
>>  /*

The *nr_apqns is now unconditional updated. But I keep the behavior with
the -ENODEV as there is code relaying on that.
-> v3