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

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

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.