Commit 0165d932 authored by Pablo Neira Ayuso's avatar Pablo Neira Ayuso

netfilter: nf_tables: fix racy rule deletion

We may lost race if we flush the rule-set (which happens asynchronously
via call_rcu) and we try to remove the table (that userspace assumes
to be empty).

Fix this by recovering synchronous rule and chain deletion. This was
introduced time ago before we had no batch support, and synchronous
rule deletion performance was not good. Now that we have the batch
support, we can just postpone the purge of old rule in a second step
in the commit phase. All object deletions are synchronous after this
patch.

As a side effect, we save memory as we don't need rcu_head per rule
anymore.

Cc: Patrick McHardy <kaber@trash.net>
Reported-by: default avatarArturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent b8ecbee6
...@@ -322,7 +322,6 @@ static inline void *nft_expr_priv(const struct nft_expr *expr) ...@@ -322,7 +322,6 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
* struct nft_rule - nf_tables rule * struct nft_rule - nf_tables rule
* *
* @list: used internally * @list: used internally
* @rcu_head: used internally for rcu
* @handle: rule handle * @handle: rule handle
* @genmask: generation mask * @genmask: generation mask
* @dlen: length of expression data * @dlen: length of expression data
...@@ -330,7 +329,6 @@ static inline void *nft_expr_priv(const struct nft_expr *expr) ...@@ -330,7 +329,6 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
*/ */
struct nft_rule { struct nft_rule {
struct list_head list; struct list_head list;
struct rcu_head rcu_head;
u64 handle:46, u64 handle:46,
genmask:2, genmask:2,
dlen:16; dlen:16;
...@@ -391,7 +389,6 @@ enum nft_chain_flags { ...@@ -391,7 +389,6 @@ enum nft_chain_flags {
* *
* @rules: list of rules in the chain * @rules: list of rules in the chain
* @list: used internally * @list: used internally
* @rcu_head: used internally
* @net: net namespace that this chain belongs to * @net: net namespace that this chain belongs to
* @table: table that this chain belongs to * @table: table that this chain belongs to
* @handle: chain handle * @handle: chain handle
...@@ -403,7 +400,6 @@ enum nft_chain_flags { ...@@ -403,7 +400,6 @@ enum nft_chain_flags {
struct nft_chain { struct nft_chain {
struct list_head rules; struct list_head rules;
struct list_head list; struct list_head list;
struct rcu_head rcu_head;
struct net *net; struct net *net;
struct nft_table *table; struct nft_table *table;
u64 handle; u64 handle;
......
...@@ -1008,10 +1008,8 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb, ...@@ -1008,10 +1008,8 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
return 0; return 0;
} }
static void nf_tables_rcu_chain_destroy(struct rcu_head *head) static void nf_tables_chain_destroy(struct nft_chain *chain)
{ {
struct nft_chain *chain = container_of(head, struct nft_chain, rcu_head);
BUG_ON(chain->use > 0); BUG_ON(chain->use > 0);
if (chain->flags & NFT_BASE_CHAIN) { if (chain->flags & NFT_BASE_CHAIN) {
...@@ -1059,7 +1057,9 @@ static int nf_tables_delchain(struct sock *nlsk, struct sk_buff *skb, ...@@ -1059,7 +1057,9 @@ static int nf_tables_delchain(struct sock *nlsk, struct sk_buff *skb,
family); family);
/* Make sure all rule references are gone before this is released */ /* Make sure all rule references are gone before this is released */
call_rcu(&chain->rcu_head, nf_tables_rcu_chain_destroy); synchronize_rcu();
nf_tables_chain_destroy(chain);
return 0; return 0;
} }
...@@ -1531,9 +1531,8 @@ static int nf_tables_getrule(struct sock *nlsk, struct sk_buff *skb, ...@@ -1531,9 +1531,8 @@ static int nf_tables_getrule(struct sock *nlsk, struct sk_buff *skb,
return err; return err;
} }
static void nf_tables_rcu_rule_destroy(struct rcu_head *head) static void nf_tables_rule_destroy(struct nft_rule *rule)
{ {
struct nft_rule *rule = container_of(head, struct nft_rule, rcu_head);
struct nft_expr *expr; struct nft_expr *expr;
/* /*
...@@ -1548,11 +1547,6 @@ static void nf_tables_rcu_rule_destroy(struct rcu_head *head) ...@@ -1548,11 +1547,6 @@ static void nf_tables_rcu_rule_destroy(struct rcu_head *head)
kfree(rule); kfree(rule);
} }
static void nf_tables_rule_destroy(struct nft_rule *rule)
{
call_rcu(&rule->rcu_head, nf_tables_rcu_rule_destroy);
}
#define NFT_RULE_MAXEXPRS 128 #define NFT_RULE_MAXEXPRS 128
static struct nft_expr_info *info; static struct nft_expr_info *info;
...@@ -1819,9 +1813,6 @@ static int nf_tables_commit(struct sk_buff *skb) ...@@ -1819,9 +1813,6 @@ static int nf_tables_commit(struct sk_buff *skb)
synchronize_rcu(); synchronize_rcu();
list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) { list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
/* Delete this rule from the dirty list */
list_del(&rupd->list);
/* This rule was inactive in the past and just became active. /* This rule was inactive in the past and just became active.
* Clear the next bit of the genmask since its meaning has * Clear the next bit of the genmask since its meaning has
* changed, now it is the future. * changed, now it is the future.
...@@ -1832,6 +1823,7 @@ static int nf_tables_commit(struct sk_buff *skb) ...@@ -1832,6 +1823,7 @@ static int nf_tables_commit(struct sk_buff *skb)
rupd->chain, rupd->rule, rupd->chain, rupd->rule,
NFT_MSG_NEWRULE, 0, NFT_MSG_NEWRULE, 0,
rupd->family); rupd->family);
list_del(&rupd->list);
kfree(rupd); kfree(rupd);
continue; continue;
} }
...@@ -1841,7 +1833,15 @@ static int nf_tables_commit(struct sk_buff *skb) ...@@ -1841,7 +1833,15 @@ static int nf_tables_commit(struct sk_buff *skb)
nf_tables_rule_notify(skb, rupd->nlh, rupd->table, rupd->chain, nf_tables_rule_notify(skb, rupd->nlh, rupd->table, rupd->chain,
rupd->rule, NFT_MSG_DELRULE, 0, rupd->rule, NFT_MSG_DELRULE, 0,
rupd->family); rupd->family);
}
/* Make sure we don't see any packet traversing old rules */
synchronize_rcu();
/* Now we can safely release unused old rules */
list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
nf_tables_rule_destroy(rupd->rule); nf_tables_rule_destroy(rupd->rule);
list_del(&rupd->list);
kfree(rupd); kfree(rupd);
} }
...@@ -1854,20 +1854,26 @@ static int nf_tables_abort(struct sk_buff *skb) ...@@ -1854,20 +1854,26 @@ static int nf_tables_abort(struct sk_buff *skb)
struct nft_rule_trans *rupd, *tmp; struct nft_rule_trans *rupd, *tmp;
list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) { list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
/* Delete all rules from the dirty list */
list_del(&rupd->list);
if (!nft_rule_is_active_next(net, rupd->rule)) { if (!nft_rule_is_active_next(net, rupd->rule)) {
nft_rule_clear(net, rupd->rule); nft_rule_clear(net, rupd->rule);
list_del(&rupd->list);
kfree(rupd); kfree(rupd);
continue; continue;
} }
/* This rule is inactive, get rid of it */ /* This rule is inactive, get rid of it */
list_del_rcu(&rupd->rule->list); list_del_rcu(&rupd->rule->list);
}
/* Make sure we don't see any packet accessing aborted rules */
synchronize_rcu();
list_for_each_entry_safe(rupd, tmp, &net->nft.commit_list, list) {
nf_tables_rule_destroy(rupd->rule); nf_tables_rule_destroy(rupd->rule);
list_del(&rupd->list);
kfree(rupd); kfree(rupd);
} }
return 0; return 0;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment