* [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]
@ 2021-03-05 11:39 Wilco Dijkstra
0 siblings, 0 replies; 6+ messages in thread
From: Wilco Dijkstra @ 2021-03-05 11:39 UTC (permalink / raw)
To: dj, Szabolcs Nagy; +Cc: 'GNU C Library'
Hi DJ,
>> + size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
>
> I think this is semantically wrong, because the chunk size
> (mptr->mchunk_size) does not include the mchunk_prev_size that's
> accounted for in CHUNK_HDR_SZ. I suspect the problem is that
> CHUNK_AVAILABLE_SIZE is wrong, in that it adds SIZE_SZ in the non-tagged
> case, and shouldn't, or that it's defined (or named) wrong.
CHUNK_AVAILABLE_SIZE is badly named, but it is the total chunksize plus
the extra SIZE_SZ word in the next non-mmap chunk. With memory tagging
you can't use the extra word in the next chunk either. Note that most existing
uses of it explicitly subtract CHUNK_HDR_SZ to get the user size of the chunk.
What would be better is to add CHUNK_USABLE_SIZE or CHUNK_DATA_SIZE which
returns size you can actually use for data and remove CHUNK_AVAILABLE_SIZE.
Cheers,
Wilco
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 00/16] memory tagging improvements
@ 2021-03-04 16:30 Szabolcs Nagy
2021-03-04 16:30 ` [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468] Szabolcs Nagy
0 siblings, 1 reply; 6+ messages in thread
From: Szabolcs Nagy @ 2021-03-04 16:30 UTC (permalink / raw)
To: libc-alpha, Richard.Earnshaw, DJ Delorie
This set tries to reduce the overhead of memory tagging support: when
glibc is built with --enable-memory-tagging there are runtime checks
that affect performance even if heap tagging is not enabled at runtime.
I refactored the code to reduce internal abstractions and ifdefs. And
a number of patches optimize heap tagging on aarch64 with MTE.
Also available in the nsz/mtag branch.
Issues found and fixed:
- realloc failed to untag the old memory in some cases.
- incorrect but useless macro definition in public sys/prctl.h.
- incorrect but harmless internal MTAG_MMAP_FLAGS declaration.
Issues found but not fixed:
- large granule is not supported: both chunk header and user allocation
must be tag granule aligned, but the code assumes that the chunk
header is 2*SIZE_SZ so larger granule is not supported.
- request2size and checked_request2size are not consistent which looks
wrong (likely works in practice other than some sizes don't end up in
fastbin or tcache that one might expected to, but i think this can
break easily). Fixing it is not obvious because of assumptions about
the chunk layout that is no longer true with mtag enabled.
Other possible optimizations:
- ifunc malloc apis
good: near zero overhead for runtime dispatch.
bad: malloc has to be built twice or larger refactoring is needed.
- unconditional top byte zero
internal data is tagged 0, only user allocation is tagged non-zero
(we rely on this e.g. where MADV_DONTNEED is used: then the kernel
zeros the tags so with non-zero tags internal pointers would not be
able to access recycled memory), so tag_at(p) for internal data
could be unconditional top byte clear.
good: faster than the branch in tag_at, may improve security (?).
bad: design change.
- unconditional layout
padding up to tag granule can be done unconditionally
good: no runtime check, same layout with and without heap tagging.
bad: wasting space with small allocations, design change.
This patchset tried to avoid intrusive changes that affect behaviour.
Benchmarked using bench-malloc-thread glibc ubenchmark on Neoverse N1.
For size=16 the runtime check overhead is
~ 32% before the patch set
~ 16% after the patch set.
On a practical workload or for large sizes the overhead should be tiny.
Szabolcs Nagy (16):
malloc: Fix a realloc crash with heap tagging [BZ 27468]
Remove PR_TAGGED_ADDR_ENABLE from sys/prctl.h
malloc: Move MTAG_MMAP_FLAGS definition
malloc: Simplify __mtag_tag_new_usable
malloc: Avoid taggig mmaped memory on free
malloc: Ensure the generic mtag hooks are not used
malloc: Refactor TAG_ macros to avoid indirection
malloc: Use global flag instead of function pointer dispatch for mtag
malloc: Only support zeroing and not arbitrary memset with mtag
malloc: Change calloc when tagging is disabled
malloc: Use branches instead of mtag_granule_mask
malloc: Use mtag_enabled instead of USE_MTAG
aarch64: inline __libc_mtag_address_get_tag
aarch64: inline __libc_mtag_new_tag
aarch64: Optimize __libc_mtag_tag_region
aarch64: Optimize __libc_mtag_tag_zero_region
include/malloc.h | 7 -
malloc/arena.c | 45 +-----
malloc/hooks.c | 20 ++-
malloc/malloc.c | 177 ++++++++++++-----------
sysdeps/aarch64/Makefile | 4 +-
sysdeps/aarch64/__mtag_address_get_tag.S | 32 ----
sysdeps/aarch64/__mtag_memset_tag.S | 53 -------
sysdeps/aarch64/__mtag_new_tag.S | 37 -----
sysdeps/aarch64/__mtag_tag_region.S | 98 ++++++++++---
sysdeps/aarch64/__mtag_tag_zero_region.S | 113 +++++++++++++++
sysdeps/aarch64/libc-mtag.h | 32 ++--
sysdeps/generic/libc-mtag.h | 43 ++++--
sysdeps/unix/sysv/linux/sys/prctl.h | 4 -
13 files changed, 353 insertions(+), 312 deletions(-)
delete mode 100644 sysdeps/aarch64/__mtag_address_get_tag.S
delete mode 100644 sysdeps/aarch64/__mtag_memset_tag.S
delete mode 100644 sysdeps/aarch64/__mtag_new_tag.S
create mode 100644 sysdeps/aarch64/__mtag_tag_zero_region.S
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]
2021-03-04 16:30 [PATCH 00/16] memory tagging improvements Szabolcs Nagy
@ 2021-03-04 16:30 ` Szabolcs Nagy
2021-03-05 0:15 ` DJ Delorie
2021-03-05 20:51 ` DJ Delorie
0 siblings, 2 replies; 6+ messages in thread
From: Szabolcs Nagy @ 2021-03-04 16:30 UTC (permalink / raw)
To: libc-alpha, Richard.Earnshaw, DJ Delorie
_int_free must be called with a chunk that has its tag reset. This was
missing in a rare case that could crash when heap tagging is enabled:
when in a multi-threaded process the current arena runs out of memory
during realloc, but another arena still has space to finish the realloc
then _int_free was called without clearing the user allocation tags.
And another _int_free call site in realloc used the wrong size for the
tag clearing: the chunk header of the next chunk was also cleared which
in practice is probably not a problem, but logically that belongs to a
different chunk so it may cause trouble.
Fixes bug 27468.
---
malloc/malloc.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 1f4bbd8edf..10ea6aa441 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3446,7 +3446,9 @@ __libc_realloc (void *oldmem, size_t bytes)
newp = __libc_malloc (bytes);
if (newp != NULL)
{
- memcpy (newp, oldmem, oldsize - SIZE_SZ);
+ size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
+ memcpy (newp, oldmem, sz);
+ (void) TAG_REGION (chunk2rawmem (oldp), sz);
_int_free (ar_ptr, oldp, 0);
}
}
@@ -4850,10 +4852,10 @@ _int_realloc(mstate av, mchunkptr oldp, INTERNAL_SIZE_T oldsize,
else
{
void *oldmem = chunk2mem (oldp);
+ size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
newmem = TAG_NEW_USABLE (newmem);
- memcpy (newmem, oldmem,
- CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ);
- (void) TAG_REGION (chunk2rawmem (oldp), oldsize);
+ memcpy (newmem, oldmem, sz);
+ (void) TAG_REGION (chunk2rawmem (oldp), sz);
_int_free (av, oldp, 1);
check_inuse_chunk (av, newp);
return chunk2mem (newp);
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]
2021-03-04 16:30 ` [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468] Szabolcs Nagy
@ 2021-03-05 0:15 ` DJ Delorie
2021-03-05 12:01 ` Szabolcs Nagy
2021-03-05 20:51 ` DJ Delorie
1 sibling, 1 reply; 6+ messages in thread
From: DJ Delorie @ 2021-03-05 0:15 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Richard.Earnshaw
Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 1f4bbd8edf..10ea6aa441 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3446,7 +3446,9 @@ __libc_realloc (void *oldmem, size_t bytes)
> newp = __libc_malloc (bytes);
> if (newp != NULL)
> {
> - memcpy (newp, oldmem, oldsize - SIZE_SZ);
> + size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
I think this is semantically wrong, because the chunk size
(mptr->mchunk_size) does not include the mchunk_prev_size that's
accounted for in CHUNK_HDR_SZ. I suspect the problem is that
CHUNK_AVAILABLE_SIZE is wrong, in that it adds SIZE_SZ in the non-tagged
case, and shouldn't, or that it's defined (or named) wrong.
chunksize(p) is the difference between this chunk and the corresponding
address in the next chunk. i.e. it's prev_ptr to prev_ptr, or
user-bytes to user-bytes.
A "chunk pointer" does NOT point to the beginning of the chunk, but to
the prev_ptr in the PREVIOUS chunk. So CHUNK_HDR_SZ is the offset from
a chunk pointer to the user data, but it is NOT the difference between
the chunk size and the user data size. Using CHUNK_HDR_SZ in any
user-data-size computations is suspect logic.
That the resulting value happens to be correct is irrelevent here,
although I suspect it will be off by a word when tagging is enabled, and
not memcpy enough data, if the prev_ptr word is still part of the "user
data" when tagging is enabled.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]
2021-03-05 0:15 ` DJ Delorie
@ 2021-03-05 12:01 ` Szabolcs Nagy
2021-03-05 18:42 ` DJ Delorie
0 siblings, 1 reply; 6+ messages in thread
From: Szabolcs Nagy @ 2021-03-05 12:01 UTC (permalink / raw)
To: DJ Delorie; +Cc: libc-alpha, Richard.Earnshaw
The 03/04/2021 19:15, DJ Delorie wrote:
> Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> > diff --git a/malloc/malloc.c b/malloc/malloc.c
> > index 1f4bbd8edf..10ea6aa441 100644
> > --- a/malloc/malloc.c
> > +++ b/malloc/malloc.c
> > @@ -3446,7 +3446,9 @@ __libc_realloc (void *oldmem, size_t bytes)
> > newp = __libc_malloc (bytes);
> > if (newp != NULL)
> > {
> > - memcpy (newp, oldmem, oldsize - SIZE_SZ);
>
> > + size_t sz = CHUNK_AVAILABLE_SIZE (oldp) - CHUNK_HDR_SZ;
>
> I think this is semantically wrong, because the chunk size
> (mptr->mchunk_size) does not include the mchunk_prev_size that's
> accounted for in CHUNK_HDR_SZ. I suspect the problem is that
> CHUNK_AVAILABLE_SIZE is wrong, in that it adds SIZE_SZ in the non-tagged
> case, and shouldn't, or that it's defined (or named) wrong.
>
> chunksize(p) is the difference between this chunk and the corresponding
> address in the next chunk. i.e. it's prev_ptr to prev_ptr, or
> user-bytes to user-bytes.
>
> A "chunk pointer" does NOT point to the beginning of the chunk, but to
> the prev_ptr in the PREVIOUS chunk. So CHUNK_HDR_SZ is the offset from
> a chunk pointer to the user data, but it is NOT the difference between
> the chunk size and the user data size. Using CHUNK_HDR_SZ in any
> user-data-size computations is suspect logic.
>
> That the resulting value happens to be correct is irrelevent here,
> although I suspect it will be off by a word when tagging is enabled, and
> not memcpy enough data, if the prev_ptr word is still part of the "user
> data" when tagging is enabled.
it seems CHUNK_AVAILABLE_SIZE is defined as
(memory owned by the user) + CHUNK_HDR_SZ
and it should work on mmaped and normal chunks with or without
tagging. so by this definition i think the change is right, but
the CHUNK_AVAILABLE_SIZE may not have the most useful definition.
i can change this macro to be more meaningful, e.g.:
CHUNK_USER_SIZE(chunk): memory owned by the user in chunk.
i.e. the interval that user code may access in chunk p is
[ chunk2mem(p), chunk2mem(p) + CHUNK_USER_SIZE(p) )
with tagging on aarch64 (granule = 2*size_t) this does not include
the prev_ptr word at the end.
I can refactor the code using this macro, or let me know if you
have a different preference (and if it should be backported with
this bug fix or have it as a follow up change on master).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]
2021-03-05 12:01 ` Szabolcs Nagy
@ 2021-03-05 18:42 ` DJ Delorie
0 siblings, 0 replies; 6+ messages in thread
From: DJ Delorie @ 2021-03-05 18:42 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Richard.Earnshaw
Szabolcs Nagy <szabolcs.nagy@arm.com> writes:
> with tagging on aarch64 (granule = 2*size_t) this does not include
> the prev_ptr word at the end.
So chunksize() includes the overhead to align tagging, but
CHUNK_USER_SIZE() would not?
It's unfortunate that we have such a good name for the chunk, but no
good name for "that portion of the chunk that the application can use".
USER_SIZE vs rawmem vs mem vs whatever.
> I can refactor the code using this macro, or let me know if you have a
> different preference (and if it should be backported with this bug fix
> or have it as a follow up change on master).
An agreement to fix it later is sufficient, let's not mess up the
existing patch set.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468]
2021-03-04 16:30 ` [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468] Szabolcs Nagy
2021-03-05 0:15 ` DJ Delorie
@ 2021-03-05 20:51 ` DJ Delorie
1 sibling, 0 replies; 6+ messages in thread
From: DJ Delorie @ 2021-03-05 20:51 UTC (permalink / raw)
To: Szabolcs Nagy; +Cc: libc-alpha, Richard.Earnshaw
LGTM given our conversation about the unfortunate name of
"CHUNK_AVAILABLE_SIZE" (to be dealt with later).
Reviewed-by: DJ Delorie <dj@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-05 20:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 11:39 [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468] Wilco Dijkstra
-- strict thread matches above, loose matches on Subject: below --
2021-03-04 16:30 [PATCH 00/16] memory tagging improvements Szabolcs Nagy
2021-03-04 16:30 ` [PATCH 01/16] malloc: Fix a realloc crash with heap tagging [BZ 27468] Szabolcs Nagy
2021-03-05 0:15 ` DJ Delorie
2021-03-05 12:01 ` Szabolcs Nagy
2021-03-05 18:42 ` DJ Delorie
2021-03-05 20:51 ` DJ Delorie
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).