Re: [PATCH v6 04/25] s390/zcrypt: Avoid alloc and copy of ep11 targets if kernelspace cprb

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

On 2025-04-18 09:57, Holger Dengler wrote:
> On 16/04/2025 15:57, Harald Freudenberger wrote:
>> If there is a target list of APQNs given when an CPRB is
>> to be send via zcrypt_send_ep11_cprb() there is always a
>> kmalloc() done and the targets are copied via z_copy_from_user.
>> 
>> As there are callers from kernel space (zcrypt_ep11misc.c)
>> which signal this via the userspace parameter improve this
>> code to directly use the given target list in case of
>> kernelspace thus removing the unnecessary memory alloc
>> and mem copy.
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
> 
> With the change below
> Reviewed-by: Holger Dengler <dengler@linux.ibm.com>
> 
>> ---
>>  drivers/s390/crypto/zcrypt_api.c | 41 
>> ++++++++++++++++----------------
>>  1 file changed, 20 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/s390/crypto/zcrypt_api.c 
>> b/drivers/s390/crypto/zcrypt_api.c
>> index ad09c5306e28..f96f596e578f 100644
>> --- a/drivers/s390/crypto/zcrypt_api.c
>> +++ b/drivers/s390/crypto/zcrypt_api.c
> [...]
>> @@ -1052,29 +1052,28 @@ static long _zcrypt_send_ep11_cprb(bool 
>> userspace, struct ap_perms *perms,
>>  	target_num = (unsigned short)xcrb->targets_num;
>> 
>>  	/* empty list indicates autoselect (all available targets) */
>> -	targets = NULL;
>> +	rc = -ENOMEM;
>>  	if (target_num != 0) {
>>  		struct ep11_target_dev __user *uptr;
>> 
>> -		targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
>> -		if (!targets) {
>> -			func_code = 0;
>> -			rc = -ENOMEM;
>> -			goto out;
>> -		}
>> -
>> -		uptr = (struct ep11_target_dev __force __user *)xcrb->targets;
>> -		if (z_copy_from_user(userspace, targets, uptr,
>> -				     target_num * sizeof(*targets))) {
>> -			func_code = 0;
>> -			rc = -EFAULT;
>> -			goto out_free;
>> +		if (userspace) {
>> +			targets = kcalloc(target_num, sizeof(*targets), GFP_KERNEL);
>> +			if (!targets)
>> +				goto out;
>> +			uptr = (struct ep11_target_dev __force __user *)xcrb->targets;
> 
> I mentioned this earlier already, the cast is not required here,
> because xcrb->targets is always a user pointer in this if clause.
> 
>> +			if (copy_from_user(targets, uptr,
>> +					   target_num * sizeof(*targets))) {
> 
> You can use xcrb->targets directly without any cast (and also drop 
> uptr).
> 
> if (copy_from_user(targets, xcrb->targets,
>     target_num * sizeof(*targets))) {
> 
> [...]

All these requested changes are included in v6. So only
Holger's signed-off was missing. Should I re-send a v7 with Holger's
signed-off on patch #4 ?