* [PATCH 0/3] Enable ASan in GHA
@ 2022-04-15 17:31 David Marchand
2022-04-15 17:31 ` [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem David Marchand
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: David Marchand @ 2022-04-15 17:31 UTC (permalink / raw)
To: dev; +Cc: john.mcnamara, dmitry.kozliuk
This series fixes two issues that were revealed while running unit tests
with ASan in GHA.
There are still some outstanding issues for which bz have been created.
The last patch enables ASan in GHA, skipping tests which have issues.
--
David Marchand
David Marchand (3):
test/mem: disable ASan when accessing unallocated mem
mem: fix ASan shadow for remapped memory segments
ci: build some job with ASan
.ci/linux-build.sh | 8 ++
.github/workflows/build.yml | 3 +-
app/test/meson.build | 208 ++++++++++++++++++-----------------
app/test/test_memory.c | 5 +
lib/eal/common/malloc_elem.h | 12 +-
lib/eal/common/malloc_heap.c | 12 +-
lib/eal/include/rte_common.h | 13 +++
7 files changed, 152 insertions(+), 109 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem
2022-04-15 17:31 [PATCH 0/3] Enable ASan in GHA David Marchand
@ 2022-04-15 17:31 ` David Marchand
2022-04-20 14:48 ` Burakov, Anatoly
2022-04-15 17:31 ` [PATCH 2/3] mem: fix ASan shadow for remapped memory segments David Marchand
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: David Marchand @ 2022-04-15 17:31 UTC (permalink / raw)
To: dev
Cc: john.mcnamara, dmitry.kozliuk, stable, Anatoly Burakov,
Xueqin Lin, Zhihong Peng
As described in bugzilla, ASan reports accesses to all memory segment as
invalid, since those parts have not been allocated.
Move __rte_no_asan to rte_common.h and disable ASan on a part of the test.
Bugzilla ID: 880
Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
app/test/test_memory.c | 5 +++++
lib/eal/common/malloc_elem.h | 10 ++--------
lib/eal/include/rte_common.h | 13 +++++++++++++
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/app/test/test_memory.c b/app/test/test_memory.c
index 140ac3f3cf..440e5ef838 100644
--- a/app/test/test_memory.c
+++ b/app/test/test_memory.c
@@ -25,6 +25,11 @@
* - Try to read all memory; it should not segfault.
*/
+/*
+ * ASan complains about accessing unallocated memory.
+ * See: https://bugs.dpdk.org/show_bug.cgi?id=880
+ */
+__rte_no_asan
static int
check_mem(const struct rte_memseg_list *msl __rte_unused,
const struct rte_memseg *ms, void *arg __rte_unused)
diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
index f2aa98821b..228f178418 100644
--- a/lib/eal/common/malloc_elem.h
+++ b/lib/eal/common/malloc_elem.h
@@ -7,6 +7,8 @@
#include <stdbool.h>
+#include <rte_common.h>
+
#define MIN_DATA_SIZE (RTE_CACHE_LINE_SIZE)
/* dummy definition of struct so we can use pointers to it in malloc_elem struct */
@@ -131,12 +133,6 @@ malloc_elem_cookies_ok(const struct malloc_elem *elem)
#define ASAN_MEM_TO_SHADOW(mem) \
RTE_PTR_ADD(ASAN_MEM_SHIFT(mem), ASAN_SHADOW_OFFSET)
-#if defined(__clang__)
-#define __rte_no_asan __attribute__((no_sanitize("address", "hwaddress")))
-#else
-#define __rte_no_asan __attribute__((no_sanitize_address))
-#endif
-
__rte_no_asan
static inline void
asan_set_shadow(void *addr, char val)
@@ -276,8 +272,6 @@ old_malloc_size(struct malloc_elem *elem)
#else /* !RTE_MALLOC_ASAN */
-#define __rte_no_asan
-
static inline void
asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 67587025ab..d56a7570c0 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -267,6 +267,19 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
*/
#define __rte_cold __attribute__((cold))
+/**
+ * Disable AddressSanitizer on some code
+ */
+#ifdef RTE_MALLOC_ASAN
+#ifdef RTE_CC_CLANG
+#define __rte_no_asan __attribute__((no_sanitize("address", "hwaddress")))
+#else
+#define __rte_no_asan __attribute__((no_sanitize_address))
+#endif
+#else /* ! RTE_MALLOC_ASAN */
+#define __rte_no_asan
+#endif
+
/*********** Macros for pointer arithmetic ********/
/**
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-15 17:31 [PATCH 0/3] Enable ASan in GHA David Marchand
2022-04-15 17:31 ` [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem David Marchand
@ 2022-04-15 17:31 ` David Marchand
2022-04-20 14:47 ` Burakov, Anatoly
2022-04-15 17:31 ` [PATCH 3/3] ci: build some job with ASan David Marchand
2022-05-05 9:29 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
3 siblings, 1 reply; 18+ messages in thread
From: David Marchand @ 2022-04-15 17:31 UTC (permalink / raw)
To: dev
Cc: john.mcnamara, dmitry.kozliuk, stable, Anatoly Burakov,
Xueqin Lin, Zhihong Peng
When releasing some memory, the allocator can choose to return some
pages to the OS. At the same time, this memory was poisoned in ASAn
shadow. Doing the latter made it impossible to remap this same page
later.
On the other hand, without this poison, the OS would pagefault in any
case for this page.
Remove the poisoning for unmapped pages.
Bugzilla ID: 994
Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
lib/eal/common/malloc_elem.h | 4 ++++
lib/eal/common/malloc_heap.c | 12 +++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
index 228f178418..b859003722 100644
--- a/lib/eal/common/malloc_elem.h
+++ b/lib/eal/common/malloc_elem.h
@@ -272,6 +272,10 @@ old_malloc_size(struct malloc_elem *elem)
#else /* !RTE_MALLOC_ASAN */
+static inline void
+asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
+ uint32_t val __rte_unused) { }
+
static inline void
asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index 6c572b6f2c..5913d9f862 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -860,6 +860,7 @@ malloc_heap_free(struct malloc_elem *elem)
size_t len, aligned_len, page_sz;
struct rte_memseg_list *msl;
unsigned int i, n_segs, before_space, after_space;
+ bool unmapped_pages = false;
int ret;
const struct internal_config *internal_conf =
eal_get_internal_configuration();
@@ -999,6 +1000,13 @@ malloc_heap_free(struct malloc_elem *elem)
/* don't care if any of this fails */
malloc_heap_free_pages(aligned_start, aligned_len);
+ /*
+ * Clear any poisoning in ASan for the associated pages so that
+ * next time EAL maps those pages, the allocator can access
+ * them.
+ */
+ asan_set_zone(aligned_start, aligned_len, 0x00);
+ unmapped_pages = true;
request_sync();
} else {
@@ -1032,7 +1040,9 @@ malloc_heap_free(struct malloc_elem *elem)
rte_mcfg_mem_write_unlock();
free_unlock:
- asan_set_freezone(asan_ptr, asan_data_len);
+ /* Poison memory range if belonging to some still mapped pages. */
+ if (!unmapped_pages)
+ asan_set_freezone(asan_ptr, asan_data_len);
rte_spinlock_unlock(&(heap->lock));
return ret;
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] ci: build some job with ASan
2022-04-15 17:31 [PATCH 0/3] Enable ASan in GHA David Marchand
2022-04-15 17:31 ` [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem David Marchand
2022-04-15 17:31 ` [PATCH 2/3] mem: fix ASan shadow for remapped memory segments David Marchand
@ 2022-04-15 17:31 ` David Marchand
2022-05-05 9:29 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
3 siblings, 0 replies; 18+ messages in thread
From: David Marchand @ 2022-04-15 17:31 UTC (permalink / raw)
To: dev; +Cc: john.mcnamara, dmitry.kozliuk, Aaron Conole, Michael Santana
Enable ASan, this can greatly help identify leaks and buffer overflows.
Running all unit tests is not possible at the moment: skip unit tests
who have known issues with ASan.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
.ci/linux-build.sh | 8 ++
.github/workflows/build.yml | 3 +-
app/test/meson.build | 208 +++++++++++++++++++-----------------
3 files changed, 118 insertions(+), 101 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 774a1441bf..93706c0131 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -95,6 +95,14 @@ if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
fi
+
+if [ "$ASAN" = "true" ]; then
+ OPTS="$OPTS -Db_sanitize=address"
+ if [ "${CC%%clang}" != "$CC" ]; then
+ OPTS="$OPTS -Db_lundef=false"
+ fi
+fi
+
meson build --werror $OPTS
ninja -C build
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 22daaabb91..45871e76ed 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -16,6 +16,7 @@ jobs:
env:
AARCH64: ${{ matrix.config.cross == 'aarch64' }}
ABI_CHECKS: ${{ contains(matrix.config.checks, 'abi') }}
+ ASAN: ${{ contains(matrix.config.checks, 'asan') }}
BUILD_32BIT: ${{ matrix.config.cross == 'i386' }}
BUILD_DOCS: ${{ contains(matrix.config.checks, 'doc') }}
CC: ccache ${{ matrix.config.compiler }}
@@ -47,7 +48,7 @@ jobs:
- os: ubuntu-18.04
compiler: clang
library: shared
- checks: doc+tests
+ checks: asan+doc+tests
- os: ubuntu-18.04
compiler: gcc
library: static
diff --git a/app/test/meson.build b/app/test/meson.build
index 5fc1dd1b7b..4622b5c010 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -149,96 +149,97 @@ test_deps = enabled_libs
# as well as libs, the pci and vdev bus drivers are needed for a lot of tests
test_deps += ['bus_pci', 'bus_vdev']
-# Each test is marked with flag true/false
-# to indicate whether it can run in no-huge mode.
+# Each test is marked with flags:
+# - the first flag indicates whether the test can run in no-huge mode,
+# - the second flag indicates whether the test can run with ASan enabled,
fast_tests = [
- ['acl_autotest', true],
- ['atomic_autotest', false],
- ['bitmap_autotest', true],
- ['bpf_autotest', true],
- ['bpf_convert_autotest', true],
- ['bitops_autotest', true],
- ['byteorder_autotest', true],
- ['cksum_autotest', true],
- ['cmdline_autotest', true],
- ['common_autotest', true],
- ['cpuflags_autotest', true],
- ['debug_autotest', true],
- ['devargs_autotest', true],
- ['eal_flags_c_opt_autotest', false],
- ['eal_flags_main_opt_autotest', false],
- ['eal_flags_n_opt_autotest', false],
- ['eal_flags_hpet_autotest', false],
- ['eal_flags_no_huge_autotest', false],
- ['eal_flags_a_opt_autotest', false],
- ['eal_flags_b_opt_autotest', false],
- ['eal_flags_vdev_opt_autotest', false],
- ['eal_flags_r_opt_autotest', false],
- ['eal_flags_mem_autotest', false],
- ['eal_flags_file_prefix_autotest', false],
- ['eal_flags_misc_autotest', false],
- ['eal_fs_autotest', true],
- ['errno_autotest', true],
- ['ethdev_link_status', true],
- ['event_ring_autotest', true],
- ['fib_autotest', true],
- ['fib6_autotest', true],
- ['func_reentrancy_autotest', false],
- ['hash_autotest', true],
- ['interrupt_autotest', true],
- ['ipfrag_autotest', false],
- ['lcores_autotest', true],
- ['logs_autotest', true],
- ['lpm_autotest', true],
- ['lpm6_autotest', true],
- ['malloc_autotest', false],
- ['mbuf_autotest', false],
- ['mcslock_autotest', false],
- ['memcpy_autotest', true],
- ['memory_autotest', false],
- ['mempool_autotest', false],
- ['memzone_autotest', false],
- ['meter_autotest', true],
- ['multiprocess_autotest', false],
- ['per_lcore_autotest', true],
- ['pflock_autotest', true],
- ['prefetch_autotest', true],
- ['rcu_qsbr_autotest', true],
- ['pie_autotest', true],
- ['rib_autotest', true],
- ['rib6_autotest', true],
- ['ring_autotest', true],
- ['rwlock_test1_autotest', true],
- ['rwlock_rda_autotest', true],
- ['rwlock_rds_wrm_autotest', true],
- ['rwlock_rde_wro_autotest', true],
- ['sched_autotest', true],
- ['security_autotest', false],
- ['spinlock_autotest', true],
- ['stack_autotest', false],
- ['stack_lf_autotest', false],
- ['string_autotest', true],
- ['tailq_autotest', true],
- ['ticketlock_autotest', true],
- ['timer_autotest', false],
- ['user_delay_us', true],
- ['version_autotest', true],
- ['crc_autotest', true],
- ['distributor_autotest', false],
- ['eventdev_common_autotest', true],
- ['fbarray_autotest', true],
- ['hash_readwrite_func_autotest', false],
- ['ipsec_autotest', true],
- ['kni_autotest', false],
- ['kvargs_autotest', true],
- ['member_autotest', true],
- ['power_cpufreq_autotest', false],
- ['power_autotest', true],
- ['power_kvm_vm_autotest', false],
- ['reorder_autotest', true],
- ['service_autotest', true],
- ['thash_autotest', true],
- ['trace_autotest', true],
+ ['acl_autotest', true, true],
+ ['atomic_autotest', false, true],
+ ['bitmap_autotest', true, true],
+ ['bpf_autotest', true, true],
+ ['bpf_convert_autotest', true, true],
+ ['bitops_autotest', true, true],
+ ['byteorder_autotest', true, true],
+ ['cksum_autotest', true, true],
+ ['cmdline_autotest', true, true],
+ ['common_autotest', true, true],
+ ['cpuflags_autotest', true, true],
+ ['debug_autotest', true, true],
+ ['devargs_autotest', true, true],
+ ['eal_flags_c_opt_autotest', false, false],
+ ['eal_flags_main_opt_autotest', false, false],
+ ['eal_flags_n_opt_autotest', false, false],
+ ['eal_flags_hpet_autotest', false, false],
+ ['eal_flags_no_huge_autotest', false, false],
+ ['eal_flags_a_opt_autotest', false, false],
+ ['eal_flags_b_opt_autotest', false, false],
+ ['eal_flags_vdev_opt_autotest', false, false],
+ ['eal_flags_r_opt_autotest', false, false],
+ ['eal_flags_mem_autotest', false, false],
+ ['eal_flags_file_prefix_autotest', false, false],
+ ['eal_flags_misc_autotest', false, false],
+ ['eal_fs_autotest', true, true],
+ ['errno_autotest', true, true],
+ ['ethdev_link_status', true, true],
+ ['event_ring_autotest', true, true],
+ ['fib_autotest', true, true],
+ ['fib6_autotest', true, true],
+ ['func_reentrancy_autotest', false, true],
+ ['hash_autotest', true, true],
+ ['interrupt_autotest', true, true],
+ ['ipfrag_autotest', false, true],
+ ['lcores_autotest', true, true],
+ ['logs_autotest', true, true],
+ ['lpm_autotest', true, true],
+ ['lpm6_autotest', true, true],
+ ['malloc_autotest', false, true],
+ ['mbuf_autotest', false, true],
+ ['mcslock_autotest', false, true],
+ ['memcpy_autotest', true, true],
+ ['memory_autotest', false, true],
+ ['mempool_autotest', false, true],
+ ['memzone_autotest', false, true],
+ ['meter_autotest', true, true],
+ ['multiprocess_autotest', false, false],
+ ['per_lcore_autotest', true, true],
+ ['pflock_autotest', true, true],
+ ['prefetch_autotest', true, true],
+ ['rcu_qsbr_autotest', true, true],
+ ['pie_autotest', true, true],
+ ['rib_autotest', true, true],
+ ['rib6_autotest', true, true],
+ ['ring_autotest', true, true],
+ ['rwlock_test1_autotest', true, true],
+ ['rwlock_rda_autotest', true, true],
+ ['rwlock_rds_wrm_autotest', true, true],
+ ['rwlock_rde_wro_autotest', true, true],
+ ['sched_autotest', true, true],
+ ['security_autotest', false, true],
+ ['spinlock_autotest', true, true],
+ ['stack_autotest', false, true],
+ ['stack_lf_autotest', false, true],
+ ['string_autotest', true, true],
+ ['tailq_autotest', true, true],
+ ['ticketlock_autotest', true, true],
+ ['timer_autotest', false, true],
+ ['user_delay_us', true, true],
+ ['version_autotest', true, true],
+ ['crc_autotest', true, true],
+ ['distributor_autotest', false, true],
+ ['eventdev_common_autotest', true, true],
+ ['fbarray_autotest', true, true],
+ ['hash_readwrite_func_autotest', false, true],
+ ['ipsec_autotest', true, true],
+ ['kni_autotest', false, true],
+ ['kvargs_autotest', true, true],
+ ['member_autotest', true, true],
+ ['power_cpufreq_autotest', false, true],
+ ['power_autotest', true, true],
+ ['power_kvm_vm_autotest', false, true],
+ ['reorder_autotest', true, true],
+ ['service_autotest', true, true],
+ ['thash_autotest', true, true],
+ ['trace_autotest', true, true],
]
# Tests known to have issues or which don't belong in other tests lists.
@@ -345,15 +346,16 @@ endif
if dpdk_conf.has('RTE_LIB_FLOW_CLASSIFY')
test_sources += 'test_flow_classify.c'
- fast_tests += [['flow_classify_autotest', false]]
+ fast_tests += [['flow_classify_autotest', false, true]]
endif
if dpdk_conf.has('RTE_LIB_METRICS')
test_sources += ['test_metrics.c']
- fast_tests += [['metrics_autotest', true]]
+ fast_tests += [['metrics_autotest', true, true]]
endif
if not is_windows and dpdk_conf.has('RTE_LIB_TELEMETRY')
test_sources += ['test_telemetry_json.c', 'test_telemetry_data.c']
- fast_tests += [['telemetry_json_autotest', true], ['telemetry_data_autotest', true]]
+ fast_tests += [['telemetry_json_autotest', true, true]]
+ fast_tests += [['telemetry_data_autotest', true, true]]
endif
if dpdk_conf.has('RTE_LIB_PIPELINE')
# pipeline lib depends on port and table libs, so those must be present
@@ -366,7 +368,7 @@ if dpdk_conf.has('RTE_LIB_PIPELINE')
'test_table_ports.c',
'test_table_tables.c',
]
- fast_tests += [['table_autotest', true]]
+ fast_tests += [['table_autotest', true, false]] # https://bugs.dpdk.org/show_bug.cgi?id=820
endif
# The following linkages of drivers are required because
@@ -386,26 +388,26 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_pmd_ring.c'
test_sources += 'test_event_eth_tx_adapter.c'
test_sources += 'sample_packet_forward.c'
- fast_tests += [['ring_pmd_autotest', true]]
+ fast_tests += [['ring_pmd_autotest', true, true]]
perf_test_names += 'ring_pmd_perf_autotest'
- fast_tests += [['event_eth_tx_adapter_autotest', false]]
+ fast_tests += [['event_eth_tx_adapter_autotest', false, true]]
if dpdk_conf.has('RTE_LIB_BITRATESTATS')
test_sources += 'test_bitratestats.c'
- fast_tests += [['bitratestats_autotest', true]]
+ fast_tests += [['bitratestats_autotest', true, true]]
endif
if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
test_sources += 'test_latencystats.c'
- fast_tests += [['latencystats_autotest', true]]
+ fast_tests += [['latencystats_autotest', true, true]]
endif
if dpdk_conf.has('RTE_LIB_PDUMP')
test_sources += 'test_pdump.c'
- fast_tests += [['pdump_autotest', true]]
+ fast_tests += [['pdump_autotest', true, false]]
endif
endif
if dpdk_conf.has('RTE_NET_NULL')
test_deps += 'net_null'
test_sources += 'test_vdev.c'
- fast_tests += [['vdev_autotest', true]]
+ fast_tests += [['vdev_autotest', true, true]]
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
@@ -431,7 +433,7 @@ if dpdk_conf.has('RTE_LIB_COMPRESSDEV')
if compress_test_dep.found()
test_dep_objs += compress_test_dep
test_sources += 'test_compressdev.c'
- fast_tests += [['compressdev_autotest', false]]
+ fast_tests += [['compressdev_autotest', false, true]]
endif
endif
@@ -478,6 +480,12 @@ foreach arg : fast_tests
endif
endif
+ if get_option('b_sanitize') == 'address' or get_option('b_sanitize') == 'address,undefined'
+ if not arg[2]
+ run_test = false
+ endif
+ endif
+
if (get_option('default_library') == 'shared' and
arg[0] == 'event_eth_tx_adapter_autotest')
foreach drv:dpdk_drivers
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-15 17:31 ` [PATCH 2/3] mem: fix ASan shadow for remapped memory segments David Marchand
@ 2022-04-20 14:47 ` Burakov, Anatoly
2022-04-21 9:37 ` David Marchand
0 siblings, 1 reply; 18+ messages in thread
From: Burakov, Anatoly @ 2022-04-20 14:47 UTC (permalink / raw)
To: David Marchand, dev
Cc: john.mcnamara, dmitry.kozliuk, stable, Xueqin Lin, Zhihong Peng
On 15-Apr-22 6:31 PM, David Marchand wrote:
> When releasing some memory, the allocator can choose to return some
> pages to the OS. At the same time, this memory was poisoned in ASAn
> shadow. Doing the latter made it impossible to remap this same page
> later.
> On the other hand, without this poison, the OS would pagefault in any
> case for this page.
>
> Remove the poisoning for unmapped pages.
>
> Bugzilla ID: 994
> Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> lib/eal/common/malloc_elem.h | 4 ++++
> lib/eal/common/malloc_heap.c | 12 +++++++++++-
> 2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
> index 228f178418..b859003722 100644
> --- a/lib/eal/common/malloc_elem.h
> +++ b/lib/eal/common/malloc_elem.h
> @@ -272,6 +272,10 @@ old_malloc_size(struct malloc_elem *elem)
>
> #else /* !RTE_MALLOC_ASAN */
>
> +static inline void
> +asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
> + uint32_t val __rte_unused) { }
> +
> static inline void
> asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
>
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index 6c572b6f2c..5913d9f862 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -860,6 +860,7 @@ malloc_heap_free(struct malloc_elem *elem)
> size_t len, aligned_len, page_sz;
> struct rte_memseg_list *msl;
> unsigned int i, n_segs, before_space, after_space;
> + bool unmapped_pages = false;
> int ret;
> const struct internal_config *internal_conf =
> eal_get_internal_configuration();
> @@ -999,6 +1000,13 @@ malloc_heap_free(struct malloc_elem *elem)
>
> /* don't care if any of this fails */
> malloc_heap_free_pages(aligned_start, aligned_len);
> + /*
> + * Clear any poisoning in ASan for the associated pages so that
> + * next time EAL maps those pages, the allocator can access
> + * them.
> + */
> + asan_set_zone(aligned_start, aligned_len, 0x00);
> + unmapped_pages = true;
>
> request_sync();
> } else {
> @@ -1032,7 +1040,9 @@ malloc_heap_free(struct malloc_elem *elem)
>
> rte_mcfg_mem_write_unlock();
> free_unlock:
> - asan_set_freezone(asan_ptr, asan_data_len);
> + /* Poison memory range if belonging to some still mapped pages. */
> + if (!unmapped_pages)
> + asan_set_freezone(asan_ptr, asan_data_len);
>
> rte_spinlock_unlock(&(heap->lock));
> return ret;
I suspect the patch should be a little more complicated than that. When
we unmap pages, we don't necessarily unmap the entire malloc element, it
could be that we have a freed allocation like so:
| malloc header | free space | unmapped space | free space | next malloc
header |
So, i think the freezone should be set from asan_ptr till aligned_start,
and then from (aligned_start + aligned_len) till (asan_ptr +
asan_data_len). Does that make sense?
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem
2022-04-15 17:31 ` [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem David Marchand
@ 2022-04-20 14:48 ` Burakov, Anatoly
0 siblings, 0 replies; 18+ messages in thread
From: Burakov, Anatoly @ 2022-04-20 14:48 UTC (permalink / raw)
To: David Marchand, dev
Cc: john.mcnamara, dmitry.kozliuk, stable, Xueqin Lin, Zhihong Peng
On 15-Apr-22 6:31 PM, David Marchand wrote:
> As described in bugzilla, ASan reports accesses to all memory segment as
> invalid, since those parts have not been allocated.
> Move __rte_no_asan to rte_common.h and disable ASan on a part of the test.
>
> Bugzilla ID: 880
> Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-20 14:47 ` Burakov, Anatoly
@ 2022-04-21 9:37 ` David Marchand
2022-04-21 9:50 ` David Marchand
2022-04-21 13:18 ` Burakov, Anatoly
0 siblings, 2 replies; 18+ messages in thread
From: David Marchand @ 2022-04-21 9:37 UTC (permalink / raw)
To: Burakov, Anatoly
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On Wed, Apr 20, 2022 at 4:47 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 15-Apr-22 6:31 PM, David Marchand wrote:
> > When releasing some memory, the allocator can choose to return some
> > pages to the OS. At the same time, this memory was poisoned in ASAn
> > shadow. Doing the latter made it impossible to remap this same page
> > later.
> > On the other hand, without this poison, the OS would pagefault in any
> > case for this page.
> >
> > Remove the poisoning for unmapped pages.
> >
> > Bugzilla ID: 994
> > Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> > lib/eal/common/malloc_elem.h | 4 ++++
> > lib/eal/common/malloc_heap.c | 12 +++++++++++-
> > 2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
> > index 228f178418..b859003722 100644
> > --- a/lib/eal/common/malloc_elem.h
> > +++ b/lib/eal/common/malloc_elem.h
> > @@ -272,6 +272,10 @@ old_malloc_size(struct malloc_elem *elem)
> >
> > #else /* !RTE_MALLOC_ASAN */
> >
> > +static inline void
> > +asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
> > + uint32_t val __rte_unused) { }
> > +
> > static inline void
> > asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
> >
> > diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> > index 6c572b6f2c..5913d9f862 100644
> > --- a/lib/eal/common/malloc_heap.c
> > +++ b/lib/eal/common/malloc_heap.c
> > @@ -860,6 +860,7 @@ malloc_heap_free(struct malloc_elem *elem)
> > size_t len, aligned_len, page_sz;
> > struct rte_memseg_list *msl;
> > unsigned int i, n_segs, before_space, after_space;
> > + bool unmapped_pages = false;
> > int ret;
> > const struct internal_config *internal_conf =
> > eal_get_internal_configuration();
> > @@ -999,6 +1000,13 @@ malloc_heap_free(struct malloc_elem *elem)
> >
> > /* don't care if any of this fails */
> > malloc_heap_free_pages(aligned_start, aligned_len);
> > + /*
> > + * Clear any poisoning in ASan for the associated pages so that
> > + * next time EAL maps those pages, the allocator can access
> > + * them.
> > + */
> > + asan_set_zone(aligned_start, aligned_len, 0x00);
> > + unmapped_pages = true;
> >
> > request_sync();
> > } else {
> > @@ -1032,7 +1040,9 @@ malloc_heap_free(struct malloc_elem *elem)
> >
> > rte_mcfg_mem_write_unlock();
> > free_unlock:
> > - asan_set_freezone(asan_ptr, asan_data_len);
> > + /* Poison memory range if belonging to some still mapped pages. */
> > + if (!unmapped_pages)
> > + asan_set_freezone(asan_ptr, asan_data_len);
> >
> > rte_spinlock_unlock(&(heap->lock));
> > return ret;
>
> I suspect the patch should be a little more complicated than that. When
> we unmap pages, we don't necessarily unmap the entire malloc element, it
> could be that we have a freed allocation like so:
>
> | malloc header | free space | unmapped space | free space | next malloc
> header |
>
> So, i think the freezone should be set from asan_ptr till aligned_start,
> and then from (aligned_start + aligned_len) till (asan_ptr +
> asan_data_len). Does that make sense?
(btw, I get a bounce for Zhihong mail address, is he not working at
Intel anymore?)
To be honest, I don't understand if we can get to this situation :-)
(especially the free space after the unmapped region).
But I guess you mean something like (on top of current patch):
@@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
rte_mcfg_mem_write_unlock();
free_unlock:
- /* Poison memory range if belonging to some still mapped pages. */
- if (!unmapped_pages)
+ if (!unmapped_pages) {
asan_set_freezone(asan_ptr, asan_data_len);
+ } else {
+ /*
+ * We may be in a situation where we unmapped pages like this:
+ * malloc header | free space | unmapped space | free
space | malloc header
+ */
+ void *free1_start = asan_ptr;
+ void *free1_end = aligned_start;
+ void *free2_start = RTE_PTR_ADD(aligned_start, aligned_len);
+ void *free2_end = RTE_PTR_ADD(asan_ptr, asan_data_len);
+
+ if (free1_start < free1_end)
+ asan_set_freezone(free1_start,
+ RTE_PTR_DIFF(free1_end, free1_start));
+ if (free2_start < free2_end)
+ asan_set_freezone(free2_start,
+ RTE_PTR_DIFF(free2_end, free2_start));
+ }
rte_spinlock_unlock(&(heap->lock));
return ret;
--
David Marchand
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-21 9:37 ` David Marchand
@ 2022-04-21 9:50 ` David Marchand
2022-04-21 13:18 ` Burakov, Anatoly
1 sibling, 0 replies; 18+ messages in thread
From: David Marchand @ 2022-04-21 9:50 UTC (permalink / raw)
To: Burakov, Anatoly
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On Thu, Apr 21, 2022 at 11:37 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Wed, Apr 20, 2022 at 4:47 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
> >
> > On 15-Apr-22 6:31 PM, David Marchand wrote:
> > > When releasing some memory, the allocator can choose to return some
> > > pages to the OS. At the same time, this memory was poisoned in ASAn
> > > shadow. Doing the latter made it impossible to remap this same page
> > > later.
> > > On the other hand, without this poison, the OS would pagefault in any
> > > case for this page.
> > >
> > > Remove the poisoning for unmapped pages.
> > >
> > > Bugzilla ID: 994
> > > Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > ---
> > > lib/eal/common/malloc_elem.h | 4 ++++
> > > lib/eal/common/malloc_heap.c | 12 +++++++++++-
> > > 2 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
> > > index 228f178418..b859003722 100644
> > > --- a/lib/eal/common/malloc_elem.h
> > > +++ b/lib/eal/common/malloc_elem.h
> > > @@ -272,6 +272,10 @@ old_malloc_size(struct malloc_elem *elem)
> > >
> > > #else /* !RTE_MALLOC_ASAN */
> > >
> > > +static inline void
> > > +asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
> > > + uint32_t val __rte_unused) { }
> > > +
> > > static inline void
> > > asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
> > >
> > > diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> > > index 6c572b6f2c..5913d9f862 100644
> > > --- a/lib/eal/common/malloc_heap.c
> > > +++ b/lib/eal/common/malloc_heap.c
> > > @@ -860,6 +860,7 @@ malloc_heap_free(struct malloc_elem *elem)
> > > size_t len, aligned_len, page_sz;
> > > struct rte_memseg_list *msl;
> > > unsigned int i, n_segs, before_space, after_space;
> > > + bool unmapped_pages = false;
> > > int ret;
> > > const struct internal_config *internal_conf =
> > > eal_get_internal_configuration();
> > > @@ -999,6 +1000,13 @@ malloc_heap_free(struct malloc_elem *elem)
> > >
> > > /* don't care if any of this fails */
> > > malloc_heap_free_pages(aligned_start, aligned_len);
> > > + /*
> > > + * Clear any poisoning in ASan for the associated pages so that
> > > + * next time EAL maps those pages, the allocator can access
> > > + * them.
> > > + */
> > > + asan_set_zone(aligned_start, aligned_len, 0x00);
> > > + unmapped_pages = true;
> > >
> > > request_sync();
> > > } else {
> > > @@ -1032,7 +1040,9 @@ malloc_heap_free(struct malloc_elem *elem)
> > >
> > > rte_mcfg_mem_write_unlock();
> > > free_unlock:
> > > - asan_set_freezone(asan_ptr, asan_data_len);
> > > + /* Poison memory range if belonging to some still mapped pages. */
> > > + if (!unmapped_pages)
> > > + asan_set_freezone(asan_ptr, asan_data_len);
> > >
> > > rte_spinlock_unlock(&(heap->lock));
> > > return ret;
> >
> > I suspect the patch should be a little more complicated than that. When
> > we unmap pages, we don't necessarily unmap the entire malloc element, it
> > could be that we have a freed allocation like so:
> >
> > | malloc header | free space | unmapped space | free space | next malloc
> > header |
> >
> > So, i think the freezone should be set from asan_ptr till aligned_start,
> > and then from (aligned_start + aligned_len) till (asan_ptr +
> > asan_data_len). Does that make sense?
>
> (btw, I get a bounce for Zhihong mail address, is he not working at
> Intel anymore?)
>
> To be honest, I don't understand if we can get to this situation :-)
> (especially the free space after the unmapped region).
> But I guess you mean something like (on top of current patch):
>
> @@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
>
> rte_mcfg_mem_write_unlock();
> free_unlock:
> - /* Poison memory range if belonging to some still mapped pages. */
> - if (!unmapped_pages)
> + if (!unmapped_pages) {
> asan_set_freezone(asan_ptr, asan_data_len);
> + } else {
> + /*
> + * We may be in a situation where we unmapped pages like this:
> + * malloc header | free space | unmapped space | free
> space | malloc header
> + */
> + void *free1_start = asan_ptr;
> + void *free1_end = aligned_start;
> + void *free2_start = RTE_PTR_ADD(aligned_start, aligned_len);
> + void *free2_end = RTE_PTR_ADD(asan_ptr, asan_data_len);
> +
> + if (free1_start < free1_end)
> + asan_set_freezone(free1_start,
> + RTE_PTR_DIFF(free1_end, free1_start));
> + if (free2_start < free2_end)
> + asan_set_freezone(free2_start,
> + RTE_PTR_DIFF(free2_end, free2_start));
> + }
>
> rte_spinlock_unlock(&(heap->lock));
> return ret;
But I get a splat for func_reentrancy_autotest:
=================================================================
==4098809==ERROR: AddressSanitizer: heap-use-after-free on address
0x7fa00fa00030 at pc 0x0000035779fe bp 0x7ffe01ed0cf0 sp
0x7ffe01ed0ce8
READ of size 1 at 0x7fa00fa00030 thread T0
#0 0x35779fd in malloc_elem_join_adjacent_free
../lib/eal/common/malloc_elem.c:539
#1 0x3577bc5 in malloc_elem_free ../lib/eal/common/malloc_elem.c:586
#2 0x357bd25 in malloc_heap_free ../lib/eal/common/malloc_heap.c:886
#3 0x357e032 in mem_free ../lib/eal/common/rte_malloc.c:37
#4 0x357e032 in rte_free ../lib/eal/common/rte_malloc.c:44
#5 0x336a547 in rte_lpm_free ../lib/lpm/rte_lpm.c:281
#6 0x2cb3488 in lpm_create_free ../app/test/test_func_reentrancy.c:395
#7 0x2cb47ed in launch_test ../app/test/test_func_reentrancy.c:455
#8 0x2cb47ed in test_func_reentrancy ../app/test/test_func_reentrancy.c:502
#9 0x2b0dd75 in cmd_autotest_parsed ../app/test/commands.c:68
#10 0x34e5dce in cmdline_parse ../lib/cmdline/cmdline_parse.c:287
#11 0x34e31af in cmdline_valid_buffer ../lib/cmdline/cmdline.c:24
#12 0x34eba9f in rdline_char_in ../lib/cmdline/cmdline_rdline.c:444
#13 0x34e329b in cmdline_in ../lib/cmdline/cmdline.c:146
#14 0x34e329b in cmdline_in ../lib/cmdline/cmdline.c:135
#15 0x40d8a5 in main ../app/test/test.c:217
#16 0x7fa41009b55f in __libc_start_call_main
../sysdeps/nptl/libc_start_call_main.h:58
#17 0x7fa41009b60b in __libc_start_main_impl ../csu/libc-start.c:409
#18 0x2b0dc64 in _start
(/home/dmarchan/dpdk/build-gcc-asan/app/test/dpdk-test+0x2b0dc64)
Address 0x7fa00fa00030 is a wild pointer.
SUMMARY: AddressSanitizer: heap-use-after-free
../lib/eal/common/malloc_elem.c:539 in malloc_elem_join_adjacent_free
Shadow bytes around the buggy address:
0x0ff481f37fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff481f37fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff481f37fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff481f37fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0ff481f37ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ff481f38000: fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd
0x0ff481f38010: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0ff481f38020: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0ff481f38030: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0ff481f38040: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x0ff481f38050: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
--
David Marchand
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-21 9:37 ` David Marchand
2022-04-21 9:50 ` David Marchand
@ 2022-04-21 13:18 ` Burakov, Anatoly
2022-04-26 12:54 ` Burakov, Anatoly
1 sibling, 1 reply; 18+ messages in thread
From: Burakov, Anatoly @ 2022-04-21 13:18 UTC (permalink / raw)
To: David Marchand
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On 21-Apr-22 10:37 AM, David Marchand wrote:
> On Wed, Apr 20, 2022 at 4:47 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>
>> On 15-Apr-22 6:31 PM, David Marchand wrote:
>>> When releasing some memory, the allocator can choose to return some
>>> pages to the OS. At the same time, this memory was poisoned in ASAn
>>> shadow. Doing the latter made it impossible to remap this same page
>>> later.
>>> On the other hand, without this poison, the OS would pagefault in any
>>> case for this page.
>>>
>>> Remove the poisoning for unmapped pages.
>>>
>>> Bugzilla ID: 994
>>> Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>> ---
>>> lib/eal/common/malloc_elem.h | 4 ++++
>>> lib/eal/common/malloc_heap.c | 12 +++++++++++-
>>> 2 files changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
>>> index 228f178418..b859003722 100644
>>> --- a/lib/eal/common/malloc_elem.h
>>> +++ b/lib/eal/common/malloc_elem.h
>>> @@ -272,6 +272,10 @@ old_malloc_size(struct malloc_elem *elem)
>>>
>>> #else /* !RTE_MALLOC_ASAN */
>>>
>>> +static inline void
>>> +asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
>>> + uint32_t val __rte_unused) { }
>>> +
>>> static inline void
>>> asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
>>>
>>> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
>>> index 6c572b6f2c..5913d9f862 100644
>>> --- a/lib/eal/common/malloc_heap.c
>>> +++ b/lib/eal/common/malloc_heap.c
>>> @@ -860,6 +860,7 @@ malloc_heap_free(struct malloc_elem *elem)
>>> size_t len, aligned_len, page_sz;
>>> struct rte_memseg_list *msl;
>>> unsigned int i, n_segs, before_space, after_space;
>>> + bool unmapped_pages = false;
>>> int ret;
>>> const struct internal_config *internal_conf =
>>> eal_get_internal_configuration();
>>> @@ -999,6 +1000,13 @@ malloc_heap_free(struct malloc_elem *elem)
>>>
>>> /* don't care if any of this fails */
>>> malloc_heap_free_pages(aligned_start, aligned_len);
>>> + /*
>>> + * Clear any poisoning in ASan for the associated pages so that
>>> + * next time EAL maps those pages, the allocator can access
>>> + * them.
>>> + */
>>> + asan_set_zone(aligned_start, aligned_len, 0x00);
>>> + unmapped_pages = true;
>>>
>>> request_sync();
>>> } else {
>>> @@ -1032,7 +1040,9 @@ malloc_heap_free(struct malloc_elem *elem)
>>>
>>> rte_mcfg_mem_write_unlock();
>>> free_unlock:
>>> - asan_set_freezone(asan_ptr, asan_data_len);
>>> + /* Poison memory range if belonging to some still mapped pages. */
>>> + if (!unmapped_pages)
>>> + asan_set_freezone(asan_ptr, asan_data_len);
>>>
>>> rte_spinlock_unlock(&(heap->lock));
>>> return ret;
>>
>> I suspect the patch should be a little more complicated than that. When
>> we unmap pages, we don't necessarily unmap the entire malloc element, it
>> could be that we have a freed allocation like so:
>>
>> | malloc header | free space | unmapped space | free space | next malloc
>> header |
>>
>> So, i think the freezone should be set from asan_ptr till aligned_start,
>> and then from (aligned_start + aligned_len) till (asan_ptr +
>> asan_data_len). Does that make sense?
>
> (btw, I get a bounce for Zhihong mail address, is he not working at
> Intel anymore?)
>
> To be honest, I don't understand if we can get to this situation :-)
> (especially the free space after the unmapped region).
> But I guess you mean something like (on top of current patch):
>
> @@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
>
> rte_mcfg_mem_write_unlock();
> free_unlock:
> - /* Poison memory range if belonging to some still mapped pages. */
> - if (!unmapped_pages)
> + if (!unmapped_pages) {
> asan_set_freezone(asan_ptr, asan_data_len);
> + } else {
> + /*
> + * We may be in a situation where we unmapped pages like this:
> + * malloc header | free space | unmapped space | free
> space | malloc header
> + */
> + void *free1_start = asan_ptr;
> + void *free1_end = aligned_start;
> + void *free2_start = RTE_PTR_ADD(aligned_start, aligned_len);
> + void *free2_end = RTE_PTR_ADD(asan_ptr, asan_data_len);
> +
> + if (free1_start < free1_end)
> + asan_set_freezone(free1_start,
> + RTE_PTR_DIFF(free1_end, free1_start));
> + if (free2_start < free2_end)
> + asan_set_freezone(free2_start,
> + RTE_PTR_DIFF(free2_end, free2_start));
> + }
>
> rte_spinlock_unlock(&(heap->lock));
> return ret;
>
Something like that, yes. I will have to think through this a bit more,
especially in light of your func_reentrancy splat :)
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-21 13:18 ` Burakov, Anatoly
@ 2022-04-26 12:54 ` Burakov, Anatoly
2022-04-26 14:15 ` David Marchand
0 siblings, 1 reply; 18+ messages in thread
From: Burakov, Anatoly @ 2022-04-26 12:54 UTC (permalink / raw)
To: David Marchand
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On 21-Apr-22 2:18 PM, Burakov, Anatoly wrote:
> On 21-Apr-22 10:37 AM, David Marchand wrote:
>> On Wed, Apr 20, 2022 at 4:47 PM Burakov, Anatoly
>> <anatoly.burakov@intel.com> wrote:
>>>
>>> On 15-Apr-22 6:31 PM, David Marchand wrote:
>>>> When releasing some memory, the allocator can choose to return some
>>>> pages to the OS. At the same time, this memory was poisoned in ASAn
>>>> shadow. Doing the latter made it impossible to remap this same page
>>>> later.
>>>> On the other hand, without this poison, the OS would pagefault in any
>>>> case for this page.
>>>>
>>>> Remove the poisoning for unmapped pages.
>>>>
>>>> Bugzilla ID: 994
>>>> Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>>>> ---
>>>> lib/eal/common/malloc_elem.h | 4 ++++
>>>> lib/eal/common/malloc_heap.c | 12 +++++++++++-
>>>> 2 files changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/eal/common/malloc_elem.h
>>>> b/lib/eal/common/malloc_elem.h
>>>> index 228f178418..b859003722 100644
>>>> --- a/lib/eal/common/malloc_elem.h
>>>> +++ b/lib/eal/common/malloc_elem.h
>>>> @@ -272,6 +272,10 @@ old_malloc_size(struct malloc_elem *elem)
>>>>
>>>> #else /* !RTE_MALLOC_ASAN */
>>>>
>>>> +static inline void
>>>> +asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
>>>> + uint32_t val __rte_unused) { }
>>>> +
>>>> static inline void
>>>> asan_set_freezone(void *ptr __rte_unused, size_t size
>>>> __rte_unused) { }
>>>>
>>>> diff --git a/lib/eal/common/malloc_heap.c
>>>> b/lib/eal/common/malloc_heap.c
>>>> index 6c572b6f2c..5913d9f862 100644
>>>> --- a/lib/eal/common/malloc_heap.c
>>>> +++ b/lib/eal/common/malloc_heap.c
>>>> @@ -860,6 +860,7 @@ malloc_heap_free(struct malloc_elem *elem)
>>>> size_t len, aligned_len, page_sz;
>>>> struct rte_memseg_list *msl;
>>>> unsigned int i, n_segs, before_space, after_space;
>>>> + bool unmapped_pages = false;
>>>> int ret;
>>>> const struct internal_config *internal_conf =
>>>> eal_get_internal_configuration();
>>>> @@ -999,6 +1000,13 @@ malloc_heap_free(struct malloc_elem *elem)
>>>>
>>>> /* don't care if any of this fails */
>>>> malloc_heap_free_pages(aligned_start, aligned_len);
>>>> + /*
>>>> + * Clear any poisoning in ASan for the associated
>>>> pages so that
>>>> + * next time EAL maps those pages, the allocator can
>>>> access
>>>> + * them.
>>>> + */
>>>> + asan_set_zone(aligned_start, aligned_len, 0x00);
>>>> + unmapped_pages = true;
>>>>
>>>> request_sync();
>>>> } else {
>>>> @@ -1032,7 +1040,9 @@ malloc_heap_free(struct malloc_elem *elem)
>>>>
>>>> rte_mcfg_mem_write_unlock();
>>>> free_unlock:
>>>> - asan_set_freezone(asan_ptr, asan_data_len);
>>>> + /* Poison memory range if belonging to some still mapped
>>>> pages. */
>>>> + if (!unmapped_pages)
>>>> + asan_set_freezone(asan_ptr, asan_data_len);
>>>>
>>>> rte_spinlock_unlock(&(heap->lock));
>>>> return ret;
>>>
>>> I suspect the patch should be a little more complicated than that. When
>>> we unmap pages, we don't necessarily unmap the entire malloc element, it
>>> could be that we have a freed allocation like so:
>>>
>>> | malloc header | free space | unmapped space | free space | next malloc
>>> header |
>>>
>>> So, i think the freezone should be set from asan_ptr till aligned_start,
>>> and then from (aligned_start + aligned_len) till (asan_ptr +
>>> asan_data_len). Does that make sense?
>>
>> (btw, I get a bounce for Zhihong mail address, is he not working at
>> Intel anymore?)
>>
>> To be honest, I don't understand if we can get to this situation :-)
>> (especially the free space after the unmapped region).
>> But I guess you mean something like (on top of current patch):
>>
>> @@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
>>
>> rte_mcfg_mem_write_unlock();
>> free_unlock:
>> - /* Poison memory range if belonging to some still mapped
>> pages. */
>> - if (!unmapped_pages)
>> + if (!unmapped_pages) {
>> asan_set_freezone(asan_ptr, asan_data_len);
>> + } else {
>> + /*
>> + * We may be in a situation where we unmapped pages
>> like this:
>> + * malloc header | free space | unmapped space | free
>> space | malloc header
>> + */
>> + void *free1_start = asan_ptr;
>> + void *free1_end = aligned_start;
>> + void *free2_start = RTE_PTR_ADD(aligned_start,
>> aligned_len);
>> + void *free2_end = RTE_PTR_ADD(asan_ptr, asan_data_len);
>> +
>> + if (free1_start < free1_end)
>> + asan_set_freezone(free1_start,
>> + RTE_PTR_DIFF(free1_end, free1_start));
>> + if (free2_start < free2_end)
>> + asan_set_freezone(free2_start,
>> + RTE_PTR_DIFF(free2_end, free2_start));
>> + }
>>
>> rte_spinlock_unlock(&(heap->lock));
>> return ret;
>>
>
> Something like that, yes. I will have to think through this a bit more,
> especially in light of your func_reentrancy splat :)
>
So, the reason splat in func_reentrancy test happens is as follows: the
above patch is sorta correct (i have a different one but does the same
thing), but incomplete. What happens then is when we add new memory, we
are integrating it into our existing malloc heap, which triggers
`malloc_elem_join_adjacent_free()` which will trigger a write into old
header space being merged, which may be marked as "freed". So, again we
are hit with our internal allocator messing with ASan.
To properly fix this is to answer the following question: what is the
goal of having ASan support in DPDK? Is it there to catch bugs *in the
allocator*, or can we just trust that our allocator code is correct, and
only concern ourselves with user-allocated areas of the code? Because it
seems like the best way to address this issue would be to just avoid
triggering ASan checks for certain allocator-internal actions: this way,
we don't need to care what allocator itself does, just what user code
does. As in, IIRC there was a compiler attribute that disables ASan
checks for a specific function: perhaps we could just wrap certain
access in that and be done with it?
What do you think?
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-26 12:54 ` Burakov, Anatoly
@ 2022-04-26 14:15 ` David Marchand
2022-04-26 16:07 ` Burakov, Anatoly
0 siblings, 1 reply; 18+ messages in thread
From: David Marchand @ 2022-04-26 14:15 UTC (permalink / raw)
To: Burakov, Anatoly
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On Tue, Apr 26, 2022 at 2:54 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
> >> @@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
> >>
> >> rte_mcfg_mem_write_unlock();
> >> free_unlock:
> >> - /* Poison memory range if belonging to some still mapped
> >> pages. */
> >> - if (!unmapped_pages)
> >> + if (!unmapped_pages) {
> >> asan_set_freezone(asan_ptr, asan_data_len);
> >> + } else {
> >> + /*
> >> + * We may be in a situation where we unmapped pages
> >> like this:
> >> + * malloc header | free space | unmapped space | free
> >> space | malloc header
> >> + */
> >> + void *free1_start = asan_ptr;
> >> + void *free1_end = aligned_start;
> >> + void *free2_start = RTE_PTR_ADD(aligned_start,
> >> aligned_len);
> >> + void *free2_end = RTE_PTR_ADD(asan_ptr, asan_data_len);
> >> +
> >> + if (free1_start < free1_end)
> >> + asan_set_freezone(free1_start,
> >> + RTE_PTR_DIFF(free1_end, free1_start));
> >> + if (free2_start < free2_end)
> >> + asan_set_freezone(free2_start,
> >> + RTE_PTR_DIFF(free2_end, free2_start));
> >> + }
> >>
> >> rte_spinlock_unlock(&(heap->lock));
> >> return ret;
> >>
> >
> > Something like that, yes. I will have to think through this a bit more,
> > especially in light of your func_reentrancy splat :)
> >
>
> So, the reason splat in func_reentrancy test happens is as follows: the
> above patch is sorta correct (i have a different one but does the same
> thing), but incomplete. What happens then is when we add new memory, we
> are integrating it into our existing malloc heap, which triggers
> `malloc_elem_join_adjacent_free()` which will trigger a write into old
> header space being merged, which may be marked as "freed". So, again we
> are hit with our internal allocator messing with ASan.
I ended up with the same conclusion.
Thanks for confirming.
>
> To properly fix this is to answer the following question: what is the
> goal of having ASan support in DPDK? Is it there to catch bugs *in the
> allocator*, or can we just trust that our allocator code is correct, and
> only concern ourselves with user-allocated areas of the code? Because it
The best would be to handle both.
I don't think clang disables ASan for the instrumentations on malloc.
> seems like the best way to address this issue would be to just avoid
> triggering ASan checks for certain allocator-internal actions: this way,
> we don't need to care what allocator itself does, just what user code
> does. As in, IIRC there was a compiler attribute that disables ASan
> checks for a specific function: perhaps we could just wrap certain
> access in that and be done with it?
>
> What do you think?
It is tempting because it is the easiest way to avoid the issue.
Though, by waiving those checks in the allocator, does it leave the
ASan shadow in a consistent state?
--
David Marchand
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-26 14:15 ` David Marchand
@ 2022-04-26 16:07 ` Burakov, Anatoly
2022-04-27 15:32 ` Burakov, Anatoly
0 siblings, 1 reply; 18+ messages in thread
From: Burakov, Anatoly @ 2022-04-26 16:07 UTC (permalink / raw)
To: David Marchand
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On 26-Apr-22 3:15 PM, David Marchand wrote:
> On Tue, Apr 26, 2022 at 2:54 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
>>>> @@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
>>>>
>>>> rte_mcfg_mem_write_unlock();
>>>> free_unlock:
>>>> - /* Poison memory range if belonging to some still mapped
>>>> pages. */
>>>> - if (!unmapped_pages)
>>>> + if (!unmapped_pages) {
>>>> asan_set_freezone(asan_ptr, asan_data_len);
>>>> + } else {
>>>> + /*
>>>> + * We may be in a situation where we unmapped pages
>>>> like this:
>>>> + * malloc header | free space | unmapped space | free
>>>> space | malloc header
>>>> + */
>>>> + void *free1_start = asan_ptr;
>>>> + void *free1_end = aligned_start;
>>>> + void *free2_start = RTE_PTR_ADD(aligned_start,
>>>> aligned_len);
>>>> + void *free2_end = RTE_PTR_ADD(asan_ptr, asan_data_len);
>>>> +
>>>> + if (free1_start < free1_end)
>>>> + asan_set_freezone(free1_start,
>>>> + RTE_PTR_DIFF(free1_end, free1_start));
>>>> + if (free2_start < free2_end)
>>>> + asan_set_freezone(free2_start,
>>>> + RTE_PTR_DIFF(free2_end, free2_start));
>>>> + }
>>>>
>>>> rte_spinlock_unlock(&(heap->lock));
>>>> return ret;
>>>>
>>>
>>> Something like that, yes. I will have to think through this a bit more,
>>> especially in light of your func_reentrancy splat :)
>>>
>>
>> So, the reason splat in func_reentrancy test happens is as follows: the
>> above patch is sorta correct (i have a different one but does the same
>> thing), but incomplete. What happens then is when we add new memory, we
>> are integrating it into our existing malloc heap, which triggers
>> `malloc_elem_join_adjacent_free()` which will trigger a write into old
>> header space being merged, which may be marked as "freed". So, again we
>> are hit with our internal allocator messing with ASan.
>
> I ended up with the same conclusion.
> Thanks for confirming.
>
>
>>
>> To properly fix this is to answer the following question: what is the
>> goal of having ASan support in DPDK? Is it there to catch bugs *in the
>> allocator*, or can we just trust that our allocator code is correct, and
>> only concern ourselves with user-allocated areas of the code? Because it
>
> The best would be to handle both.
> I don't think clang disables ASan for the instrumentations on malloc.
I've actually prototyped these changes a bit. We use memset in a few
places, and that one can't be disabled as far as i can tell (not without
blacklisting memset for entire DPDK).
>
>
>> seems like the best way to address this issue would be to just avoid
>> triggering ASan checks for certain allocator-internal actions: this way,
>> we don't need to care what allocator itself does, just what user code
>> does. As in, IIRC there was a compiler attribute that disables ASan
>> checks for a specific function: perhaps we could just wrap certain
>> access in that and be done with it?
>>
>> What do you think?
>
> It is tempting because it is the easiest way to avoid the issue.
> Though, by waiving those checks in the allocator, does it leave the
> ASan shadow in a consistent state?
>
The "consistent state" is kinda difficult to achieve because there is no
"default" state for memory - sometimes it comes as available (0x00),
sometimes it is marked as already freed (0xFF). So, coming into a malloc
function, we don't know whether the memory we're about to mess with is
0x00 or 0xFF.
What we could do is mark every malloc header with 0xFF regardless of its
status, and leave the rest to "regular" zoning. This would be strange
from ASan's point of view (because we're marking memory as "freed" when
it wasn't ever allocated), but at least this would be consistent :D
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mem: fix ASan shadow for remapped memory segments
2022-04-26 16:07 ` Burakov, Anatoly
@ 2022-04-27 15:32 ` Burakov, Anatoly
0 siblings, 0 replies; 18+ messages in thread
From: Burakov, Anatoly @ 2022-04-27 15:32 UTC (permalink / raw)
To: David Marchand
Cc: dev, Mcnamara, John, Dmitry Kozlyuk, dpdk stable, Xueqin Lin
On 26-Apr-22 5:07 PM, Burakov, Anatoly wrote:
> On 26-Apr-22 3:15 PM, David Marchand wrote:
>> On Tue, Apr 26, 2022 at 2:54 PM Burakov, Anatoly
>> <anatoly.burakov@intel.com> wrote:
>>>>> @@ -1040,9 +1040,25 @@ malloc_heap_free(struct malloc_elem *elem)
>>>>>
>>>>> rte_mcfg_mem_write_unlock();
>>>>> free_unlock:
>>>>> - /* Poison memory range if belonging to some still mapped
>>>>> pages. */
>>>>> - if (!unmapped_pages)
>>>>> + if (!unmapped_pages) {
>>>>> asan_set_freezone(asan_ptr, asan_data_len);
>>>>> + } else {
>>>>> + /*
>>>>> + * We may be in a situation where we unmapped pages
>>>>> like this:
>>>>> + * malloc header | free space | unmapped space | free
>>>>> space | malloc header
>>>>> + */
>>>>> + void *free1_start = asan_ptr;
>>>>> + void *free1_end = aligned_start;
>>>>> + void *free2_start = RTE_PTR_ADD(aligned_start,
>>>>> aligned_len);
>>>>> + void *free2_end = RTE_PTR_ADD(asan_ptr,
>>>>> asan_data_len);
>>>>> +
>>>>> + if (free1_start < free1_end)
>>>>> + asan_set_freezone(free1_start,
>>>>> + RTE_PTR_DIFF(free1_end, free1_start));
>>>>> + if (free2_start < free2_end)
>>>>> + asan_set_freezone(free2_start,
>>>>> + RTE_PTR_DIFF(free2_end, free2_start));
>>>>> + }
>>>>>
>>>>> rte_spinlock_unlock(&(heap->lock));
>>>>> return ret;
>>>>>
>>>>
>>>> Something like that, yes. I will have to think through this a bit more,
>>>> especially in light of your func_reentrancy splat :)
>>>>
>>>
>>> So, the reason splat in func_reentrancy test happens is as follows: the
>>> above patch is sorta correct (i have a different one but does the same
>>> thing), but incomplete. What happens then is when we add new memory, we
>>> are integrating it into our existing malloc heap, which triggers
>>> `malloc_elem_join_adjacent_free()` which will trigger a write into old
>>> header space being merged, which may be marked as "freed". So, again we
>>> are hit with our internal allocator messing with ASan.
>>
>> I ended up with the same conclusion.
>> Thanks for confirming.
>>
>>
>>>
>>> To properly fix this is to answer the following question: what is the
>>> goal of having ASan support in DPDK? Is it there to catch bugs *in the
>>> allocator*, or can we just trust that our allocator code is correct, and
>>> only concern ourselves with user-allocated areas of the code? Because it
>>
>> The best would be to handle both.
>> I don't think clang disables ASan for the instrumentations on malloc.
>
> I've actually prototyped these changes a bit. We use memset in a few
> places, and that one can't be disabled as far as i can tell (not without
> blacklisting memset for entire DPDK).
>
>>
>>
>>> seems like the best way to address this issue would be to just avoid
>>> triggering ASan checks for certain allocator-internal actions: this way,
>>> we don't need to care what allocator itself does, just what user code
>>> does. As in, IIRC there was a compiler attribute that disables ASan
>>> checks for a specific function: perhaps we could just wrap certain
>>> access in that and be done with it?
>>>
>>> What do you think?
>>
>> It is tempting because it is the easiest way to avoid the issue.
>> Though, by waiving those checks in the allocator, does it leave the
>> ASan shadow in a consistent state?
>>
>
> The "consistent state" is kinda difficult to achieve because there is no
> "default" state for memory - sometimes it comes as available (0x00),
> sometimes it is marked as already freed (0xFF). So, coming into a malloc
> function, we don't know whether the memory we're about to mess with is
> 0x00 or 0xFF.
>
> What we could do is mark every malloc header with 0xFF regardless of its
> status, and leave the rest to "regular" zoning. This would be strange
> from ASan's point of view (because we're marking memory as "freed" when
> it wasn't ever allocated), but at least this would be consistent :D
>
I've been prototyping a solution for this, but I keep bumping into our
dual usage of ASan: ASan doesn't differentiate between
allocator-internal accesses, and user code accesses. Therefore, we can't
either, so either we start marking areas as "accessible" even when they
shouldn't be (such as unallocated areas that correspond to malloc
headers), or we only use ASan to mark user-available areas and forego
its usage inside the allocator entirely.
Right now, the best I can think of is the combination of approaches
discussed earlier: that is, we mark all malloc element header areas as
"available" unconditionally (thereby sacrificing part of the protection
ASan provides us - because we can't prevent ASan from complaining about
accesses from inside the allocator without losing our ability to detect
cases where user accidentally accesses a malloc element), and we also
mark unmapped memory as "available" (because writing to it will trigger
a fault anyway).
I haven't yet figured out the cleanest solution (we miss asan zoning for
headers somewhere), but at least i got func reentrancy test to pass :D
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 0/2] Enable ASan in GHA
2022-04-15 17:31 [PATCH 0/3] Enable ASan in GHA David Marchand
` (2 preceding siblings ...)
2022-04-15 17:31 ` [PATCH 3/3] ci: build some job with ASan David Marchand
@ 2022-05-05 9:29 ` David Marchand
2022-05-05 9:29 ` [PATCH v2 1/2] test/mem: disable ASan when accessing unallocated mem David Marchand
` (2 more replies)
3 siblings, 3 replies; 18+ messages in thread
From: David Marchand @ 2022-05-05 9:29 UTC (permalink / raw)
To: dev; +Cc: john.mcnamara
Now that rte_malloc instrumentations are fixed, we can enable ASan in
GHA.
There are still some unit tests (relying on multiprocess) that can't
reliably run with ASan enabled. Those unit tests are skipped.
--
David Marchand
Changes since v1:
- dropped patch 2 in favor of Anatoly fix,
- rebased the last patch after other unit tests fixes have been merged,
David Marchand (2):
test/mem: disable ASan when accessing unallocated mem
ci: build some job with ASan
.ci/linux-build.sh | 8 ++
.github/workflows/build.yml | 3 +-
app/test/meson.build | 208 ++++++++++++++++++-----------------
app/test/test_memory.c | 5 +
lib/eal/common/malloc_elem.h | 10 +-
lib/eal/include/rte_common.h | 13 +++
6 files changed, 138 insertions(+), 109 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/2] test/mem: disable ASan when accessing unallocated mem
2022-05-05 9:29 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
@ 2022-05-05 9:29 ` David Marchand
2022-05-05 9:29 ` [PATCH v2 2/2] ci: build some job with ASan David Marchand
2022-05-11 12:01 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
2 siblings, 0 replies; 18+ messages in thread
From: David Marchand @ 2022-05-05 9:29 UTC (permalink / raw)
To: dev; +Cc: john.mcnamara, stable, Anatoly Burakov, Xueqin Lin, Zhihong Peng
As described in bugzilla, ASan reports accesses to all memory segment as
invalid, since those parts have not been allocated.
Move __rte_no_asan to rte_common.h and disable ASan on a part of the test.
Bugzilla ID: 880
Fixes: 6cc51b1293ce ("mem: instrument allocator for ASan")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
app/test/test_memory.c | 5 +++++
lib/eal/common/malloc_elem.h | 10 ++--------
lib/eal/include/rte_common.h | 13 +++++++++++++
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/app/test/test_memory.c b/app/test/test_memory.c
index 140ac3f3cf..440e5ef838 100644
--- a/app/test/test_memory.c
+++ b/app/test/test_memory.c
@@ -25,6 +25,11 @@
* - Try to read all memory; it should not segfault.
*/
+/*
+ * ASan complains about accessing unallocated memory.
+ * See: https://bugs.dpdk.org/show_bug.cgi?id=880
+ */
+__rte_no_asan
static int
check_mem(const struct rte_memseg_list *msl __rte_unused,
const struct rte_memseg *ms, void *arg __rte_unused)
diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
index c5f65895e1..952ce7343b 100644
--- a/lib/eal/common/malloc_elem.h
+++ b/lib/eal/common/malloc_elem.h
@@ -7,6 +7,8 @@
#include <stdbool.h>
+#include <rte_common.h>
+
#define MIN_DATA_SIZE (RTE_CACHE_LINE_SIZE)
/* dummy definition of struct so we can use pointers to it in malloc_elem struct */
@@ -131,12 +133,6 @@ malloc_elem_cookies_ok(const struct malloc_elem *elem)
#define ASAN_MEM_TO_SHADOW(mem) \
RTE_PTR_ADD(ASAN_MEM_SHIFT(mem), ASAN_SHADOW_OFFSET)
-#if defined(__clang__)
-#define __rte_no_asan __attribute__((no_sanitize("address", "hwaddress")))
-#else
-#define __rte_no_asan __attribute__((no_sanitize_address))
-#endif
-
__rte_no_asan
static inline void
asan_set_shadow(void *addr, char val)
@@ -276,8 +272,6 @@ old_malloc_size(struct malloc_elem *elem)
#else /* !RTE_MALLOC_ASAN */
-#define __rte_no_asan
-
static inline void
asan_set_zone(void *ptr __rte_unused, size_t len __rte_unused,
uint32_t val __rte_unused) { }
diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index 67587025ab..d56a7570c0 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -267,6 +267,19 @@ static void __attribute__((destructor(RTE_PRIO(prio)), used)) func(void)
*/
#define __rte_cold __attribute__((cold))
+/**
+ * Disable AddressSanitizer on some code
+ */
+#ifdef RTE_MALLOC_ASAN
+#ifdef RTE_CC_CLANG
+#define __rte_no_asan __attribute__((no_sanitize("address", "hwaddress")))
+#else
+#define __rte_no_asan __attribute__((no_sanitize_address))
+#endif
+#else /* ! RTE_MALLOC_ASAN */
+#define __rte_no_asan
+#endif
+
/*********** Macros for pointer arithmetic ********/
/**
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/2] ci: build some job with ASan
2022-05-05 9:29 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
2022-05-05 9:29 ` [PATCH v2 1/2] test/mem: disable ASan when accessing unallocated mem David Marchand
@ 2022-05-05 9:29 ` David Marchand
2022-05-10 19:22 ` Aaron Conole
2022-05-11 12:01 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
2 siblings, 1 reply; 18+ messages in thread
From: David Marchand @ 2022-05-05 9:29 UTC (permalink / raw)
To: dev; +Cc: john.mcnamara, Aaron Conole, Michael Santana
Enable ASan, this can greatly help identify leaks and buffer overflows.
Running unit tests relying on multiprocess is unreliable with ASan
enabled, so skip them.
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changes since v1:
- reinstated table_autotest in "ASan-safe" list of ut,
---
.ci/linux-build.sh | 8 ++
.github/workflows/build.yml | 3 +-
app/test/meson.build | 208 +++++++++++++++++++-----------------
3 files changed, 118 insertions(+), 101 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 774a1441bf..93706c0131 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -95,6 +95,14 @@ if [ "$MINI" = "true" ]; then
OPTS="$OPTS -Denable_drivers=net/null"
OPTS="$OPTS -Ddisable_libs=*"
fi
+
+if [ "$ASAN" = "true" ]; then
+ OPTS="$OPTS -Db_sanitize=address"
+ if [ "${CC%%clang}" != "$CC" ]; then
+ OPTS="$OPTS -Db_lundef=false"
+ fi
+fi
+
meson build --werror $OPTS
ninja -C build
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 22daaabb91..45871e76ed 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -16,6 +16,7 @@ jobs:
env:
AARCH64: ${{ matrix.config.cross == 'aarch64' }}
ABI_CHECKS: ${{ contains(matrix.config.checks, 'abi') }}
+ ASAN: ${{ contains(matrix.config.checks, 'asan') }}
BUILD_32BIT: ${{ matrix.config.cross == 'i386' }}
BUILD_DOCS: ${{ contains(matrix.config.checks, 'doc') }}
CC: ccache ${{ matrix.config.compiler }}
@@ -47,7 +48,7 @@ jobs:
- os: ubuntu-18.04
compiler: clang
library: shared
- checks: doc+tests
+ checks: asan+doc+tests
- os: ubuntu-18.04
compiler: gcc
library: static
diff --git a/app/test/meson.build b/app/test/meson.build
index 5fc1dd1b7b..bb4621ed2a 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -149,96 +149,97 @@ test_deps = enabled_libs
# as well as libs, the pci and vdev bus drivers are needed for a lot of tests
test_deps += ['bus_pci', 'bus_vdev']
-# Each test is marked with flag true/false
-# to indicate whether it can run in no-huge mode.
+# Each test is marked with flags:
+# - the first flag indicates whether the test can run in no-huge mode,
+# - the second flag indicates whether the test can run with ASan enabled,
fast_tests = [
- ['acl_autotest', true],
- ['atomic_autotest', false],
- ['bitmap_autotest', true],
- ['bpf_autotest', true],
- ['bpf_convert_autotest', true],
- ['bitops_autotest', true],
- ['byteorder_autotest', true],
- ['cksum_autotest', true],
- ['cmdline_autotest', true],
- ['common_autotest', true],
- ['cpuflags_autotest', true],
- ['debug_autotest', true],
- ['devargs_autotest', true],
- ['eal_flags_c_opt_autotest', false],
- ['eal_flags_main_opt_autotest', false],
- ['eal_flags_n_opt_autotest', false],
- ['eal_flags_hpet_autotest', false],
- ['eal_flags_no_huge_autotest', false],
- ['eal_flags_a_opt_autotest', false],
- ['eal_flags_b_opt_autotest', false],
- ['eal_flags_vdev_opt_autotest', false],
- ['eal_flags_r_opt_autotest', false],
- ['eal_flags_mem_autotest', false],
- ['eal_flags_file_prefix_autotest', false],
- ['eal_flags_misc_autotest', false],
- ['eal_fs_autotest', true],
- ['errno_autotest', true],
- ['ethdev_link_status', true],
- ['event_ring_autotest', true],
- ['fib_autotest', true],
- ['fib6_autotest', true],
- ['func_reentrancy_autotest', false],
- ['hash_autotest', true],
- ['interrupt_autotest', true],
- ['ipfrag_autotest', false],
- ['lcores_autotest', true],
- ['logs_autotest', true],
- ['lpm_autotest', true],
- ['lpm6_autotest', true],
- ['malloc_autotest', false],
- ['mbuf_autotest', false],
- ['mcslock_autotest', false],
- ['memcpy_autotest', true],
- ['memory_autotest', false],
- ['mempool_autotest', false],
- ['memzone_autotest', false],
- ['meter_autotest', true],
- ['multiprocess_autotest', false],
- ['per_lcore_autotest', true],
- ['pflock_autotest', true],
- ['prefetch_autotest', true],
- ['rcu_qsbr_autotest', true],
- ['pie_autotest', true],
- ['rib_autotest', true],
- ['rib6_autotest', true],
- ['ring_autotest', true],
- ['rwlock_test1_autotest', true],
- ['rwlock_rda_autotest', true],
- ['rwlock_rds_wrm_autotest', true],
- ['rwlock_rde_wro_autotest', true],
- ['sched_autotest', true],
- ['security_autotest', false],
- ['spinlock_autotest', true],
- ['stack_autotest', false],
- ['stack_lf_autotest', false],
- ['string_autotest', true],
- ['tailq_autotest', true],
- ['ticketlock_autotest', true],
- ['timer_autotest', false],
- ['user_delay_us', true],
- ['version_autotest', true],
- ['crc_autotest', true],
- ['distributor_autotest', false],
- ['eventdev_common_autotest', true],
- ['fbarray_autotest', true],
- ['hash_readwrite_func_autotest', false],
- ['ipsec_autotest', true],
- ['kni_autotest', false],
- ['kvargs_autotest', true],
- ['member_autotest', true],
- ['power_cpufreq_autotest', false],
- ['power_autotest', true],
- ['power_kvm_vm_autotest', false],
- ['reorder_autotest', true],
- ['service_autotest', true],
- ['thash_autotest', true],
- ['trace_autotest', true],
+ ['acl_autotest', true, true],
+ ['atomic_autotest', false, true],
+ ['bitmap_autotest', true, true],
+ ['bpf_autotest', true, true],
+ ['bpf_convert_autotest', true, true],
+ ['bitops_autotest', true, true],
+ ['byteorder_autotest', true, true],
+ ['cksum_autotest', true, true],
+ ['cmdline_autotest', true, true],
+ ['common_autotest', true, true],
+ ['cpuflags_autotest', true, true],
+ ['debug_autotest', true, true],
+ ['devargs_autotest', true, true],
+ ['eal_flags_c_opt_autotest', false, false],
+ ['eal_flags_main_opt_autotest', false, false],
+ ['eal_flags_n_opt_autotest', false, false],
+ ['eal_flags_hpet_autotest', false, false],
+ ['eal_flags_no_huge_autotest', false, false],
+ ['eal_flags_a_opt_autotest', false, false],
+ ['eal_flags_b_opt_autotest', false, false],
+ ['eal_flags_vdev_opt_autotest', false, false],
+ ['eal_flags_r_opt_autotest', false, false],
+ ['eal_flags_mem_autotest', false, false],
+ ['eal_flags_file_prefix_autotest', false, false],
+ ['eal_flags_misc_autotest', false, false],
+ ['eal_fs_autotest', true, true],
+ ['errno_autotest', true, true],
+ ['ethdev_link_status', true, true],
+ ['event_ring_autotest', true, true],
+ ['fib_autotest', true, true],
+ ['fib6_autotest', true, true],
+ ['func_reentrancy_autotest', false, true],
+ ['hash_autotest', true, true],
+ ['interrupt_autotest', true, true],
+ ['ipfrag_autotest', false, true],
+ ['lcores_autotest', true, true],
+ ['logs_autotest', true, true],
+ ['lpm_autotest', true, true],
+ ['lpm6_autotest', true, true],
+ ['malloc_autotest', false, true],
+ ['mbuf_autotest', false, true],
+ ['mcslock_autotest', false, true],
+ ['memcpy_autotest', true, true],
+ ['memory_autotest', false, true],
+ ['mempool_autotest', false, true],
+ ['memzone_autotest', false, true],
+ ['meter_autotest', true, true],
+ ['multiprocess_autotest', false, false],
+ ['per_lcore_autotest', true, true],
+ ['pflock_autotest', true, true],
+ ['prefetch_autotest', true, true],
+ ['rcu_qsbr_autotest', true, true],
+ ['pie_autotest', true, true],
+ ['rib_autotest', true, true],
+ ['rib6_autotest', true, true],
+ ['ring_autotest', true, true],
+ ['rwlock_test1_autotest', true, true],
+ ['rwlock_rda_autotest', true, true],
+ ['rwlock_rds_wrm_autotest', true, true],
+ ['rwlock_rde_wro_autotest', true, true],
+ ['sched_autotest', true, true],
+ ['security_autotest', false, true],
+ ['spinlock_autotest', true, true],
+ ['stack_autotest', false, true],
+ ['stack_lf_autotest', false, true],
+ ['string_autotest', true, true],
+ ['tailq_autotest', true, true],
+ ['ticketlock_autotest', true, true],
+ ['timer_autotest', false, true],
+ ['user_delay_us', true, true],
+ ['version_autotest', true, true],
+ ['crc_autotest', true, true],
+ ['distributor_autotest', false, true],
+ ['eventdev_common_autotest', true, true],
+ ['fbarray_autotest', true, true],
+ ['hash_readwrite_func_autotest', false, true],
+ ['ipsec_autotest', true, true],
+ ['kni_autotest', false, true],
+ ['kvargs_autotest', true, true],
+ ['member_autotest', true, true],
+ ['power_cpufreq_autotest', false, true],
+ ['power_autotest', true, true],
+ ['power_kvm_vm_autotest', false, true],
+ ['reorder_autotest', true, true],
+ ['service_autotest', true, true],
+ ['thash_autotest', true, true],
+ ['trace_autotest', true, true],
]
# Tests known to have issues or which don't belong in other tests lists.
@@ -345,15 +346,16 @@ endif
if dpdk_conf.has('RTE_LIB_FLOW_CLASSIFY')
test_sources += 'test_flow_classify.c'
- fast_tests += [['flow_classify_autotest', false]]
+ fast_tests += [['flow_classify_autotest', false, true]]
endif
if dpdk_conf.has('RTE_LIB_METRICS')
test_sources += ['test_metrics.c']
- fast_tests += [['metrics_autotest', true]]
+ fast_tests += [['metrics_autotest', true, true]]
endif
if not is_windows and dpdk_conf.has('RTE_LIB_TELEMETRY')
test_sources += ['test_telemetry_json.c', 'test_telemetry_data.c']
- fast_tests += [['telemetry_json_autotest', true], ['telemetry_data_autotest', true]]
+ fast_tests += [['telemetry_json_autotest', true, true]]
+ fast_tests += [['telemetry_data_autotest', true, true]]
endif
if dpdk_conf.has('RTE_LIB_PIPELINE')
# pipeline lib depends on port and table libs, so those must be present
@@ -366,7 +368,7 @@ if dpdk_conf.has('RTE_LIB_PIPELINE')
'test_table_ports.c',
'test_table_tables.c',
]
- fast_tests += [['table_autotest', true]]
+ fast_tests += [['table_autotest', true, true]]
endif
# The following linkages of drivers are required because
@@ -386,26 +388,26 @@ if dpdk_conf.has('RTE_NET_RING')
test_sources += 'test_pmd_ring.c'
test_sources += 'test_event_eth_tx_adapter.c'
test_sources += 'sample_packet_forward.c'
- fast_tests += [['ring_pmd_autotest', true]]
+ fast_tests += [['ring_pmd_autotest', true, true]]
perf_test_names += 'ring_pmd_perf_autotest'
- fast_tests += [['event_eth_tx_adapter_autotest', false]]
+ fast_tests += [['event_eth_tx_adapter_autotest', false, true]]
if dpdk_conf.has('RTE_LIB_BITRATESTATS')
test_sources += 'test_bitratestats.c'
- fast_tests += [['bitratestats_autotest', true]]
+ fast_tests += [['bitratestats_autotest', true, true]]
endif
if dpdk_conf.has('RTE_LIB_LATENCYSTATS')
test_sources += 'test_latencystats.c'
- fast_tests += [['latencystats_autotest', true]]
+ fast_tests += [['latencystats_autotest', true, true]]
endif
if dpdk_conf.has('RTE_LIB_PDUMP')
test_sources += 'test_pdump.c'
- fast_tests += [['pdump_autotest', true]]
+ fast_tests += [['pdump_autotest', true, false]]
endif
endif
if dpdk_conf.has('RTE_NET_NULL')
test_deps += 'net_null'
test_sources += 'test_vdev.c'
- fast_tests += [['vdev_autotest', true]]
+ fast_tests += [['vdev_autotest', true, true]]
endif
if dpdk_conf.has('RTE_HAS_LIBPCAP')
@@ -431,7 +433,7 @@ if dpdk_conf.has('RTE_LIB_COMPRESSDEV')
if compress_test_dep.found()
test_dep_objs += compress_test_dep
test_sources += 'test_compressdev.c'
- fast_tests += [['compressdev_autotest', false]]
+ fast_tests += [['compressdev_autotest', false, true]]
endif
endif
@@ -478,6 +480,12 @@ foreach arg : fast_tests
endif
endif
+ if get_option('b_sanitize') == 'address' or get_option('b_sanitize') == 'address,undefined'
+ if not arg[2]
+ run_test = false
+ endif
+ endif
+
if (get_option('default_library') == 'shared' and
arg[0] == 'event_eth_tx_adapter_autotest')
foreach drv:dpdk_drivers
--
2.23.0
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/2] ci: build some job with ASan
2022-05-05 9:29 ` [PATCH v2 2/2] ci: build some job with ASan David Marchand
@ 2022-05-10 19:22 ` Aaron Conole
0 siblings, 0 replies; 18+ messages in thread
From: Aaron Conole @ 2022-05-10 19:22 UTC (permalink / raw)
To: David Marchand; +Cc: dev, john.mcnamara, Michael Santana
David Marchand <david.marchand@redhat.com> writes:
> Enable ASan, this can greatly help identify leaks and buffer overflows.
> Running unit tests relying on multiprocess is unreliable with ASan
> enabled, so skip them.
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - reinstated table_autotest in "ASan-safe" list of ut,
This is great!
Acked-by: Aaron Conole <aconole@redhat.com>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/2] Enable ASan in GHA
2022-05-05 9:29 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
2022-05-05 9:29 ` [PATCH v2 1/2] test/mem: disable ASan when accessing unallocated mem David Marchand
2022-05-05 9:29 ` [PATCH v2 2/2] ci: build some job with ASan David Marchand
@ 2022-05-11 12:01 ` David Marchand
2 siblings, 0 replies; 18+ messages in thread
From: David Marchand @ 2022-05-11 12:01 UTC (permalink / raw)
To: dev; +Cc: Mcnamara, John, Burakov, Anatoly, Aaron Conole
On Thu, May 5, 2022 at 11:30 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> Now that rte_malloc instrumentations are fixed, we can enable ASan in
> GHA.
> There are still some unit tests (relying on multiprocess) that can't
> reliably run with ASan enabled. Those unit tests are skipped.
>
> --
> David Marchand
>
> Changes since v1:
> - dropped patch 2 in favor of Anatoly fix,
> - rebased the last patch after other unit tests fixes have been merged,
>
> David Marchand (2):
> test/mem: disable ASan when accessing unallocated mem
> ci: build some job with ASan
>
Series applied.
Thanks for the reviews.
--
David Marchand
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-05-11 12:01 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 17:31 [PATCH 0/3] Enable ASan in GHA David Marchand
2022-04-15 17:31 ` [PATCH 1/3] test/mem: disable ASan when accessing unallocated mem David Marchand
2022-04-20 14:48 ` Burakov, Anatoly
2022-04-15 17:31 ` [PATCH 2/3] mem: fix ASan shadow for remapped memory segments David Marchand
2022-04-20 14:47 ` Burakov, Anatoly
2022-04-21 9:37 ` David Marchand
2022-04-21 9:50 ` David Marchand
2022-04-21 13:18 ` Burakov, Anatoly
2022-04-26 12:54 ` Burakov, Anatoly
2022-04-26 14:15 ` David Marchand
2022-04-26 16:07 ` Burakov, Anatoly
2022-04-27 15:32 ` Burakov, Anatoly
2022-04-15 17:31 ` [PATCH 3/3] ci: build some job with ASan David Marchand
2022-05-05 9:29 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
2022-05-05 9:29 ` [PATCH v2 1/2] test/mem: disable ASan when accessing unallocated mem David Marchand
2022-05-05 9:29 ` [PATCH v2 2/2] ci: build some job with ASan David Marchand
2022-05-10 19:22 ` Aaron Conole
2022-05-11 12:01 ` [PATCH v2 0/2] Enable ASan in GHA David Marchand
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 NNTP newsgroup(s).