From 102293accbc6ac3a21d68ab98e058263b316a407 Mon Sep 17 00:00:00 2001 From: Pablo Neira Ayuso Date: Tue, 6 Nov 2012 16:51:11 +0100 Subject: conntrackd: fix deadlock due to wrong nested signal blocking The existing code may nest several signal blocking and unblocking calls in different paths of the code. This may result in deadlocks while receiving signals. This patch simplifies the signal blocking approach. Now signals are blocked in three paths: 1) Internal timers handling, while running timer callback for expired timers. 2) File descriptor handling, while running file descriptor callbacks. 3) While handling signals, to avoid that SIGINT and SIGTERM in a row results in a deadlock. Thanks a lot to Ulrich Weber for discussing a fix for this problem. Signed-off-by: Pablo Neira Ayuso --- src/process.c | 12 +----------- src/run.c | 13 ++++++++----- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/process.c b/src/process.c index 9b9734c..7f0a395 100644 --- a/src/process.c +++ b/src/process.c @@ -27,25 +27,19 @@ int fork_process_new(int type, int flags, void (*cb)(void *data), void *data) struct child_process *c, *this; int pid; - /* block SIGCHLD to avoid the access of the list concurrently */ - sigprocmask(SIG_BLOCK, &STATE(block), NULL); - /* We only want one process of this type at the same time. This is * useful if you want to prevent two child processes from accessing * a shared descriptor at the same time. */ if (flags & CTD_PROC_F_EXCL) { list_for_each_entry(this, &process_list, head) { if (this->type == type) { - sigprocmask(SIG_UNBLOCK, &STATE(block), NULL); return -1; } } } c = calloc(sizeof(struct child_process), 1); - if (c == NULL) { - sigprocmask(SIG_UNBLOCK, &STATE(block), NULL); + if (c == NULL) return -1; - } c->type = type; c->cb = cb; @@ -55,8 +49,6 @@ int fork_process_new(int type, int flags, void (*cb)(void *data), void *data) if (c->pid > 0) list_add(&c->head, &process_list); - sigprocmask(SIG_UNBLOCK, &STATE(block), NULL); - return pid; } @@ -89,7 +81,6 @@ void fork_process_dump(int fd) char buf[4096]; int size = 0; - sigprocmask(SIG_BLOCK, &STATE(block), NULL); list_for_each_entry(this, &process_list, head) { size += snprintf(buf+size, sizeof(buf), "PID=%u type=%s\n", @@ -97,7 +88,6 @@ void fork_process_dump(int fd) this->type < CTD_PROC_MAX ? process_type_to_name[this->type] : "unknown"); } - sigprocmask(SIG_UNBLOCK, &STATE(block), NULL); send(fd, buf, size, 0); } diff --git a/src/run.c b/src/run.c index 3337694..44a179f 100644 --- a/src/run.c +++ b/src/run.c @@ -40,10 +40,15 @@ #include #include -void killer(int foo) +void killer(int signal) { - /* no signals while handling signals */ - sigprocmask(SIG_BLOCK, &STATE(block), NULL); + /* Signals are re-entrant, disable signal handling to avoid problems + * in case we receive SIGINT and SIGTERM in a row. This function is + * also called via -k from the unix socket context, we already disabled + * signals in that path, so don't do it. + */ + if (signal) + sigprocmask(SIG_BLOCK, &STATE(block), NULL); local_server_destroy(&STATE(local)); @@ -58,8 +63,6 @@ void killer(int foo) dlog(LOG_NOTICE, "---- shutdown received ----"); close_log(); - sigprocmask(SIG_UNBLOCK, &STATE(block), NULL); - exit(0); } -- cgit v1.2.3 From 134bffa852edc341664dcc4808b1de41107d16d6 Mon Sep 17 00:00:00 2001 From: Pablo Neira Ayuso Date: Tue, 27 Nov 2012 20:57:42 +0100 Subject: conntrack: add support to dump the dying and unconfirmed list via ctnetlink This patch adds support for: conntrack -L dying conntrack -L unconfirmed To display the list of dying and unconfirmed conntracks. This provides some instrumentation in case that `conntrack -C` really deviates from what `conntrack -L | wc -l` says. Users like to check this to make sure things are going OK. Still, some conntrack objects may be still in the dying and the unconfirmed list. With this patch, we can also dump their content, before it was not possible. In normal cases both lists would be simply empty, or in the case of the dying list, you can observe that entries go slightly down in number. Signed-off-by: Pablo Neira Ayuso --- src/conntrack.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 95 insertions(+), 13 deletions(-) diff --git a/src/conntrack.c b/src/conntrack.c index 7451a80..227c355 100644 --- a/src/conntrack.c +++ b/src/conntrack.c @@ -822,27 +822,45 @@ add_command(unsigned int *cmd, const int newcmd) *cmd |= newcmd; } -static unsigned int -check_type(int argc, char *argv[]) +static char *get_table(int argc, char *argv[]) { char *table = NULL; - /* Nasty bug or feature in getopt_long ? + /* Nasty bug or feature in getopt_long ? * It seems that it behaves badly with optional arguments. * Fortunately, I just stole the fix from iptables ;) */ if (optarg) return 0; - else if (optind < argc && argv[optind][0] != '-' - && argv[optind][0] != '!') + else if (optind < argc && argv[optind][0] != '-' && + argv[optind][0] != '!') table = argv[optind++]; - - if (!table) - return 0; - + + return table; +} + +enum { + CT_TABLE_CONNTRACK, + CT_TABLE_EXPECT, + CT_TABLE_DYING, + CT_TABLE_UNCONFIRMED, +}; + +static unsigned int check_type(int argc, char *argv[]) +{ + const char *table = get_table(argc, argv); + + /* default to conntrack subsystem if nothing has been specified. */ + if (table == NULL) + return CT_TABLE_CONNTRACK; + if (strncmp("expect", table, strlen(table)) == 0) - return 1; + return CT_TABLE_EXPECT; else if (strncmp("conntrack", table, strlen(table)) == 0) - return 0; + return CT_TABLE_CONNTRACK; + else if (strncmp("dying", table, strlen(table)) == 0) + return CT_TABLE_DYING; + else if (strncmp("unconfirmed", table, strlen(table)) == 0) + return CT_TABLE_UNCONFIRMED; else exit_error(PARAMETER_PROBLEM, "unknown type `%s'", table); @@ -1686,6 +1704,27 @@ static int nfct_global_stats_cb(const struct nlmsghdr *nlh, void *data) return MNL_CB_OK; } +static int mnl_nfct_dump_cb(const struct nlmsghdr *nlh, void *data) +{ + struct nf_conntrack *ct; + char buf[4096]; + + ct = nfct_new(); + if (ct == NULL) + return MNL_CB_OK; + + nfct_nlmsg_parse(nlh, ct); + + nfct_snprintf(buf, sizeof(buf), ct, NFCT_T_UNKNOWN, NFCT_O_DEFAULT, 0); + printf("%s\n", buf); + + nfct_destroy(ct); + + counter++; + + return MNL_CB_OK; +} + static struct ctproto_handler *h; int main(int argc, char *argv[]) @@ -1720,6 +1759,16 @@ int main(int argc, char *argv[]) switch(c) { /* commands */ case 'L': + type = check_type(argc, argv); + /* Special case: dumping dying and unconfirmed list + * are handled like normal conntrack dumps. + */ + if (type == CT_TABLE_DYING || + type == CT_TABLE_UNCONFIRMED) + add_command(&command, cmd2type[c][0]); + else + add_command(&command, cmd2type[c][type]); + break; case 'I': case 'D': case 'G': @@ -1730,14 +1779,25 @@ int main(int argc, char *argv[]) case 'C': case 'S': type = check_type(argc, argv); + if (type == CT_TABLE_DYING || + type == CT_TABLE_UNCONFIRMED) { + exit_error(PARAMETER_PROBLEM, + "Can't do that command with " + "tables `dying' and `unconfirmed'"); + } add_command(&command, cmd2type[c][type]); break; case 'U': type = check_type(argc, argv); - if (type == 0) + if (type == CT_TABLE_DYING || + type == CT_TABLE_UNCONFIRMED) { + exit_error(PARAMETER_PROBLEM, + "Can't do that command with " + "tables `dying' and `unconfirmed'"); + } else if (type == CT_TABLE_CONNTRACK) add_command(&command, CT_UPDATE); else - exit_error(PARAMETER_PROBLEM, + exit_error(PARAMETER_PROBLEM, "Can't update expectations"); break; /* options */ @@ -1937,6 +1997,28 @@ int main(int argc, char *argv[]) struct nfct_filter_dump *filter_dump; case CT_LIST: + if (type == CT_TABLE_DYING) { + if (nfct_mnl_socket_open() < 0) + exit_error(OTHER_PROBLEM, "Can't open handler"); + + res = nfct_mnl_dump(NFNL_SUBSYS_CTNETLINK, + IPCTNL_MSG_CT_GET_DYING, + mnl_nfct_dump_cb); + + nfct_mnl_socket_close(); + break; + } else if (type == CT_TABLE_UNCONFIRMED) { + if (nfct_mnl_socket_open() < 0) + exit_error(OTHER_PROBLEM, "Can't open handler"); + + res = nfct_mnl_dump(NFNL_SUBSYS_CTNETLINK, + IPCTNL_MSG_CT_GET_UNCONFIRMED, + mnl_nfct_dump_cb); + + nfct_mnl_socket_close(); + break; + } + cth = nfct_open(CONNTRACK, 0); if (!cth) exit_error(OTHER_PROBLEM, "Can't open handler"); -- cgit v1.2.3 From a96fdeaac8274c0544b0ffa808782932e637a6f5 Mon Sep 17 00:00:00 2001 From: Pablo Neira Ayuso Date: Sun, 3 Mar 2013 21:49:58 +0100 Subject: build: bump version to 1.4.1 Signed-off-by: Pablo Neira Ayuso --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index cbe581c..44c49e2 100644 --- a/configure.ac +++ b/configure.ac @@ -1,4 +1,4 @@ -AC_INIT(conntrack-tools, 1.4.0, pablo@netfilter.org) +AC_INIT(conntrack-tools, 1.4.1, pablo@netfilter.org) AC_CONFIG_AUX_DIR([build-aux]) AC_CANONICAL_HOST -- cgit v1.2.3