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(-)
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] )
--
2.30.2
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, -- Julien Grall
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
© 2016 - 2024 Red Hat, Inc.