From 458255c1599cb4a9af843d531d5158d8b2fdc06f Mon Sep 17 00:00:00 2001 From: roykingz Date: Fri, 18 Aug 2023 09:14:15 +0800 Subject: [PATCH] dpvs: fix deadlock when svc deleted before template conn expire When svc was deleted before template conn expire, template conn is the only object who hold the dest reference, and dpvs will call timer_sched_lock on g_timer_sched twice when template conn expire, one in rte_timer_tick_cb and another in dp_vs_dest_put->dpvs_timer_cancel, then dpvs fall into deadlock. So when template conn expire, we use dpvs_timer_cancel_nolock in dp_vs_dest_put instead. --- include/ipvs/dest.h | 2 +- src/ipvs/ip_vs_conhash.c | 2 +- src/ipvs/ip_vs_conn.c | 13 ++++++++----- src/ipvs/ip_vs_dest.c | 12 ++++++++---- src/ipvs/ip_vs_mh.c | 4 ++-- src/ipvs/ip_vs_service.c | 2 +- 6 files changed, 21 insertions(+), 14 deletions(-) diff --git a/include/ipvs/dest.h b/include/ipvs/dest.h index 1e1c70c5b..8d59bbcca 100644 --- a/include/ipvs/dest.h +++ b/include/ipvs/dest.h @@ -151,7 +151,7 @@ int dp_vs_dest_edit_health(struct dp_vs_service *svc, struct dp_vs_dest_conf *ud void dp_vs_dest_unlink(struct dp_vs_service *svc, struct dp_vs_dest *dest, int svcupd); -void dp_vs_dest_put(struct dp_vs_dest *dest); +void dp_vs_dest_put(struct dp_vs_dest *dest, bool lock); int dp_vs_dest_del(struct dp_vs_service *svc, struct dp_vs_dest_conf *udest); diff --git a/src/ipvs/ip_vs_conhash.c b/src/ipvs/ip_vs_conhash.c index 858c4b503..6c0ecbb6c 100644 --- a/src/ipvs/ip_vs_conhash.c +++ b/src/ipvs/ip_vs_conhash.c @@ -141,7 +141,7 @@ static void node_fini(struct node_s *node) return; if (node->data) { - dp_vs_dest_put((struct dp_vs_dest *)node->data); + dp_vs_dest_put((struct dp_vs_dest *)node->data, true); node->data = NULL; } diff --git a/src/ipvs/ip_vs_conn.c b/src/ipvs/ip_vs_conn.c index 82a30a06f..b0eb5c94e 100644 --- a/src/ipvs/ip_vs_conn.c +++ b/src/ipvs/ip_vs_conn.c @@ -388,12 +388,15 @@ static int dp_vs_conn_bind_dest(struct dp_vs_conn *conn, return EDPVS_OK; } -static int dp_vs_conn_unbind_dest(struct dp_vs_conn *conn) +static int dp_vs_conn_unbind_dest(struct dp_vs_conn *conn, bool must_lock) { struct dp_vs_dest *dest = conn->dest; + bool dest_timer_lock = true; if (dp_vs_conn_is_template(conn)) { rte_atomic32_dec(&dest->persistconns); + if (!must_lock) + dest_timer_lock = false; } else { if (conn->flags & DPVS_CONN_F_INACTIVE) rte_atomic32_dec(&dest->inactconns); @@ -407,7 +410,7 @@ static int dp_vs_conn_unbind_dest(struct dp_vs_conn *conn) dest->flags &= ~DPVS_DEST_F_OVERLOAD; } - dp_vs_dest_put(dest); + dp_vs_dest_put(dest, dest_timer_lock); conn->dest = NULL; return EDPVS_OK; @@ -680,7 +683,7 @@ static int dp_vs_conn_expire(void *priv) pp->conn_expire(pp, conn); dp_vs_conn_sa_release(conn); - dp_vs_conn_unbind_dest(conn); + dp_vs_conn_unbind_dest(conn, false); dp_vs_laddr_unbind(conn); dp_vs_conn_free_packets(conn); @@ -784,7 +787,7 @@ static void conn_flush(void) (struct sockaddr_storage *)&saddr); } - dp_vs_conn_unbind_dest(conn); + dp_vs_conn_unbind_dest(conn, true); dp_vs_laddr_unbind(conn); rte_atomic32_dec(&conn->refcnt); @@ -987,7 +990,7 @@ struct dp_vs_conn *dp_vs_conn_new(struct rte_mbuf *mbuf, unbind_laddr: dp_vs_laddr_unbind(new); unbind_dest: - dp_vs_conn_unbind_dest(new); + dp_vs_conn_unbind_dest(new, true); errout: dp_vs_conn_free(new); return NULL; diff --git a/src/ipvs/ip_vs_dest.c b/src/ipvs/ip_vs_dest.c index 9d0c93198..940923d98 100644 --- a/src/ipvs/ip_vs_dest.c +++ b/src/ipvs/ip_vs_dest.c @@ -268,14 +268,18 @@ int dp_vs_dest_edit_health(struct dp_vs_service *svc, struct dp_vs_dest_conf *ud return EDPVS_OK; } -void dp_vs_dest_put(struct dp_vs_dest *dest) +void dp_vs_dest_put(struct dp_vs_dest *dest, bool lock) { if (!dest) return; if (rte_atomic32_dec_and_test(&dest->refcnt)) { - if (rte_lcore_id() == g_master_lcore_id) - dpvs_timer_cancel(&dest->dfc.master.timer, true); + if (rte_lcore_id() == g_master_lcore_id) { + if (lock) + dpvs_timer_cancel(&dest->dfc.master.timer, true); + else + dpvs_timer_cancel_nolock(&dest->dfc.master.timer, true); + } dp_vs_service_unbind(dest); rte_free(dest); } @@ -333,7 +337,7 @@ dp_vs_dest_del(struct dp_vs_service *svc, struct dp_vs_dest_conf *udest) /* * Delete the destination */ - dp_vs_dest_put(dest); + dp_vs_dest_put(dest, true); return EDPVS_OK; } diff --git a/src/ipvs/ip_vs_mh.c b/src/ipvs/ip_vs_mh.c index b14cc05af..eba4781f8 100644 --- a/src/ipvs/ip_vs_mh.c +++ b/src/ipvs/ip_vs_mh.c @@ -97,7 +97,7 @@ static void dp_vs_mh_reset(struct dp_vs_mh_state *s) l = &s->lookup[0]; for (i = 0; i < DP_VS_MH_TAB_SIZE; i++) { if (l->dest) { - dp_vs_dest_put(l->dest); + dp_vs_dest_put(l->dest, true); l->dest = NULL; } l++; @@ -195,7 +195,7 @@ static int dp_vs_mh_populate(struct dp_vs_mh_state *s, new_dest = list_entry(p, struct dp_vs_dest, n_list); if (dest != new_dest) { if (dest) - dp_vs_dest_put(dest); + dp_vs_dest_put(dest, true); rte_atomic32_inc(&new_dest->refcnt); s->lookup[c].dest = new_dest; } diff --git a/src/ipvs/ip_vs_service.c b/src/ipvs/ip_vs_service.c index 039c3738c..839b4828e 100644 --- a/src/ipvs/ip_vs_service.c +++ b/src/ipvs/ip_vs_service.c @@ -614,7 +614,7 @@ static void __dp_vs_service_del(struct dp_vs_service *svc) */ list_for_each_entry_safe(dest, nxt, &svc->dests, n_list) { dp_vs_dest_unlink(svc, dest, 0); - dp_vs_dest_put(dest); + dp_vs_dest_put(dest, true); } /*