[PATCH v2 0/1] Remove the need to alloc memory in uv.c

[PATCH v2 0/1] Remove the need to alloc memory in uv.c

[Harald Freudenberger]

The pkey uv handler may be called in a do-not-allocate memory
situation. For example when an encrypted swap file is used and the
encryption is done via UV retrievable secrets with protected keys.

The pkey uv handler calls uv_get_secret_metadata() and thus has a need
to have this function work without memory allocations. So this patch
extends the uv_get_secret_metadata() function to be able to work on
a provided working page instead of allocating/freeing memory via
kmalloc/kfree:

int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
			   struct uv_secret_list_item_hdr *secret,
			   u8 *workpage);

Parameter workpage is a ephemeral working page used by the function.
If given (!= NULL), it needs to point to memory of at least PAGE_SIZE
bytes. If NULL, the function uses kmalloc/kfree to allocate and free a
working buffer.

Changelog:
v1: Pre-allocated one page during init of the uv.c code.
v2: As a result of feedback from Heiko about the v1 implementation and
    with another idea of how to deal with a do-not-allocate situation
    in uv.c now another approach: The caller may give a ptr to an
    ephemeral working page if no memory may be allocated.
    Note this patch does not compile as pkey_uv.c needs to get
    adapted to this changed uv function. However, patch is good as a
    starting point for code review and discussions.
    
Harald Freudenberger (1):
  s390/uv: New param workpage for the uv_get_secret_metadata() function

 arch/s390/include/asm/uv.h |  3 ++-
 arch/s390/kernel/uv.c      | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

--
2.43.0

[PATCH v2 1/1] s390/uv: New param workpage for the uv_get_secret_metadata() function

[Harald Freudenberger]

The pkey uv handler may be called in a do-not-allocate memory
situation. For example when an encrypted swap file is used and the
encryption is done via UV retrievable secrets with protected keys.

The pkey uv handler calls uv_get_secret_metadata() and thus has a need
to have this function work without memory allocations. So this patch
extends the uv_get_secret_metadata() function to be able to work on
a provided working page instead of allocating/freeing memory via
kmalloc/kfree:

int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
			   struct uv_secret_list_item_hdr *secret,
			   u8 *workpage);

Parameter workpage is a ephemeral working page used by the function.
If given (!= NULL), it needs to point to memory of at least PAGE_SIZE
bytes. If NULL, the function uses kmalloc/kfree to allocate and free a
working buffer.

Signed-off-by: Harald Freudenberger <freude@linux.ibm.com>
---
 arch/s390/include/asm/uv.h |  3 ++-
 arch/s390/kernel/uv.c      | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index b11f5b6d0bd1..496cd9c9f2a5 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -617,7 +617,8 @@ static inline int uv_remove_shared(unsigned long addr)
 }
 
 int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
