Re: [PATCH 09/11] kexec: avoid effectively open-coding xzalloc_flex_struct()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.04.2021 14:54, Andrew Cooper wrote:
> On 08/04/2021 13:21, Jan Beulich wrote:
>> There is a difference in generated code: xzalloc_bytes() forces
>> SMP_CACHE_BYTES alignment. I think we not only don't need this here, but
>> actually don't want it.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> This honestly looks like a backwards step.  In particular, ...
> 
>>
>> --- a/xen/common/kexec.c
>> +++ b/xen/common/kexec.c
>> @@ -463,7 +463,10 @@ static void * alloc_from_crash_heap(cons
>>  /* Allocate a crash note buffer for a newly onlined cpu. */
>>  static int kexec_init_cpu_notes(const unsigned long cpu)
>>  {
>> -    Elf_Note * note = NULL;
>> +    struct elf_notes {
>> +        Elf_Note first;
>> +        unsigned char more[];
>> +    } *notes = NULL;
>>      int ret = 0;
>>      int nr_bytes = 0;
>>  
>> @@ -477,7 +480,8 @@ static int kexec_init_cpu_notes(const un
>>  
>>      /* If we dont care about the position of allocation, malloc. */
>>      if ( low_crashinfo_mode == LOW_CRASHINFO_NONE )
>> -        note = xzalloc_bytes(nr_bytes);
>> +        notes = xzalloc_flex_struct(struct elf_notes, more,
>> +                                    nr_bytes - sizeof(notes->first));
> 
> ... this is far more error prone than the code you replaced, seeing as
> there is now a chance that it can underflow.

You're kidding, aren't you? sizeof_cpu_notes() can't possibly return
a value which would allow underflow here. There would be bigger
issues than the underflow if any such happened, in particular with
any theoretical underflow here simply resulting in the allocation to
fail.

The original code is type-unsafe and requests more alignment than
necessary. Besides objecting to my present way of addressing this,
I would find it rather helpful if you could mention some kind of
alternative you'd consider acceptable. As said in the cover letter,
I think mid-term we ought to do away with xmalloc_bytes(). Hence
some alternative would be needed here anyway.

Jan