Re: [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 22/10/2023 5:52 pm, Julien Grall wrote:
> Hi Andrew,
>
> On 20/10/2023 21:26, Andrew Cooper wrote:
>> When in use, the spew:
>>
>>    (XEN) Assertion '!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE,
>> POISON_BYTE, (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE)' failed at
>> common/xmalloc_tlsf.c:246
>>
>> is unweidly and meaningless to non-Xen developers.  Therefore:
>>
>>   * Switch to IS_ENABLED().  There's no need for full #ifdef-ary.
>>   * Pull memchr_inv() out into the if(), and provide a better error
>> message.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Julien Grall <julien@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Observations from the debugging of:
>>    https://github.com/Dasharo/dasharo-issues/issues/488
>>
>> There's a further bug.  XMEM_POOL_POISON can be enabled in release
>> builds,
>> where the ASSERT() gets dropped.
>>
>> However, upping to a BUG() can't provide any helpful message out to
>> the user.
>>
>> I tried modifying BUG() to take an optional message, but xen/bug.h needs
>> untangling substantially before that will work, and I don't have time
>> right now.
>
> Do we actually care about the registers values? If not, we can either
> use:
>
> printk(...);
> BUG();
>
> Or
>
> panic(...);
>
> This would allow us to use XMEM_POOL_POISON in prod build before BUG()
> can accept string.

We care about the backtrace, so panic() on it's own isn't good enough.

But printk();BUG(); should be good enough so I'll swap to that.

~Andrew