-			   struct uv_secret_list_item_hdr *secret);
+			   struct uv_secret_list_item_hdr *secret,
+			   u8 *workpage);
 int uv_retrieve_secret(u16 secret_idx, u8 *buf, size_t buf_size);
 
 extern int prot_virt_host;
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
index 9f05df2da2f7..0a8a6bc19c49 100644
--- a/arch/s390/kernel/uv.c
+++ b/arch/s390/kernel/uv.c
@@ -713,6 +713,9 @@ static int find_secret(const u8 secret_id[UV_SECRET_ID_LEN],
  * uv_get_secret_metadata() - get secret metadata for a given secret id.
  * @secret_id: search pattern.
  * @secret: output data, containing the secret's metadata.
+ * @workpage: ephemeral working page. Caller may give a ptr to one page
+ *            here as ephemeral working buffer. If NULL, kmalloc is used
+ *            to alloc a working buffer.
  *
  * Search for a secret with the given secret_id in the Ultravisor secret store.
  *
@@ -725,16 +728,19 @@ static int find_secret(const u8 secret_id[UV_SECRET_ID_LEN],
  * * %EIO:	- Other unexpected UV error.
  */
 int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
-			   struct uv_secret_list_item_hdr *secret)
+			   struct uv_secret_list_item_hdr *secret,
+			   u8 *workpage)
 {
 	struct uv_secret_list *buf;
 	int rc;
 
-	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+	buf = workpage ? (struct uv_secret_list *)workpage :
+		kzalloc(sizeof(*buf), GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 	rc = find_secret(secret_id, buf, secret);
-	kfree(buf);
+	if (!workpage)
+		kfree(buf);
 	return rc;
 }
 EXPORT_SYMBOL_GPL(uv_get_secret_metadata);
-- 
2.43.0

Re: [PATCH v2 1/1] s390/uv: New param workpage for the uv_get_secret_metadata() function

[Heiko Carstens]

On Mon, Mar 31, 2025 at 12:35:05PM +0200, Harald Freudenberger wrote:
> The pkey uv handler may be called in a do-not-allocate memory
> situation. For example when an encrypted swap file is used and the
> encryption is done via UV retrievable secrets with protected keys.

This doesn't answer the question if the context is process, bottom halve,
or interrupt context. If it is process context, is it sleepable?

> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
> index 9f05df2da2f7..0a8a6bc19c49 100644
> --- a/arch/s390/kernel/uv.c
> +++ b/arch/s390/kernel/uv.c
> @@ -713,6 +713,9 @@ static int find_secret(const u8 secret_id[UV_SECRET_ID_LEN],
>   * uv_get_secret_metadata() - get secret metadata for a given secret id.
>   * @secret_id: search pattern.
>   * @secret: output data, containing the secret's metadata.
> + * @workpage: ephemeral working page. Caller may give a ptr to one page
> + *            here as ephemeral working buffer. If NULL, kmalloc is used
> + *            to alloc a working buffer.
>   *
>   * Search for a secret with the given secret_id in the Ultravisor secret store.
>   *
> @@ -725,16 +728,19 @@ static int find_secret(const u8 secret_id[UV_SECRET_ID_LEN],
>   * * %EIO:	- Other unexpected UV error.
>   */
>  int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
> -			   struct uv_secret_list_item_hdr *secret)
> +			   struct uv_secret_list_item_hdr *secret,
> +			   u8 *workpage)
>  {
>  	struct uv_secret_list *buf;
>  	int rc;
>  
> -	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> +	buf = workpage ? (struct uv_secret_list *)workpage :
> +		kzalloc(sizeof(*buf), GFP_KERNEL);
>  	if (!buf)
>  		return -ENOMEM;
>  	rc = find_secret(secret_id, buf, secret);
> -	kfree(buf);
> +	if (!workpage)
> +		kfree(buf);
>  	return rc;
>  }
>  EXPORT_SYMBOL_GPL(uv_get_secret_metadata);

Please don't do this ugly workpage interface. Just rename find_secret() to
e.g. uv_find_secret() and make it globally visable. Then you have the
unchanged uv_get_secret_metadata() + find_secret() interfaces, and in addition
can call find_secret() / uv_find_secret() with a custom buffer.

Given that pkey_uv.c is the only user of this interface: how would the changes
to pkey_uv.c look like?  Or in other words: wouldn't it make more sense to get
rid of uv_get_secret_metadata(), and just keep uv_find_secret() and push the
allocation to the pkey code?

Re: [PATCH v2 1/1] s390/uv: New param workpage for the uv_get_secret_metadata() function

[Harald Freudenberger]

On 2025-03-31 16:55, Heiko Carstens wrote:
> On Mon, Mar 31, 2025 at 12:35:05PM +0200, Harald Freudenberger wrote:
>> The pkey uv handler may be called in a do-not-allocate memory
>> situation. For example when an encrypted swap file is used and the
>> encryption is done via UV retrievable secrets with protected keys.
> 
> This doesn't answer the question if the context is process, bottom 
> halve,
> or interrupt context. If it is process context, is it sleepable?

Sleepable but no memory must be allocated which would cause IO 
operations.

> 
>> diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c
>> index 9f05df2da2f7..0a8a6bc19c49 100644
>> --- a/arch/s390/kernel/uv.c
>> +++ b/arch/s390/kernel/uv.c
>> @@ -713,6 +713,9 @@ static int find_secret(const u8 
>> secret_id[UV_SECRET_ID_LEN],
>>   * uv_get_secret_metadata() - get secret metadata for a given secret 
>> id.
>>   * @secret_id: search pattern.
>>   * @secret: output data, containing the secret's metadata.
>> + * @workpage: ephemeral working page. Caller may give a ptr to one 
>> page
>> + *            here as ephemeral working buffer. If NULL, kmalloc is 
>> used
>> + *            to alloc a working buffer.
>>   *
>>   * Search for a secret with the given secret_id in the Ultravisor 
>> secret store.
>>   *
>> @@ -725,16 +728,19 @@ static int find_secret(const u8 
>> secret_id[UV_SECRET_ID_LEN],
>>   * * %EIO:	- Other unexpected UV error.
>>   */
>>  int uv_get_secret_metadata(const u8 secret_id[UV_SECRET_ID_LEN],
>> -			   struct uv_secret_list_item_hdr *secret)
>> +			   struct uv_secret_list_item_hdr *secret,
>> +			   u8 *workpage)
>>  {
>>  	struct uv_secret_list *buf;
>>  	int rc;
>> 
>> -	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
>> +	buf = workpage ? (struct uv_secret_list *)workpage :
>> +		kzalloc(sizeof(*buf), GFP_KERNEL);
>>  	if (!buf)
>>  		return -ENOMEM;
>>  	rc = find_secret(secret_id, buf, secret);
>> -	kfree(buf);
>> +	if (!workpage)
>> +		kfree(buf);
>>  	return rc;
>>  }
>>  EXPORT_SYMBOL_GPL(uv_get_secret_metadata);
> 
> Please don't do this ugly workpage interface. Just rename find_secret() 
> to
> e.g. uv_find_secret() and make it globally visable. Then you have the
> unchanged uv_get_secret_metadata() + find_secret() interfaces, and in 
> addition
> can call find_secret() / uv_find_secret() with a custom buffer.

Will do.

> 
> Given that pkey_uv.c is the only user of this interface: how would the 
> changes
> to pkey_uv.c look like?  Or in other words: wouldn't it make more sense 
> to get
> rid of uv_get_secret_metadata(), and just keep uv_find_secret() and 
> push the
> allocation to the pkey code?

As of now the pkey_uv.c would continue to use the old function.
This would then change when my 20 do-not-allocate patch series is 
applied.
After that or with that, I would hold one pre-allocated page in pkey_uv
for exactly this purpose and use the uv_find_secret with this 
pre-allocated
page - thus not using uv_get_secret_metadata() any more.