Skip to content

Commit

Permalink
dpvs: fix deadlock when svc deleted before template conn expire
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
roykingz committed Aug 18, 2023
1 parent 89e8c30 commit 458255c
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 14 deletions.
2 changes: 1 addition & 1 deletion include/ipvs/dest.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion src/ipvs/ip_vs_conhash.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
13 changes: 8 additions & 5 deletions src/ipvs/ip_vs_conn.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
Expand Down
12 changes: 8 additions & 4 deletions src/ipvs/ip_vs_dest.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
Expand Down
4 changes: 2 additions & 2 deletions src/ipvs/ip_vs_mh.c
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/ipvs/ip_vs_service.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/*
Expand Down

0 comments on commit 458255c

Please sign in to comment.