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

[Holger Dengler <dengler@linux.ibm.com>]

On 09/04/2025 16:02, 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>

I have a few comments below. The rest looks good to me.

> ---
>  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;

I would prefer to set rc in the kmalloc error handling. See below.

>  	if (target_num != 0) {
>  		struct ep11_target_dev __user *uptr;

*uptr is no longer required, as the userspace case is ensured by code.

>  
> -		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;

Please set rc here. It makes it easier to read as it correlates with the other error case for copy_from_user().

if (!targets) {
	rc = -ENOMEM;
	goto out;
}

> +			uptr = (struct ep11_target_dev __force __user *)xcrb->targets;
> +			if (copy_from_user(targets, uptr,
> +					   target_num * sizeof(*targets))) {

The cast can be dropped and you can copy directly from the userspace pointer in xcrb.

if (copy_from_user(targets, xcrb->targets,
		   target_num * sizeof(*targets))) {

> +				rc = -EFAULT;
> +				goto out;
> +			}
> +		} else {
> +			targets = (struct ep11_target_dev __force __kernel *)xcrb->targets;
>  		}
>  	}
>  
>  	rc = prep_ep11_ap_msg(userspace, xcrb, &ap_msg, &func_code, &domain);
>  	if (rc)
> -		goto out_free;
> +		goto out;
>  	print_hex_dump_debug("ep11req: ", DUMP_PREFIX_ADDRESS, 16, 1,
>  			     ap_msg.msg, ap_msg.len, false);
>  
[...]

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