Message ID | 20201123154236.25809-1-rearnsha@arm.com |
---|---|
Headers | show |
Series | Memory tagging support | expand |
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