[v3,0/8] Memory tagging support

  This is the third iteration of the patch set to enable memory tagging
in glibc's malloc code.  It mainly addresses the following issues raised
during the previous review:

 - Clean up/add some internal API documentation
 - Remove ROUND_UP_ALLOCATION_SIZE and use some conditionalized code
   in checked_request2size instead.
 - Support MALLOC_CHECK_ in conjuction with _MTAG_ENABLE.

The first two issues are addressed in patch 4 of this series, and the
third in patch 5.  I intend to merge patches 3, 4 and 5 into a single
update to the malloc code before the final commit; I've kept them
separate for now to (hopefully) simplify the review.

The patches have all been rebased against master as of 2020/11/20.

I spent quite a bit of time while working on these looking at whether
the code could be refactored in order to reduce the places where
SIZE_SZ was being added (in different multiples) to various pointers.
I eventually concluded that this wasn't significantly improving the
readability of the code, but one change has survived - I've replaced
usage of 2 * SIZE_SZ with CHUNK_HDR_SZ when it is clear that this is
referring to the header block at the start of a chunk.

I've pushed a copy of this patch series to rearnsha/mte-v3.0, since I
understand some people want to try the patch series as a whole.

R.

Richard Earnshaw (8):
  config: Allow memory tagging to be enabled when configuring glibc
  elf: Add a tunable to control use of tagged memory
  malloc: Basic support for memory tagging in the malloc() family
  malloc: Clean up commentary
  malloc: support MALLOC_CHECK_ in conjunction with _MTAG_ENABLE.
  linux: Add compatibility definitions to sys/prctl.h for MTE
  aarch64: Add sysv specific enabling code for memory tagging
  aarch64: Add aarch64-specific files for memory tagging support

 INSTALL                                       |  14 +
 config.h.in                                   |   3 +
 config.make.in                                |   2 +
 configure                                     |  17 +
 configure.ac                                  |  10 +
 elf/dl-tunables.list                          |   9 +
 malloc/arena.c                                |  59 ++-
 malloc/hooks.c                                |  79 ++--
 malloc/malloc.c                               | 336 ++++++++++++++----
 malloc/malloc.h                               |   7 +
 manual/install.texi                           |  13 +
 manual/tunables.texi                          |  31 ++
 sysdeps/aarch64/Makefile                      |   5 +
 sysdeps/aarch64/__mtag_address_get_tag.S      |  31 ++
 sysdeps/aarch64/__mtag_memset_tag.S           |  46 +++
 sysdeps/aarch64/__mtag_new_tag.S              |  38 ++
 sysdeps/aarch64/__mtag_tag_region.S           |  44 +++
 sysdeps/aarch64/libc-mtag.h                   |  57 +++
 sysdeps/generic/libc-mtag.h                   |  52 +++
 sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h  |   1 +
 sysdeps/unix/sysv/linux/aarch64/bits/mman.h   |   7 +
 .../unix/sysv/linux/aarch64/cpu-features.c    |  28 ++
 .../unix/sysv/linux/aarch64/cpu-features.h    |   1 +
 sysdeps/unix/sysv/linux/sys/prctl.h           |  18 +
 24 files changed, 811 insertions(+), 97 deletions(-)
 create mode 100644 sysdeps/aarch64/__mtag_address_get_tag.S
 create mode 100644 sysdeps/aarch64/__mtag_memset_tag.S
 create mode 100644 sysdeps/aarch64/__mtag_new_tag.S
 create mode 100644 sysdeps/aarch64/__mtag_tag_region.S
 create mode 100644 sysdeps/aarch64/libc-mtag.h
 create mode 100644 sysdeps/generic/libc-mtag.h
  

The 11/23/2020 15:42, Richard Earnshaw wrote:
> This is the third iteration of the patch set to enable memory tagging
> in glibc's malloc code.  It mainly addresses the following issues raised
> during the previous review:
> 
>  - Clean up/add some internal API documentation
>  - Remove ROUND_UP_ALLOCATION_SIZE and use some conditionalized code
>    in checked_request2size instead.
>  - Support MALLOC_CHECK_ in conjuction with _MTAG_ENABLE.
> 
> The first two issues are addressed in patch 4 of this series, and the
> third in patch 5.  I intend to merge patches 3, 4 and 5 into a single
> update to the malloc code before the final commit; I've kept them
> separate for now to (hopefully) simplify the review.
> 
> The patches have all been rebased against master as of 2020/11/20.
> 
> I spent quite a bit of time while working on these looking at whether
> the code could be refactored in order to reduce the places where
> SIZE_SZ was being added (in different multiples) to various pointers.
> I eventually concluded that this wasn't significantly improving the
> readability of the code, but one change has survived - I've replaced
> usage of 2 * SIZE_SZ with CHUNK_HDR_SZ when it is clear that this is
> referring to the header block at the start of a chunk.
> 
> I've pushed a copy of this patch series to rearnsha/mte-v3.0, since I
> understand some people want to try the patch series as a whole.

cross testing with --enable-memory-tagging and
_MTAG_ENABLE=3 in qemu with arm64 for-next/mte
linux branch, the new failures i see:

FAIL: malloc/tst-malloc-backtrace
FAIL: malloc/tst-mallocstate
FAIL: malloc/tst-safe-linking
FAIL: malloc/tst-tcfree1
FAIL: malloc/tst-tcfree2
FAIL: malloc/tst-tcfree3
	all these tests have use after free
FAIL: posix/tst-mmap
	unaligned mmap over malloced memory now
	fails with ENOMEM instead of EINVAL.

i think these are all reasonable failures for mte.
(the malloc/ ones can be unsupported, and the mmap
test can use mmaped memory instead of malloced)
  

On 11/23/20 9:12 PM, Richard Earnshaw via Libc-alpha wrote:
> This is the third iteration of the patch set to enable memory tagging
> in glibc's malloc code.  It mainly addresses the following issues raised
> during the previous review:
> 
>   - Clean up/add some internal API documentation
>   - Remove ROUND_UP_ALLOCATION_SIZE and use some conditionalized code
>     in checked_request2size instead.
>   - Support MALLOC_CHECK_ in conjuction with _MTAG_ENABLE.

I tried to review but the patchset doesn't build on x86_64 because 
__libc_mtag_tag_region() and __libc_mtag_address_get_tag() are macros:

arena.c: In function ‘ptmalloc_init’:
arena.c:342:22: error: ‘__libc_mtag_tag_region’ undeclared (first use in 
this function); did you mean ‘__default_tag_region’?
   342 |       __tag_region = __libc_mtag_tag_region;
       |                      ^~~~~~~~~~~~~~~~~~~~~~
       |                      __default_tag_region

I'll review 1, 2. 6 and 7 (and the overall design) anyway since I 
suspect you'll only end up changing 3 and 8 to fix this failure. 
Hopefully that will help you make forward progress.

> The first two issues are addressed in patch 4 of this series, and the
> third in patch 5.  I intend to merge patches 3, 4 and 5 into a single
> update to the malloc code before the final commit; I've kept them
> separate for now to (hopefully) simplify the review.

Please merge them in first; it is actually confusing to review because I 
have to then refer across three patches to see what's fixed in 3/8.

Besides, having the version of the patch in review being the same as the 
one that's committed helps since we can then auto-close patchwork patches.

> I spent quite a bit of time while working on these looking at whether
> the code could be refactored in order to reduce the places where
> SIZE_SZ was being added (in different multiples) to various pointers.
> I eventually concluded that this wasn't significantly improving the
> readability of the code, but one change has survived - I've replaced
> usage of 2 * SIZE_SZ with CHUNK_HDR_SZ when it is clear that this is
> referring to the header block at the start of a chunk.

I skimmed through the patches and I think this is a useful change, thanks.

Siddhesh
  

