[v4] lib/eal: fix segfaults due to thread exit order
Checks
Commit Message
From: Zhichao Zeng <zhichaox.zeng@intel.com>
The eal-intr-thread is not closed before memory cleanup in the
process of exiting. There is a small probability that when the
eal-intr-thread is about to use some pointers, the memory were
just cleaned, which cause the segment fault error caught by ASan.
This patch close the eal-intr-thread before memory cleanup when
exiting to avoid segment fault. And add some atomic operations
to avoid executing rte_eal_cleanup in the child process spawned
by fork() in some test cases, e.g. debug_autotest of dpdk-test.
Cc: stable@dpdk.org
---
v2:
add the same API for FreeBSD
---
v3:
fix rte_eal_cleanup crash in debug_autotest
---
v4:
shorten the prompt message and optimize the commit log
Suggested-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
---
lib/eal/common/eal_private.h | 7 +++++++
lib/eal/freebsd/eal.c | 21 ++++++++++++++++++++-
lib/eal/freebsd/eal_interrupts.c | 12 ++++++++++++
lib/eal/linux/eal.c | 20 +++++++++++++++++++-
lib/eal/linux/eal_interrupts.c | 12 ++++++++++++
5 files changed, 70 insertions(+), 2 deletions(-)
Comments
Hi David, Harman
Please review this patch at your convenience, it's been about a month since the v1 version.
Thanks!
Best regards
Zhichao
> -----Original Message-----
> From: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Sent: Wednesday, June 15, 2022 2:02 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; david.marchand@redhat.com; stephen@networkplumber.org; mb@smartsharesystems.com; Zeng, ZhichaoX <zhichaox.zeng@intel.com>; Richardson, Bruce > <bruce.richardson@intel.com>; Harman Kalra <hkalra@marvell.com>
> Subject: [PATCH v4] lib/eal: fix segfaults due to thread exit order
>
> From: Zhichao Zeng <zhichaox.zeng@intel.com>
>
> The eal-intr-thread is not closed before memory cleanup in the process of exiting. There is a small probability that when the eal-intr-thread is about to use some pointers, the memory were just cleaned, which cause the > segment fault error caught by ASan.
>
> This patch close the eal-intr-thread before memory cleanup when exiting to avoid segment fault. And add some atomic operations to avoid executing rte_eal_cleanup in the child process spawned by fork() in some test > cases, e.g. debug_autotest of dpdk-test.
>
> Cc: stable@dpdk.org
>
> ---
> v2:
> add the same API for FreeBSD
> ---
> v3:
> fix rte_eal_cleanup crash in debug_autotest
> ---
> v4:
> shorten the prompt message and optimize the commit log
>
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
On Fri, Jun 24, 2022 at 3:42 AM Zeng, ZhichaoX <zhichaox.zeng@intel.com> wrote:
>
> Hi David, Harman
> Please review this patch at your convenience, it's been about a month since the v1 version.
> Thanks!
Yes, it's been a month.
Feel free to help on reviewing patches from others so that reviews can
be quicker for everyone.
Thanks.
Hi Harman, Bruce
Could you please help to review this patch, thank you so much!
Best regards
Zhichao
> -----Original Message-----
> From: Zeng, ZhichaoX <zhichaox.zeng@intel.com>
> Sent: Wednesday, June 15, 2022 2:02 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Yang, Qiming <qiming.yang@intel.com>; david.marchand@redhat.com; stephen@networkplumber.org; mb@smartsharesystems.com; Zeng, ZhichaoX <zhichaox.zeng@intel.com>; Richardson, Bruce > <bruce.richardson@intel.com>; Harman Kalra <hkalra@marvell.com>
> Subject: [PATCH v4] lib/eal: fix segfaults due to thread exit order
>
> From: Zhichao Zeng <zhichaox.zeng@intel.com>
>
> The eal-intr-thread is not closed before memory cleanup in the process of exiting. There is a small probability that when the eal-intr-thread is about to use some pointers, the memory were just cleaned, which cause the > segment fault error caught by ASan.
>
> This patch close the eal-intr-thread before memory cleanup when exiting to avoid segment fault. And add some atomic operations to avoid executing rte_eal_cleanup in the child process spawned by fork() in some test > cases, e.g. debug_autotest of dpdk-test.
>
> Cc: stable@dpdk.org
>
> ---
> v2:
> add the same API for FreeBSD
> ---
> v3:
> fix rte_eal_cleanup crash in debug_autotest
> ---
> v4:
> shorten the prompt message and optimize the commit log
>
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
On Wed, Jun 15, 2022 at 02:01:54PM +0800, zhichaox.zeng@intel.com wrote:
> From: Zhichao Zeng <zhichaox.zeng@intel.com>
>
> The eal-intr-thread is not closed before memory cleanup in the
> process of exiting. There is a small probability that when the
> eal-intr-thread is about to use some pointers, the memory were
> just cleaned, which cause the segment fault error caught by ASan.
>
> This patch close the eal-intr-thread before memory cleanup when
> exiting to avoid segment fault. And add some atomic operations
> to avoid executing rte_eal_cleanup in the child process spawned
> by fork() in some test cases, e.g. debug_autotest of dpdk-test.
>
> Cc: stable@dpdk.org
>
Hi,
some comments inline below.
/Bruce
> ---
> v2:
> add the same API for FreeBSD
> ---
> v3:
> fix rte_eal_cleanup crash in debug_autotest
> ---
> v4:
> shorten the prompt message and optimize the commit log
>
Please put these updates below the cutline after the sign-offs, i.e.
immediately before the diffstat.
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Zhichao Zeng <zhichaox.zeng@intel.com>
> ---
> lib/eal/common/eal_private.h | 7 +++++++
> lib/eal/freebsd/eal.c | 21 ++++++++++++++++++++-
> lib/eal/freebsd/eal_interrupts.c | 12 ++++++++++++
> lib/eal/linux/eal.c | 20 +++++++++++++++++++-
> lib/eal/linux/eal_interrupts.c | 12 ++++++++++++
> 5 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
> index 44d14241f0..7adf41b7d7 100644
> --- a/lib/eal/common/eal_private.h
> +++ b/lib/eal/common/eal_private.h
> @@ -152,6 +152,13 @@ int rte_eal_tailqs_init(void);
> */
> int rte_eal_intr_init(void);
>
> +/**
> + * Destroy interrupt handling thread.
> + *
> + * This function is private to EAL.
> + */
> +void rte_eal_intr_destroy(void);
> +
> /**
> * Close the default log stream
> *
> diff --git a/lib/eal/freebsd/eal.c b/lib/eal/freebsd/eal.c
> index a6b20960f2..4882f27abd 100644
> --- a/lib/eal/freebsd/eal.c
> +++ b/lib/eal/freebsd/eal.c
> @@ -72,6 +72,8 @@ struct lcore_config lcore_config[RTE_MAX_LCORE];
> /* used by rte_rdtsc() */
> int rte_cycles_vmware_tsc_map;
>
> +/* used to judge the running status of the eal */
> +static uint32_t run_once;
>
I don't like just moving this variable from the eal_init function. When in
eal_init the name "run_once" made sense as it tracked how often the EAL
init function was run. However, now as a global variable the name
"run_once" no longer makes sense.
Two suggestions:
1. Keep run_once in EAL init as-is, and use a different variable or value
to indicate that DPDK is initialized for cleanup.
2. Move the variable as you have here, just rename it to a more meaningful
name.
> int
> eal_clean_runtime_dir(void)
> @@ -574,12 +576,22 @@ static void rte_eal_init_alert(const char *msg)
> RTE_LOG(ERR, EAL, "%s\n", msg);
> }
>
> +static void warn_parent(void)
> +{
> + RTE_LOG(WARNING, EAL, "DPDK won't work in the child process\n");
> +}
I wonder if this contains enough information. Can we identify briefly what
parts will or won't work, or if we just want to deny everything, can we
give a brief reason why?
> +
> +static void scratch_child(void)
> +{
> + /* Scratch run_once so that a call to rte_eal_cleanup won't crash... */
> + __atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
> +}
> +
I think the name of this function needs improvement. I'm not sure that
"scratch" is the best term to use. Something like "clear_eal_flag" is
probably better.
> /* Launch threads, called at application init(). */
> int
> rte_eal_init(int argc, char **argv)
> {
> int i, fctret, ret;
> - static uint32_t run_once;
> uint32_t has_run = 0;
> char cpuset[RTE_CPU_AFFINITY_STR_LEN];
> char thread_name[RTE_MAX_THREAD_NAME_LEN];
> @@ -883,6 +895,8 @@ rte_eal_init(int argc, char **argv)
>
> eal_mcfg_complete();
>
> + pthread_atfork(NULL, warn_parent, scratch_child);
> +
> return fctret;
> }
>
> @@ -891,8 +905,13 @@ rte_eal_cleanup(void)
> {
> struct internal_config *internal_conf =
> eal_get_internal_configuration();
> +
> + if (__atomic_load_n(&run_once, __ATOMIC_RELAXED) == 0)
> + return 0;
> +
> rte_service_finalize();
> rte_mp_channel_cleanup();
> + rte_eal_intr_destroy();
> /* after this point, any DPDK pointers will become dangling */
> rte_eal_memory_detach();
> rte_eal_alarm_cleanup();
<snip for brevity>
@@ -152,6 +152,13 @@ int rte_eal_tailqs_init(void);
*/
int rte_eal_intr_init(void);
+/**
+ * Destroy interrupt handling thread.
+ *
+ * This function is private to EAL.
+ */
+void rte_eal_intr_destroy(void);
+
/**
* Close the default log stream
*
@@ -72,6 +72,8 @@ struct lcore_config lcore_config[RTE_MAX_LCORE];
/* used by rte_rdtsc() */
int rte_cycles_vmware_tsc_map;
+/* used to judge the running status of the eal */
+static uint32_t run_once;
int
eal_clean_runtime_dir(void)
@@ -574,12 +576,22 @@ static void rte_eal_init_alert(const char *msg)
RTE_LOG(ERR, EAL, "%s\n", msg);
}
+static void warn_parent(void)
+{
+ RTE_LOG(WARNING, EAL, "DPDK won't work in the child process\n");
+}
+
+static void scratch_child(void)
+{
+ /* Scratch run_once so that a call to rte_eal_cleanup won't crash... */
+ __atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
+}
+
/* Launch threads, called at application init(). */
int
rte_eal_init(int argc, char **argv)
{
int i, fctret, ret;
- static uint32_t run_once;
uint32_t has_run = 0;
char cpuset[RTE_CPU_AFFINITY_STR_LEN];
char thread_name[RTE_MAX_THREAD_NAME_LEN];
@@ -883,6 +895,8 @@ rte_eal_init(int argc, char **argv)
eal_mcfg_complete();
+ pthread_atfork(NULL, warn_parent, scratch_child);
+
return fctret;
}
@@ -891,8 +905,13 @@ rte_eal_cleanup(void)
{
struct internal_config *internal_conf =
eal_get_internal_configuration();
+
+ if (__atomic_load_n(&run_once, __ATOMIC_RELAXED) == 0)
+ return 0;
+
rte_service_finalize();
rte_mp_channel_cleanup();
+ rte_eal_intr_destroy();
/* after this point, any DPDK pointers will become dangling */
rte_eal_memory_detach();
rte_eal_alarm_cleanup();
@@ -648,6 +648,18 @@ rte_eal_intr_init(void)
return ret;
}
+void
+rte_eal_intr_destroy(void)
+{
+ /* cancel the host thread to wait/handle the interrupt */
+ pthread_cancel(intr_thread);
+ pthread_join(intr_thread, NULL);
+
+ /* close kqueue */
+ close(kq);
+ kq = -1;
+}
+
int
rte_intr_rx_ctl(struct rte_intr_handle *intr_handle,
int epfd, int op, unsigned int vec, void *data)
@@ -76,6 +76,8 @@ struct lcore_config lcore_config[RTE_MAX_LCORE];
/* used by rte_rdtsc() */
int rte_cycles_vmware_tsc_map;
+/* used to judge the running status of the eal */
+static uint32_t run_once;
int
eal_clean_runtime_dir(void)
@@ -857,12 +859,22 @@ is_iommu_enabled(void)
return n > 2;
}
+static void warn_parent(void)
+{
+ RTE_LOG(WARNING, EAL, "DPDK won't work in the child process\n");
+}
+
+static void scratch_child(void)
+{
+ /* Scratch run_once so that a call to rte_eal_cleanup won't crash... */
+ __atomic_store_n(&run_once, 0, __ATOMIC_RELAXED);
+}
+
/* Launch threads, called at application init(). */
int
rte_eal_init(int argc, char **argv)
{
int i, fctret, ret;
- static uint32_t run_once;
uint32_t has_run = 0;
const char *p;
static char logid[PATH_MAX];
@@ -1228,6 +1240,8 @@ rte_eal_init(int argc, char **argv)
eal_mcfg_complete();
+ pthread_atfork(NULL, warn_parent, scratch_child);
+
return fctret;
}
@@ -1257,6 +1271,9 @@ rte_eal_cleanup(void)
struct internal_config *internal_conf =
eal_get_internal_configuration();
+ if (__atomic_load_n(&run_once, __ATOMIC_RELAXED) == 0)
+ return 0;
+
if (rte_eal_process_type() == RTE_PROC_PRIMARY &&
internal_conf->hugepage_file.unlink_existing)
rte_memseg_walk(mark_freeable, NULL);
@@ -1266,6 +1283,7 @@ rte_eal_cleanup(void)
vfio_mp_sync_cleanup();
#endif
rte_mp_channel_cleanup();
+ rte_eal_intr_destroy();
/* after this point, any DPDK pointers will become dangling */
rte_eal_memory_detach();
eal_mp_dev_hotplug_cleanup();
@@ -1199,6 +1199,18 @@ rte_eal_intr_init(void)
return ret;
}
+void
+rte_eal_intr_destroy(void)
+{
+ /* cancel the host thread to wait/handle the interrupt */
+ pthread_cancel(intr_thread);
+ pthread_join(intr_thread, NULL);
+
+ /* close the pipe used by epoll */
+ close(intr_pipe.writefd);
+ close(intr_pipe.readfd);
+}
+
static void
eal_intr_proc_rxtx_intr(int fd, const struct rte_intr_handle *intr_handle)
{