diff mbox series

xen/xmalloc: XMEM_POOL_POISON improvements

Message ID 20231220214716.2510402-1-andrew.cooper3@citrix.com (mailing list archive)
State New, archived
Headers show
Series xen/xmalloc: XMEM_POOL_POISON improvements | expand

Commit Message

Andrew Cooper Dec. 20, 2023, 9:47 p.m. UTC
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 likely 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 an error message which
   clearly states that corruption has been found.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

Observations from the debugging of:
  https://github.com/Dasharo/dasharo-issues/issues/488

v2:
 * Switch to printk()+BUG()
---
 xen/common/xmalloc_tlsf.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Julien Grall Dec. 20, 2023, 9:51 p.m. UTC | #1
Hi Andrew,

On 20/12/2023 21:47, 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 likely 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 an error message which
>     clearly states that corruption has been found.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

With one remark:

Reviewed-by: Julien Grall <jgrall@amazon.com>

> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> Observations from the debugging of:
>    https://github.com/Dasharo/dasharo-issues/issues/488
> 
> v2:
>   * Switch to printk()+BUG()

I would suggest to add a sentence about the switch to printk() + BUG() 
in the commit message. Maybe:

"CONFIG_XMEM_POOL_POISON can be enabled even on production build. So we 
should switch from ASSERT() to BUG()."

Cheers,
Andrew Cooper Dec. 21, 2023, 10:28 a.m. UTC | #2
On 20/12/2023 9:51 pm, Julien Grall wrote:
> Hi Andrew,
>
> On 20/12/2023 21:47, 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 likely 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 an error message
>> which
>>     clearly states that corruption has been found.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> With one remark:
>
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks.  Will fix.

~Andrew
diff mbox series

Patch

diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c
index 349b31cb4cc1..5e55fc463e7d 100644
--- a/xen/common/xmalloc_tlsf.c
+++ b/xen/common/xmalloc_tlsf.c
@@ -249,11 +249,14 @@  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) )
+    {
+        printk(XENLOG_ERR "XMEM Pool corruption found");
+        BUG();
+    }
 }
 
 /**
@@ -261,11 +264,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] )