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

[Julien Grall <julien@xxxxxxx>]

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.
> ---
>   xen/common/xmalloc_tlsf.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
> index 349b31cb4cc1..13305cd87c2f 100644
> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -249,11 +249,11 @@ static inline void EXTRACT_BLOCK(struct bhdr *b, struct 
> xmem_pool *p, int fl,
>       }
>       b->ptr.free_ptr = (struct free_ptr) {NULL, NULL};
>   
> -#ifdef CONFIG_XMEM_POOL_POISON
> -    if ( (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE )
> -        ASSERT(!memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, POISON_BYTE,
> -                           (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE));
> -#endif /* CONFIG_XMEM_POOL_POISON */
> +    if ( IS_ENABLED(CONFIG_XMEM_POOL_POISON) &&
> +         (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE &&
> +         memchr_inv(b->ptr.buffer + MIN_BLOCK_SIZE, POISON_BYTE,
> +                    (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE) )
> +        ASSERT(!"XMEM Pool corruption found");
>   }
>   
>   /**
> @@ -261,11 +261,10 @@ static inline void EXTRACT_BLOCK(struct bhdr *b, struct 
> xmem_pool *p, int fl,
>    */
>   static inline void INSERT_BLOCK(struct bhdr *b, struct xmem_pool *p, int fl, 
> int sl)
>   {
> -#ifdef CONFIG_XMEM_POOL_POISON
> -    if ( (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE )
> +    if ( IS_ENABLED(CONFIG_XMEM_POOL_POISON) &&
> +         (b->size & BLOCK_SIZE_MASK) > MIN_BLOCK_SIZE )
>           memset(b->ptr.buffer + MIN_BLOCK_SIZE, POISON_BYTE,
>                  (b->size & BLOCK_SIZE_MASK) - MIN_BLOCK_SIZE);
> -#endif /* CONFIG_XMEM_POOL_POISON */
>   
>       b->ptr.free_ptr = (struct free_ptr) {NULL, p->matrix[fl][sl]};
>       if ( p->matrix[fl][sl] )

Cheers,

--
Julien Grall