Re: [PATCH v4 03/11] KVM: Export kvm_uevent_notify_vm_create()

Ackerley Tng <[email protected]> writes:

> [email protected] writes:
>
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Low] The patch claims to export `kvm_uevent_notify_vm_create()` but fails to include the `EXPORT_SYMBOL_GPL()` or `EXPORT_SYMBOL_FOR_KVM_INTERNAL()` macro.
>> --
>>
>> --- Patch [3]: [PATCH v4 03/11] KVM: Export kvm_uevent_notify_vm_create() ---
>> commit 5e642ea16f0d267e622a352de60bfe397ecf79df
>> Author: Tarun Sahu <[email protected]>
>>
>> KVM: Export kvm_uevent_notify_vm_create()
>>
>> This commit exposes the kvm_uevent_notify_vm_create function to allow
>> triggering KVM event notifications during VM creation.
>>
>> [ ... ]
>>
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -6348,6 +6348,11 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
>>>  	kfree(env);
>>>  }
>>>
>>> +void kvm_uevent_notify_vm_create(struct kvm *kvm)
>>> +{
>>> +	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
>>> +}
>>
>> [Severity: Low]
>> The commit message mentions exporting this function, but is it missing an
>> EXPORT_SYMBOL_GPL or EXPORT_SYMBOL_FOR_KVM_INTERNAL declaration?
>>
>> Without the macro, won't dynamically loaded architecture-specific modules
>> like kvm-intel.ko hit undefined symbol linking errors when they try to
>> call it?
>>
>
> I'm not 100% sure about the use of EXPORT_SYMBOL... here but usually I
> compile kvm as a module to try and catch these types of linking
> issues. Maybe Sean will have a better method/explanation for this.

The Intention is just to make it global to kvm module files. it is only
used in kvm module so need to use EXPORT symbol as Sean also mentioned.
>
> This brings about another question I have though. How does luo work if
> the new kernel's deserialization of the persisted data structures
> depends on a module? Like in this case, KVM is a module, if KVM isn't
> loaded in the new kernel, do the data structures stick around until KVM
> is loaded? Or is KVM required to be loaded as a module in the new
> kernel?

If kvm module is not loaded then guest_memfd_luo/kvm_luo handler will
also not be registered. So luo_finish call will leak this memory and not
be freed. This is aligned with LUO design.

I have raised the saperate thread with LUO list to discuss if LUO_CORE
should throw a warning in such cases. [1]

[1] https://lore.kernel.org/all/[email protected]/

~Tarun
>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3