Re: [PATCH v2] EFI: make for_each_efi_memory_desc_in_map() cope with running on Xen

Jan Beulich
2016-08-19T12:07:34Z

>>> On 19.08.16 at 12:47, <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> On Tue, 16 Aug, at 12:49:17PM, Matt Fleming wrote:
>> On Mon, 15 Aug, at 09:05:45AM, Jan Beulich wrote:
>> > --- 4.8-rc2/include/linux/efi.h
>> > +++ 4.8-rc2-EFI-memdesc-iterator-Xen/include/linux/efi.h
>> > @@ -946,7 +946,11 @@ extern int efi_memattr_apply_permissions
>> > /* Iterate through an efi_memory_map */
>> > #define for_each_efi_memory_desc_in_map(m, md) \
>> > for ((md) = (m)->map; \
>> > - ((void *)(md) + (m)->desc_size) <= (m)->map_end; \
>> > + /* \
>> > + * Leverage wrapping of the calculation to avoid an infinite \
>> > + * loop when all three values are zero. \
>> > + */ \
>> > + ((void *)(md) + (m)->desc_size - 1) < (m)->map_end; \
>> > (md) = (void *)(md) + (m)->desc_size)
>> >
>>
>> Thanks Jan, applied to the 'urgent' EFI queue and tagged for stable.
>> I'll send it out this week.
>
> Jiri caught me on IRC and repeated his concern that this patch relies
> on undefined behaviour per the C language standard.
>
> I really don't want to get into a discussion of compiler
> implementations, so instead can we check whether the 'md' is NULL and
> skip the loop if so? e.g. would something like this work?

I think so, yes, but I've not seen the issue myself, so I can't verify
that's the case.

Jan

> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -946,7 +946,7 @@ extern int efi_memattr_apply_permissions(struct mm_struct *mm,
> /* Iterate through an efi_memory_map */
> #define for_each_efi_memory_desc_in_map(m, md) \
> for ((md) = (m)->map; \
> - ((void *)(md) + (m)->desc_size) <= (m)->map_end; \
> + (md) && ((void *)(md) + (m)->desc_size) <= (m)->map_end; \
> (md) = (void *)(md) + (m)->desc_size)
>
> /**