On Wed, 12 Feb 2020 at 12:44, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Ard,
>
> While booting 5.6-rc1 on one of my test machines I noticed the WARN_ON
> on line 198 of arch/x86/platform/efi/efi_64.c trigger many times.
>
> I've done some debugging on this an this is caused by the following
> call path:
>
> drivers/firmware/efi/vars.c: efivar_init():
>
> unsigned long variable_name_size = 1024;
> efi_char16_t *variable_name;
> efi_guid_t vendor_guid;
>
> variable_name = kzalloc(variable_name_size, GFP_KERNEL);
> if (!variable_name) {
> printk(KERN_ERR "efivars: Memory allocation failed.\n");
> return -ENOMEM;
> }
>
> ...
>
>
> do {
> variable_name_size = 1024;
>
> status = ops->get_next_variable(&variable_name_size,
> variable_name,
> &vendor_guid);
> ...
>
> arch/x86/platform/efi/efi_64.c: efi_thunk_get_next_variable()
>
> ...
> phys_vendor = virt_to_phys_or_null(vendor);
> ...
>
> arch/x86/platform/efi/efi_64.c: virt_to_phys_or_null_size()
>
> ...
> WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> ...
>
> Specifically the problem is that the efi_guid_t vendor_guid has an 8 bytes
> aligned address and the WARN_ON checks for it being aligned to\
> sizeof(efi_guid_t) which is 16 bytes.
>
> I've fixed this for now with the following local fix, but I'm not sure
> what the alignment rules actually are so I'm not sure this is correct:
>
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -181,6 +181,7 @@ static inline phys_addr_t
> virt_to_phys_or_null_size(void *va, unsigned long size)
> {
> bool bad_size;
> + int alignment;
>
> if (!va)
> return 0;
> @@ -195,7 +196,8 @@ virt_to_phys_or_null_size(void *va, unsigned long size)
> */
> bad_size = size > PAGE_SIZE || !is_power_of_2(size);
>
> - WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> + alignment = size > 8 ? 8 : size;
> + WARN_ON(!IS_ALIGNED((unsigned long)va, alignment) || bad_size);
>
> return slow_virt_to_phys(va);
> }
>
>
> I have a feeling that this is the right thing to do, but as said I'm not 100%
> sure. If you can confirm that this is the right fix, then I can submit this
> upstream.
>
It seems that the purpose of the alignment check is to ensure that the
data does not cross a page boundary, so that the data is guaranteed to
be contiguous in the physical address space as well.
So in that light, the fix is most definitely wrong, although I am not
sure how this is supposed to work generally.