On Mon, Nov 23, 2020 at 7:46 AM Richard Earnshaw via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> This is the third iteration of the patch set to enable memory tagging
> in glibc's malloc code.  It mainly addresses the following issues raised
> during the previous review:
>
>  - Clean up/add some internal API documentation
>  - Remove ROUND_UP_ALLOCATION_SIZE and use some conditionalized code
>    in checked_request2size instead.
>  - Support MALLOC_CHECK_ in conjuction with _MTAG_ENABLE.
>
> The first two issues are addressed in patch 4 of this series, and the
> third in patch 5.  I intend to merge patches 3, 4 and 5 into a single
> update to the malloc code before the final commit; I've kept them
> separate for now to (hopefully) simplify the review.
>
> The patches have all been rebased against master as of 2020/11/20.
>
> I spent quite a bit of time while working on these looking at whether
> the code could be refactored in order to reduce the places where
> SIZE_SZ was being added (in different multiples) to various pointers.
> I eventually concluded that this wasn't significantly improving the
> readability of the code, but one change has survived - I've replaced
> usage of 2 * SIZE_SZ with CHUNK_HDR_SZ when it is clear that this is
> referring to the header block at the start of a chunk.
>
> I've pushed a copy of this patch series to rearnsha/mte-v3.0, since I
> understand some people want to try the patch series as a whole.
>
> R.
>
> Richard Earnshaw (8):
>   config: Allow memory tagging to be enabled when configuring glibc
>   elf: Add a tunable to control use of tagged memory
>   malloc: Basic support for memory tagging in the malloc() family
>   malloc: Clean up commentary
>   malloc: support MALLOC_CHECK_ in conjunction with _MTAG_ENABLE.
>   linux: Add compatibility definitions to sys/prctl.h for MTE
>   aarch64: Add sysv specific enabling code for memory tagging
>   aarch64: Add aarch64-specific files for memory tagging support
>

Please add some tests to verify use-after-free and underflow/overflow detections
which aren't detected by the current malloc implementation.

Also should glibc provide a memory tag interface for other memory allocators?

H.J.
  

On 25/11/2020 14:49, Siddhesh Poyarekar wrote:
> On 11/23/20 9:12 PM, Richard Earnshaw via Libc-alpha wrote:
>> This is the third iteration of the patch set to enable memory tagging
>> in glibc's malloc code.  It mainly addresses the following issues raised
>> during the previous review:
>>
>>   - Clean up/add some internal API documentation
>>   - Remove ROUND_UP_ALLOCATION_SIZE and use some conditionalized code
>>     in checked_request2size instead.
>>   - Support MALLOC_CHECK_ in conjuction with _MTAG_ENABLE.
> 
> I tried to review but the patchset doesn't build on x86_64 because
> __libc_mtag_tag_region() and __libc_mtag_address_get_tag() are macros:
> 
> arena.c: In function ‘ptmalloc_init’:
> arena.c:342:22: error: ‘__libc_mtag_tag_region’ undeclared (first use in
> this function); did you mean ‘__default_tag_region’?
>   342 |       __tag_region = __libc_mtag_tag_region;
>       |                      ^~~~~~~~~~~~~~~~~~~~~~
>       |                      __default_tag_region
> 

Ah, you're configuring x86 with this feature enabled.  Sorry, I didn't
think to test that :(

I think the right solution is to make the configure step detect that
this won't work and ignore the config option in that case.


> I'll review 1, 2. 6 and 7 (and the overall design) anyway since I
> suspect you'll only end up changing 3 and 8 to fix this failure.
> Hopefully that will help you make forward progress.
> 
>> The first two issues are addressed in patch 4 of this series, and the
>> third in patch 5.  I intend to merge patches 3, 4 and 5 into a single
>> update to the malloc code before the final commit; I've kept them
>> separate for now to (hopefully) simplify the review.
> 
> Please merge them in first; it is actually confusing to review because I
> have to then refer across three patches to see what's fixed in 3/8.
> 
> Besides, having the version of the patch in review being the same as the
> one that's committed helps since we can then auto-close patchwork patches.

OK.

> 
>> I spent quite a bit of time while working on these looking at whether
>> the code could be refactored in order to reduce the places where
>> SIZE_SZ was being added (in different multiples) to various pointers.
>> I eventually concluded that this wasn't significantly improving the
>> readability of the code, but one change has survived - I've replaced
>> usage of 2 * SIZE_SZ with CHUNK_HDR_SZ when it is clear that this is
>> referring to the header block at the start of a chunk.
> 
> I skimmed through the patches and I think this is a useful change, thanks.
> 
> Siddhesh

R.
  

On 11/25/20 9:18 PM, Richard Earnshaw wrote:
> Ah, you're configuring x86 with this feature enabled.  Sorry, I didn't
> think to test that :(
> 
> I think the right solution is to make the configure step detect that
> this won't work and ignore the config option in that case.
> 

If you take that route then I think the earlier description of 
--enable-memory-tagging in configure is more precise and you'd need to 
adjust the description in INSTALL ;)

Siddhesh