On 23/02/2025 10:54, Harald Freudenberger wrote:
> There is a need for a do-not-allocate-memory path through the
> ap bus layer. When ap_init_apmsg() with the AP_MSG_FLAG_MEMPOOL
> xflag is called, instead of kmalloc() the ap message buffer is
> allocated from the ap_msg_pool. This pool only holds a limited
> amount of buffers: AP_MSG_POOL_MIN_ITEMS with the item size
> AP_DEFAULT_MAX_MSG_SIZE and exactly one of these items (if available)
> is returned if ap_init_apmsg() with the MEMPOOL flag is called.
> When this pool is exhausted and the MEMPOOL flag is effective,
> ap_init_apmsg() returns -ENOMEM without any attempt to allocate
> memory.
>
> The zcrypt layer may use this flag to indicate to the ap bus
> that the processing path for this message should not allocate
> memory. This is to prevent deadlocks with crypto and io for
> example with encrypted swap volumes.
See my comments below.
The rest looks good to me.
>
> Signed-off-by: Harald Freudenberger <freude@xxxxxxxxxxxxx>
> ---
> drivers/s390/crypto/ap_bus.c | 59 +++++++++++++++++++++++++++-----
> drivers/s390/crypto/ap_bus.h | 3 +-
> drivers/s390/crypto/zcrypt_api.c | 10 +++---
> 3 files changed, 57 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
> index 4940eaf538e9..b585b5d11074 100644
> --- a/drivers/s390/crypto/ap_bus.c
> +++ b/drivers/s390/crypto/ap_bus.c
[...]
> @@ -546,16 +562,27 @@ static void ap_poll_thread_stop(void)
> #define is_card_dev(x) ((x)->parent == ap_root_device)
> #define is_queue_dev(x) ((x)->parent != ap_root_device)
>
> -/**
> +/*
What is the reason for mixing coding styles?
> * ap_init_apmsg() - Initialize ap_message.
> - * Initialize a message before using. Otherwise this might result in
> - * unexpected behaviour.
> + * Initialize struct ap_message and allocate buffer to construct
> + * the ap message.
> */
> -int ap_init_apmsg(struct ap_message *ap_msg)
> +int ap_init_apmsg(struct ap_message *ap_msg, u32 xflags)
The xflags function parameter is very confusing (here and also in all other APIs too), because it allows to set some, but not all flags in ap_msg-flags. Why not using `bool alloc`? If you will keep the more flexible interface, please add another xflags element to struct ap_message. There is nothing in common between the ap_msg->flags and xflags, beside they're both named "flags".
> {
> - unsigned int maxmsgsize = atomic_read(&ap_max_msg_size);
> + unsigned int maxmsgsize;
>
> memset(ap_msg, 0, sizeof(*ap_msg));
> +
> + if (xflags & AP_MSG_FLAG_MEMPOOL) {
> + ap_msg->msg = mempool_alloc_preallocated(ap_msg_pool);
> + if (!ap_msg->msg)
> + return -ENOMEM;
> + ap_msg->bufsize = AP_DEFAULT_MAX_MSG_SIZE;
> + ap_msg->flags |= AP_MSG_FLAG_MEMPOOL;
> + return 0;
> + }
> +
> + maxmsgsize = atomic_read(&ap_max_msg_size);
> ap_msg->msg = kmalloc(maxmsgsize, GFP_KERNEL);
> if (!ap_msg->msg)
> return -ENOMEM;
> @@ -565,14 +592,18 @@ int ap_init_apmsg(struct ap_message *ap_msg)
> }
> EXPORT_SYMBOL(ap_init_apmsg);
>
> -/**
> +/*
???
> * ap_release_apmsg() - Release ap_message.
> - * Releases all memory used internal within the ap_message struct
> - * Currently this is the message and private field.
> + * Cleanup struct ap_message and release all memory held.
> */
> void ap_release_apmsg(struct ap_message *ap_msg)
> {
> - kfree_sensitive(ap_msg->msg);
> + if (ap_msg->flags & AP_MSG_FLAG_MEMPOOL) {
> + memzero_explicit(ap_msg->msg, ap_msg->bufsize);
> + mempool_free(ap_msg->msg, ap_msg_pool);
> + } else {
> + kfree_sensitive(ap_msg->msg);
> + }
> }
> EXPORT_SYMBOL(ap_release_apmsg);
>
[...]
--
Mit freundlichen Grüßen / Kind regards
Holger Dengler
--
IBM Systems, Linux on IBM Z Development
dengler@xxxxxxxxxxxxx
