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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2023 22: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.

Just to mention it (no objection to the changes done): While I agree with
unwieldy (and I'd add "of limited use"), an assertion message not necessarily
being meaningful to non-Xen developers is imo not really a criteria - that's
to be expected in various cases.

>  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.

Thing is - this "better" error message now ends up being less specific, and
hence could mean also other kinds of corruption.

> 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.

Just to mention it - this limits usefulness, but doesn't make the option
entirely useless. The poisoning itself also allows certain cases of use-
after-free to be caught. The assertion really is "only" about write-after-
free. (Maybe the improved error message could indicate so?)

> 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.

I agree with Julien's suggestion of using panic() in the meantime, as a
possible alternative. Question though is whether it's better to halt the
system right away, as opposed to e.g. permitting orderly shutdown to cover
the case where the corruption ends up not being "deadly".

Jan