Re: [PATCH] eal/linux: unregister alarm callback before free ptr

[David Marchand <david.marchand@redhat.com>]

On Tue, May 20, 2025 at 5:08 PM Rui Ferreira
<rui.ferreira1@h-partners.com> wrote:
>
> This was flagged by Address sanitizer as a use after free. The
> intr_handle ptr is shared between the main thread and the interrupt
> thread, and the interrupt thread can dereference the ptr after free
> is called when the main thread cleans up (from the alarm callback).
>
> The interrupt thread never terminates (eal_intr_thread_main) so
> use rte_intr_callback_unregister_sync during cleanup to
> ensure the callback is removed before freeing the ptr.
>
> To be more defensive clear out the pointer and registration
> variable if we can unregister.
>
> Bugzilla ID: 1683
>
> Signed-off-by: Rui Ferreira <rui.ferreira1@h-partners.com>

I remember mentioning that other OS may be affected by the bug.
Please consider fixing this issue for Windows and FreeBSD too.


> ---
>  .mailmap                  | 1 +
>  lib/eal/linux/eal_alarm.c | 9 ++++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index d8439b79ce..907c5ea967 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1332,6 +1332,7 @@ Rosen Xu <rosen.xu@altera.com> <rosen.xu@intel.com>
>  Roy Franz <roy.franz@cavium.com>
>  Roy Pledge <roy.pledge@nxp.com>
>  Roy Shterman <roy.shterman@vastdata.com>
> +Rui Ferreira <rui.ferreira1@h-partners.com>
>  Ruifeng Wang <ruifeng.wang@arm.com>
>  Rushil Gupta <rushilg@google.com>
>  Ryan E Hall <ryan.e.hall@intel.com>
> diff --git a/lib/eal/linux/eal_alarm.c b/lib/eal/linux/eal_alarm.c
> index b216a007a3..eb6a21d4f0 100644
> --- a/lib/eal/linux/eal_alarm.c
> +++ b/lib/eal/linux/eal_alarm.c
> @@ -57,7 +57,14 @@ static void eal_alarm_callback(void *arg);
>  void
>  rte_eal_alarm_cleanup(void)
>  {
> -       rte_intr_instance_free(intr_handle);
> +       /* unregister callback using intr_handle in interrupt thread */
> +       int ret = rte_intr_callback_unregister_sync(intr_handle,
> +                                               eal_alarm_callback, (void *)-1);
> +       if (ret >= 0) {
> +               rte_intr_instance_free(intr_handle);
> +               intr_handle = NULL;
> +               handler_registered = 0;
> +       }
>  }
>
>  int

In rte_eal_cleanup, the trace framework is uninitialised prior to
rte_eal_alarm_cleanup().

And the CI caught this issue (see ovsrobot report in patchwork).

ERROR: AddressSanitizer: heap-use-after-free on address 0x7f872ca90f80
at pc 0x7f873db9fb1e bp 0x7fffc98a3720 sp 0x7fffc98a3718
READ of size 4 at 0x7f872ca90f80 thread T0
    #0 0x7f873db9fb1d in __rte_trace_mem_get
/home/runner/work/dpdk/dpdk/build/../lib/eal/include/rte_trace_point.h:331:27
    #1 0x7f873db9fb1d in rte_eal_trace_intr_callback_unregister
/home/runner/work/dpdk/dpdk/build/../lib/eal/include/eal_trace_internal.h:58:1
    #2 0x7f873db9fb1d in rte_intr_callback_unregister
/home/runner/work/dpdk/dpdk/build/../lib/eal/linux/eal_interrupts.c:685:2
    #3 0x7f873db9fe44 in rte_intr_callback_unregister_sync
/home/runner/work/dpdk/dpdk/build/../lib/eal/linux/eal_interrupts.c:697:16
    #4 0x7f873db996ad in rte_eal_alarm_cleanup
/home/runner/work/dpdk/dpdk/build/../lib/eal/linux/eal_alarm.c:61:12
    #5 0x7f873db98b4f in rte_eal_cleanup
/home/runner/work/dpdk/dpdk/build/../lib/eal/linux/eal.c:1334:2
    #6 0x561c211092db in main
/home/runner/work/dpdk/dpdk/build/../app/test/test.c:263:2
    #7 0x7f873c429d8f in __libc_start_call_main
csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #8 0x7f873c429e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #9 0x561c2104a534 in _start
(/home/runner/work/dpdk/dpdk/build/app/dpdk-test+0x1fc534) (BuildId:
37b8659fbf174b3d78222edbdcd9f2f6a3027aff)

Address 0x7f872ca90f80 is a wild pointer inside of access range of
size 0x000000000004.
SUMMARY: AddressSanitizer: heap-use-after-free
/home/runner/work/dpdk/dpdk/build/../lib/eal/include/rte_trace_point.h:331:27
in __rte_trace_mem_get
Shadow bytes around the buggy address:
  0x0ff16594a1a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a1b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a1c0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a1d0: fd fd fd fd fd fd fd fd 00 00 00 00 00 00 00 00
  0x0ff16594a1e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ff16594a1f0:[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a200: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a210: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a220: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a230: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0ff16594a240: 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):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==55106==ABORTING


-- 
David Marchand