* Re: [PATCH] x86/kvm: fix the decrypted pages free in kvmclock
[not found] <20240611024835.43671-1-lirongqing@baidu.com>
@ 2024-06-17 8:30 ` Vitaly Kuznetsov
2024-06-20 11:23 ` [外部邮件] " Li,Rongqing
0 siblings, 1 reply; 2+ messages in thread
From: Vitaly Kuznetsov @ 2024-06-17 8:30 UTC (permalink / raw)
To: Li RongQing
Cc: pbonzini, wanpengli, tglx, mingo, bp, dave.hansen, x86, hpa, kvm
Li RongQing <lirongqing@baidu.com> writes:
> When set_memory_decrypted() fails, pages may be left fully or partially
> decrypted. before free the pages to return pool, it should be encypted via
> set_memory_encrypted(), and if encryption fails, leak the pages
Out of curiosity,
shouldn't we rather try to make set_memory_decrypted() more atomic to
avoid the need to hunt down all users of the API? E.g. in Hyper-V's
__vmbus_establish_gpadl() I see:
ret = set_memory_decrypted((unsigned long)kbuffer,
PFN_UP(size));
if (ret) {
dev_warn(&channel->device_obj->device,
...
doesn't it have the exact same issue you're trying to address for kvmclock?
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> arch/x86/kernel/kvmclock.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> index 5b2c152..5e9f9d2 100644
> --- a/arch/x86/kernel/kvmclock.c
> +++ b/arch/x86/kernel/kvmclock.c
> @@ -228,7 +228,8 @@ static void __init kvmclock_init_mem(void)
> r = set_memory_decrypted((unsigned long) hvclock_mem,
> 1UL << order);
> if (r) {
> - __free_pages(p, order);
> + if (!set_memory_encrypted((unsigned long)hvclock_mem, 1UL << order))
> + __free_pages(p, order);
> hvclock_mem = NULL;
> pr_warn("kvmclock: set_memory_decrypted() failed. Disabling\n");
> return;
--
Vitaly
^ permalink raw reply [flat|nested] 2+ messages in thread