Re: [PATCH v1 01/20] s390/ap: Move response_type struct into ap_msg struct

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

On 2025-02-24 16:23, Holger Dengler wrote:
> On 23/02/2025 10:54, Harald Freudenberger wrote:
>> Move the very small response_type struct into struct ap_msg.
>> So there is no need to kmalloc this tiny struct with each
>> ap message preparation.
> 
> I understand the intention for this patch, but in my opinion the
> layering concept between ap and zcrypt is violated by defining the
> response-type as part of the ap message struct. But I don't have any
> better solution, so for the moment you may leave it as is.
> 
>> 
>> Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
>> ---
>>  drivers/s390/crypto/ap_bus.h           |  12 ++-
>>  drivers/s390/crypto/zcrypt_msgtype50.c |  22 +++---
>>  drivers/s390/crypto/zcrypt_msgtype6.c  | 101 
>> ++++++++++---------------
>>  3 files changed, 59 insertions(+), 76 deletions(-)
>> 
>> diff --git a/drivers/s390/crypto/ap_bus.h 
>> b/drivers/s390/crypto/ap_bus.h
>> index f4622ee4d894..a5d8f805625f 100644
>> --- a/drivers/s390/crypto/ap_bus.h
>> +++ b/drivers/s390/crypto/ap_bus.h
>> @@ -214,6 +214,15 @@ struct ap_queue {
>> 
>>  typedef enum ap_sm_wait (ap_func_t)(struct ap_queue *queue);
>> 
>> +struct ap_response_type {
>> +	struct completion work;
>> +	int type;
>> +};
>> +
>> +#define CEXXC_RESPONSE_TYPE_ICA  1
>> +#define CEXXC_RESPONSE_TYPE_XCRB 2
>> +#define CEXXC_RESPONSE_TYPE_EP11 3
>> +
> 
> I would leave the type defines in the zcrypt_msgtype50.c.

Done -> v2

> 
>>  struct ap_message {
>>  	struct list_head list;		/* Request queueing. */
>>  	unsigned long psmid;		/* Message id. */
>> @@ -222,7 +231,7 @@ struct ap_message {
>>  	size_t bufsize;			/* allocated msg buffer size */
>>  	u16 flags;			/* Flags, see AP_MSG_FLAG_xxx */
>>  	int rc;				/* Return code for this message */
>> -	void *private;			/* ap driver private pointer. */
>> +	struct ap_response_type response;
> 
> I don't like this change. The completion and the type are both
> message-type related. That means, this change pulls messate-type
> related data definitions into the ap-layer. On the other hand, I have
> currently no idea how this can be solved.
> 

Well, the "private" data could be opaque allocated in ap_init_apmsg 
without
any knowledge about the data - just the size. And the msg type 50 and 6
implementations could just check for the right size and then overlay the
private data bytes with their own struct.

>>  	/* receive is called from tasklet context */
>>  	void (*receive)(struct ap_queue *, struct ap_message *,
>>  			struct ap_message *);
>> @@ -250,7 +259,6 @@ static inline void ap_init_message(struct 
>> ap_message *ap_msg)
>>  static inline void ap_release_message(struct ap_message *ap_msg)
>>  {
>>  	kfree_sensitive(ap_msg->msg);
>> -	kfree_sensitive(ap_msg->private);
> 
> As far as I can see, the kfree_sensitive() was not really required, as
> this only contains the type and the completion, but no sensitive data,
> right?
> If the assumption is true, the change is ok (if not, we should replace
> it with a memzero_explicit()).

Your assumption is true - the private data only held a completion and
the type of the msg (type 50 or type 6) - all that is no sensitive info.

> 
>> @@ -454,25 +454,24 @@ static long zcrypt_msgtype50_modexpo(struct 
>> zcrypt_queue *zq,
>>  				     struct ica_rsa_modexpo *mex,
>>  				     struct ap_message *ap_msg)
>>  {
>> -	struct completion work;
>>  	int rc;
>> 
>>  	ap_msg->bufsize = MSGTYPE50_CRB3_MAX_MSG_SIZE;
>> -	ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
>> +	if (!ap_msg->msg)
>> +		ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
> 
> This change will be removed by the next patch in the series. Please
> drop it. There are other occurances later in the code, see my further
> comments.
> 

Done as you suggest -> v2

>>  	if (!ap_msg->msg)
>>  		return -ENOMEM;
>>  	ap_msg->receive = zcrypt_msgtype50_receive;
>>  	ap_msg->psmid = (((unsigned long)current->pid) << 32) +
>>  		atomic_inc_return(&zcrypt_step);
>> -	ap_msg->private = &work;
>>  	rc = ICAMEX_msg_to_type50MEX_msg(zq, ap_msg, mex);
>>  	if (rc)
>>  		goto out;
>> -	init_completion(&work);
>> +	init_completion(&ap_msg->response.work);
>>  	rc = ap_queue_message(zq->queue, ap_msg);
>>  	if (rc)
>>  		goto out;
>> -	rc = wait_for_completion_interruptible(&work);
>> +	rc = wait_for_completion_interruptible(&ap_msg->response.work);
>>  	if (rc == 0) {
>>  		rc = ap_msg->rc;
>>  		if (rc == 0)
> [...]
>> @@ -504,25 +502,24 @@ static long zcrypt_msgtype50_modexpo_crt(struct 
>> zcrypt_queue *zq,
>>  					 struct ica_rsa_modexpo_crt *crt,
>>  					 struct ap_message *ap_msg)
>>  {
>> -	struct completion work;
>>  	int rc;
>> 
>>  	ap_msg->bufsize = MSGTYPE50_CRB3_MAX_MSG_SIZE;
>> -	ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
>> +	if (!ap_msg->msg)
>> +		ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
> 
> Same here, please drop it.

-> v2

> 
>>  	if (!ap_msg->msg)
>>  		return -ENOMEM;
>>  	ap_msg->receive = zcrypt_msgtype50_receive;
>>  	ap_msg->psmid = (((unsigned long)current->pid) << 32) +
>>  		atomic_inc_return(&zcrypt_step);
>> -	ap_msg->private = &work;
>>  	rc = ICACRT_msg_to_type50CRT_msg(zq, ap_msg, crt);
>>  	if (rc)
>>  		goto out;
>> -	init_completion(&work);
>> +	init_completion(&ap_msg->response.work);
>>  	rc = ap_queue_message(zq->queue, ap_msg);
>>  	if (rc)
>>  		goto out;
>> -	rc = wait_for_completion_interruptible(&work);
>> +	rc = wait_for_completion_interruptible(&ap_msg->response.work);
>>  	if (rc == 0) {
>>  		rc = ap_msg->rc;
>>  		if (rc == 0)
> [...]
>> diff --git a/drivers/s390/crypto/zcrypt_msgtype6.c 
>> b/drivers/s390/crypto/zcrypt_msgtype6.c
>> index b64c9d9fc613..21ee311cf33d 100644
>> --- a/drivers/s390/crypto/zcrypt_msgtype6.c
>> +++ b/drivers/s390/crypto/zcrypt_msgtype6.c
>> @@ -31,15 +31,6 @@
>> 
>>  #define CEIL4(x) ((((x) + 3) / 4) * 4)
>> 
>> -struct response_type {
>> -	struct completion work;
>> -	int type;
>> -};
>> -
>> -#define CEXXC_RESPONSE_TYPE_ICA  0
>> -#define CEXXC_RESPONSE_TYPE_XCRB 1
>> -#define CEXXC_RESPONSE_TYPE_EP11 2
>> -
> 
> Please define the message types here (see my previous comment).
> 

done -> v2

>>  MODULE_AUTHOR("IBM Corporation");
>>  MODULE_DESCRIPTION("Cryptographic Coprocessor (message type 6), " \
>>  		   "Copyright IBM Corp. 2001, 2023");
> [...]
>> @@ -1061,28 +1046,26 @@ static long zcrypt_msgtype6_modexpo_crt(struct 
>> zcrypt_queue *zq,
>>   * Prepare a CCA AP msg: fetch the required data from userspace,
>>   * prepare the AP msg, fill some info into the ap_message struct,
>>   * extract some data from the CPRB and give back to the caller.
>> - * This function allocates memory and needs an ap_msg prepared
>> - * by the caller with ap_init_message(). Also the caller has to
>> - * make sure ap_release_message() is always called even on failure.
>> + * This function may allocate memory if the ap_msg msg buffer is
>> + * not preallocated and needs an ap_msg prepared by the caller
>> + * with ap_init_message(). Also the caller has to make sure
>> + * ap_release_message() is always called even on failure.
> 
> Please move this change to the patch, which makes the allocation 
> optional.

done -> v2

> 
>>   */
>>  int prep_cca_ap_msg(bool userspace, struct ica_xcRB *xcrb,
>>  		    struct ap_message *ap_msg,
>>  		    unsigned int *func_code, unsigned short **dom)
>>  {
>> -	struct response_type resp_type = {
>> -		.type = CEXXC_RESPONSE_TYPE_XCRB,
>> -	};
>> +	struct ap_response_type *resp_type = &ap_msg->response;
>> 
>>  	ap_msg->bufsize = atomic_read(&ap_max_msg_size);
>> -	ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
>> +	if (!ap_msg->msg)
>> +		ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
> 
> Please drop the kmalloc change.

done -> v2

> 
>>  	if (!ap_msg->msg)
>>  		return -ENOMEM;
>>  	ap_msg->receive = zcrypt_msgtype6_receive;
>>  	ap_msg->psmid = (((unsigned long)current->pid) << 32) +
>>  				atomic_inc_return(&zcrypt_step);
>> -	ap_msg->private = kmemdup(&resp_type, sizeof(resp_type), 
>> GFP_KERNEL);
>> -	if (!ap_msg->private)
>> -		return -ENOMEM;
>> +	resp_type->type = CEXXC_RESPONSE_TYPE_XCRB;
>>  	return xcrb_msg_to_type6cprb_msgx(userspace, ap_msg, xcrb, 
>> func_code, dom);
>>  }
>> 
> [...]
>> @@ -1158,28 +1141,26 @@ static long zcrypt_msgtype6_send_cprb(bool 
>> userspace, struct zcrypt_queue *zq,
>>   * Prepare an EP11 AP msg: fetch the required data from userspace,
>>   * prepare the AP msg, fill some info into the ap_message struct,
>>   * extract some data from the CPRB and give back to the caller.
>> - * This function allocates memory and needs an ap_msg prepared
>> - * by the caller with ap_init_message(). Also the caller has to
>> - * make sure ap_release_message() is always called even on failure.
>> + * This function may allocate memory if the ap_msg msg buffer is
>> + * not preallocated and needs an ap_msg prepared by the caller
>> + * with ap_init_message(). Also the caller has to make sure
>> + * ap_release_message() is always called even on failure.
> 
> Please move this change to the patch, which makes the allocation 
> optional.

done -> v2

> 
>>   */
>>  int prep_ep11_ap_msg(bool userspace, struct ep11_urb *xcrb,
>>  		     struct ap_message *ap_msg,
>>  		     unsigned int *func_code, unsigned int *domain)
>>  {
>> -	struct response_type resp_type = {
>> -		.type = CEXXC_RESPONSE_TYPE_EP11,
>> -	};
>> +	struct ap_response_type *resp_type = &ap_msg->response;
>> 
>>  	ap_msg->bufsize = atomic_read(&ap_max_msg_size);
>> -	ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
>> +	if (!ap_msg->msg)
>> +		ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
> 
> Please drop the kmalloc change.
> 

done -> v2

>>  	if (!ap_msg->msg)
>>  		return -ENOMEM;
>>  	ap_msg->receive = zcrypt_msgtype6_receive_ep11;
>>  	ap_msg->psmid = (((unsigned long)current->pid) << 32) +
>>  				atomic_inc_return(&zcrypt_step);
>> -	ap_msg->private = kmemdup(&resp_type, sizeof(resp_type), 
>> GFP_KERNEL);
>> -	if (!ap_msg->private)
>> -		return -ENOMEM;
>> +	resp_type->type = CEXXC_RESPONSE_TYPE_EP11;
>>  	return xcrb_msg_to_type6_ep11cprb_msgx(userspace, ap_msg, xcrb,
>>  					       func_code, domain);
>>  }
> [...]
>> @@ -1279,20 +1260,18 @@ static long 
>> zcrypt_msgtype6_send_ep11_cprb(bool userspace, struct zcrypt_queue *
>>  int prep_rng_ap_msg(struct ap_message *ap_msg, int *func_code,
>>  		    unsigned int *domain)
>>  {
>> -	struct response_type resp_type = {
>> -		.type = CEXXC_RESPONSE_TYPE_XCRB,
>> -	};
>> +	struct ap_response_type *resp_type = &ap_msg->response;
>> 
>>  	ap_msg->bufsize = AP_DEFAULT_MAX_MSG_SIZE;
>> -	ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
>> +	if (!ap_msg->msg)
>> +		ap_msg->msg = kmalloc(ap_msg->bufsize, GFP_KERNEL);
> 
> Please drop the kmalloc change.
> 

done -> v2

>>  	if (!ap_msg->msg)
>>  		return -ENOMEM;
>>  	ap_msg->receive = zcrypt_msgtype6_receive;
>>  	ap_msg->psmid = (((unsigned long)current->pid) << 32) +
>>  				atomic_inc_return(&zcrypt_step);
>> -	ap_msg->private = kmemdup(&resp_type, sizeof(resp_type), 
>> GFP_KERNEL);
>> -	if (!ap_msg->private)
>> -		return -ENOMEM;
>> +
>> +	resp_type->type = CEXXC_RESPONSE_TYPE_XCRB;
>> 
>>  	rng_type6cprb_msgx(ap_msg, ZCRYPT_RNG_BUFFER_SIZE, domain);
>> 
> [...]