* [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements
@ 2023-10-20 20:26 Andrew Cooper
2023-10-22 16:52 ` Julien Grall
2023-10-23 8:04 ` Jan Beulich
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Cooper @ 2023-10-20 20:26 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, George Dunlap, Jan Beulich, Stefano Stabellini,
Wei Liu, Julien Grall, Roger Pau Monné
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@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
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.
---
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] )
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements
2023-10-20 20:26 [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements Andrew Cooper
@ 2023-10-22 16:52 ` Julien Grall
2023-10-23 9:51 ` Andrew Cooper
2023-10-23 8:04 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Julien Grall @ 2023-10-22 16:52 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
Cc: George Dunlap, Jan Beulich, Stefano Stabellini, Wei Liu,
Roger Pau Monné
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@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
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements
2023-10-22 16:52 ` Julien Grall
@ 2023-10-23 9:51 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2023-10-23 9:51 UTC (permalink / raw)
To: Julien Grall, Xen-devel
Cc: George Dunlap, Jan Beulich, Stefano Stabellini, Wei Liu,
Roger Pau Monné
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@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
>>
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements
2023-10-20 20:26 [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements Andrew Cooper
2023-10-22 16:52 ` Julien Grall
@ 2023-10-23 8:04 ` Jan Beulich
2023-10-23 8:28 ` Roger Pau Monné
1 sibling, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2023-10-23 8:04 UTC (permalink / raw)
To: Andrew Cooper
Cc: George Dunlap, Stefano Stabellini, Wei Liu, Julien Grall,
Roger Pau Monné, Xen-devel
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@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
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements
2023-10-23 8:04 ` Jan Beulich
@ 2023-10-23 8:28 ` Roger Pau Monné
2023-10-23 8:41 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2023-10-23 8:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Julien Grall, Xen-devel
On Mon, Oct 23, 2023 at 10:04:21AM +0200, Jan Beulich wrote:
> On 20.10.2023 22:26, Andrew Cooper wrote:
> > 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.
We might care about the stack trace, so would be helpful to print it,
maybe WARN + panic?
> 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".
Hm, won't this be risky, as we could then possibly corrupt data on disk
for example?
Thanks, Roger.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements
2023-10-23 8:28 ` Roger Pau Monné
@ 2023-10-23 8:41 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2023-10-23 8:41 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Andrew Cooper, George Dunlap, Stefano Stabellini, Wei Liu,
Julien Grall, Xen-devel
On 23.10.2023 10:28, Roger Pau Monné wrote:
> On Mon, Oct 23, 2023 at 10:04:21AM +0200, Jan Beulich wrote:
>> On 20.10.2023 22:26, Andrew Cooper wrote:
>>> 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.
>
> We might care about the stack trace, so would be helpful to print it,
> maybe WARN + panic?
Perhaps, yes.
>> 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".
>
> Hm, won't this be risky, as we could then possibly corrupt data on disk
> for example?
It is risky - you never really know which of the two options is better.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-23 9:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-20 20:26 [PATCH for-4.19] xen/xmalloc: XMEM_POOL_POISON improvements Andrew Cooper
2023-10-22 16:52 ` Julien Grall
2023-10-23 9:51 ` Andrew Cooper
2023-10-23 8:04 ` Jan Beulich
2023-10-23 8:28 ` Roger Pau Monné
2023-10-23 8:41 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.