Anonymous sets that are bound to rules from the same transaction trigger
a kernel splat from the abort path due to double set list removal and
double free.
This patch updates the logic to search for the transaction that is
responsible for creating the set and disable the set list removal and
release, given the rule is now responsible for this.
Moreover, this patch adds the unbind step to deliver the event from the
commit path. This should not be done from the worker thread, since we
have no guarantees of in-order delivery to the listener.
Fixes: cd5125d8f518 ("netfilter: nf_tables: split set destruction in deactivate and destroy phase")
Reported-by: Mikhail Morfikov <mmorfikov@xxxxxxxxx>
Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
---
include/net/netfilter/nf_tables.h | 11 +++++--
net/netfilter/nf_tables_api.c | 69 +++++++++++++++++++++++++++------------
net/netfilter/nft_dynset.c | 17 +++-------
net/netfilter/nft_lookup.c | 17 +++-------
net/netfilter/nft_objref.c | 17 +++-------
5 files changed, 69 insertions(+), 62 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 45eba7d7ab38..f2869cca970d 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -469,9 +469,7 @@ struct nft_set_binding {
int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
struct nft_set_binding *binding);
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding);
-void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding);
+ struct nft_set_binding *binding, bool event);
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set);
/**
@@ -729,6 +727,7 @@ struct nft_expr_type {
* @init: initialization function
* @activate: activate expression in the next generation
* @deactivate: deactivate expression in next generation
+ * @unbind: unbind set in this expression
* @destroy: destruction function, called after synchronize_rcu
* @dump: function to dump parameters
* @type: expression type
@@ -751,6 +750,9 @@ struct nft_expr_ops {
const struct nft_expr *expr);
void (*deactivate)(const struct nft_ctx *ctx,
const struct nft_expr *expr);
+ void (*unbind)(const struct nft_ctx *ctx,
+ const struct nft_expr *expr,
+ bool event);
void (*destroy)(const struct nft_ctx *ctx,
const struct nft_expr *expr);
void (*destroy_clone)(const struct nft_ctx *ctx,
@@ -1335,12 +1337,15 @@ struct nft_trans_rule {
struct nft_trans_set {
struct nft_set *set;
u32 set_id;
+ bool bound;
};
#define nft_trans_set(trans) \
(((struct nft_trans_set *)trans->data)->set)
#define nft_trans_set_id(trans) \
(((struct nft_trans_set *)trans->data)->set_id)
+#define nft_trans_set_bound(trans) \
+ (((struct nft_trans_set *)trans->data)->bound)
struct nft_trans_chain {
bool update;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 7495f29d24e8..9cd299f2c068 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -131,6 +131,23 @@ static void nft_trans_destroy(struct nft_trans *trans)
kfree(trans);
}
+static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set)
+{
+ struct net *net = ctx->net;
+ struct nft_trans *trans;
+
+ if (!nft_set_is_anonymous(set))
+ return;
+
+ list_for_each_entry(trans, &net->nft.commit_list, list) {
+ if (trans->msg_type == NFT_MSG_NEWSET &&
+ nft_trans_set(trans) == set) {
+ nft_trans_set_bound(trans) = true;
+ break;
+ }
+ }
+}
+
static int nf_tables_register_hook(struct net *net,
const struct nft_table *table,
struct nft_chain *chain)
@@ -266,6 +283,20 @@ static void nft_rule_expr_deactivate(const struct nft_ctx *ctx,
}
}
+static void nft_rule_expr_unbind(const struct nft_ctx *ctx,
+ struct nft_rule *rule, bool abort)
+{
+ struct nft_expr *expr;
+
+ expr = nft_expr_first(rule);
+ while (expr != nft_expr_last(rule) && expr->ops) {
+ if (expr->ops->unbind)
+ expr->ops->unbind(ctx, expr, abort);
+
+ expr = nft_expr_next(expr);
+ }
+}
+
static int
nf_tables_delrule_deactivate(struct nft_ctx *ctx, struct nft_rule *rule)
{
@@ -2555,6 +2586,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
static void nf_tables_rule_release(const struct nft_ctx *ctx,
struct nft_rule *rule)
{
+ nft_rule_expr_unbind(ctx, rule, false);
nft_rule_expr_deactivate(ctx, rule);
nf_tables_rule_destroy(ctx, rule);
}
@@ -3761,39 +3793,30 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
bind:
binding->chain = ctx->chain;
list_add_tail_rcu(&binding->list, &set->bindings);
+ nft_set_trans_bind(ctx, set);
+
return 0;
}
EXPORT_SYMBOL_GPL(nf_tables_bind_set);
-void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding)
-{
- if (list_empty(&set->bindings) && nft_set_is_anonymous(set) &&
- nft_is_active(ctx->net, set))
- list_add_tail_rcu(&set->list, &ctx->table->sets);
-
- list_add_tail_rcu(&binding->list, &set->bindings);
-}
-EXPORT_SYMBOL_GPL(nf_tables_rebind_set);
-
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
- struct nft_set_binding *binding)
+ struct nft_set_binding *binding, bool event)
{
list_del_rcu(&binding->list);
- if (list_empty(&set->bindings) && nft_set_is_anonymous(set) &&
- nft_is_active(ctx->net, set))
+ if (list_empty(&set->bindings) && nft_set_is_anonymous(set)) {
list_del_rcu(&set->list);
+ if (event)
+ nf_tables_set_notify(ctx, set, NFT_MSG_DELSET,
+ GFP_KERNEL);
+ }
}
EXPORT_SYMBOL_GPL(nf_tables_unbind_set);
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
{
- if (list_empty(&set->bindings) && nft_set_is_anonymous(set) &&
- nft_is_active(ctx->net, set)) {
- nf_tables_set_notify(ctx, set, NFT_MSG_DELSET, GFP_ATOMIC);
+ if (list_empty(&set->bindings) && nft_set_is_anonymous(set))
nft_set_destroy(set);
- }
}
EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
@@ -6618,6 +6641,8 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
nft_trans_destroy(trans);
break;
case NFT_MSG_DELRULE:
+ nft_rule_expr_unbind(&trans->ctx, nft_trans_rule(trans),
+ true);
list_del_rcu(&nft_trans_rule(trans)->list);
nf_tables_rule_notify(&trans->ctx,
nft_trans_rule(trans),
@@ -6708,7 +6733,8 @@ static void nf_tables_abort_release(struct nft_trans *trans)
nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans));
break;
case NFT_MSG_NEWSET:
- nft_set_destroy(nft_trans_set(trans));
+ if (!nft_trans_set_bound(trans))
+ nft_set_destroy(nft_trans_set(trans));
break;
case NFT_MSG_NEWSETELEM:
nft_set_elem_destroy(nft_trans_elem_set(trans),
@@ -6770,6 +6796,8 @@ static int __nf_tables_abort(struct net *net)
trans->ctx.chain->use--;
list_del_rcu(&nft_trans_rule(trans)->list);
nft_rule_expr_deactivate(&trans->ctx, nft_trans_rule(trans));
+ nft_rule_expr_unbind(&trans->ctx, nft_trans_rule(trans),
+ false);
break;
case NFT_MSG_DELRULE:
trans->ctx.chain->use++;
@@ -6779,7 +6807,8 @@ static int __nf_tables_abort(struct net *net)
break;
case NFT_MSG_NEWSET:
trans->ctx.table->use--;
- list_del_rcu(&nft_trans_set(trans)->list);
+ if (!nft_trans_set_bound(trans))
+ list_del_rcu(&nft_trans_set(trans)->list);
break;
case NFT_MSG_DELSET:
trans->ctx.table->use++;
diff --git a/net/netfilter/nft_dynset.c b/net/netfilter/nft_dynset.c
index 9658493d37d4..1080d1928d29 100644
--- a/net/netfilter/nft_dynset.c
+++ b/net/netfilter/nft_dynset.c
@@ -234,20 +234,12 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
return err;
}
-static void nft_dynset_activate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+static void nft_dynset_unbind(const struct nft_ctx *ctx,
+ const struct nft_expr *expr, bool event)
{
struct nft_dynset *priv = nft_expr_priv(expr);
- nf_tables_rebind_set(ctx, priv->set, &priv->binding);
-}
-
-static void nft_dynset_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
-{
- struct nft_dynset *priv = nft_expr_priv(expr);
-
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding, event);
}
static void nft_dynset_destroy(const struct nft_ctx *ctx,
@@ -295,8 +287,7 @@ static const struct nft_expr_ops nft_dynset_ops = {
.eval = nft_dynset_eval,
.init = nft_dynset_init,
.destroy = nft_dynset_destroy,
- .activate = nft_dynset_activate,
- .deactivate = nft_dynset_deactivate,
+ .unbind = nft_dynset_unbind,
.dump = nft_dynset_dump,
};
diff --git a/net/netfilter/nft_lookup.c b/net/netfilter/nft_lookup.c
index 227b2b15a19c..97ade22b9377 100644
--- a/net/netfilter/nft_lookup.c
+++ b/net/netfilter/nft_lookup.c
@@ -121,20 +121,12 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
return 0;
}
-static void nft_lookup_activate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+static void nft_lookup_unbind(const struct nft_ctx *ctx,
+ const struct nft_expr *expr, bool event)
{
struct nft_lookup *priv = nft_expr_priv(expr);
- nf_tables_rebind_set(ctx, priv->set, &priv->binding);
-}
-
-static void nft_lookup_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
-{
- struct nft_lookup *priv = nft_expr_priv(expr);
-
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding, event);
}
static void nft_lookup_destroy(const struct nft_ctx *ctx,
@@ -225,8 +217,7 @@ static const struct nft_expr_ops nft_lookup_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_lookup)),
.eval = nft_lookup_eval,
.init = nft_lookup_init,
- .activate = nft_lookup_activate,
- .deactivate = nft_lookup_deactivate,
+ .unbind = nft_lookup_unbind,
.destroy = nft_lookup_destroy,
.dump = nft_lookup_dump,
.validate = nft_lookup_validate,
diff --git a/net/netfilter/nft_objref.c b/net/netfilter/nft_objref.c
index c1f2adf198a0..7f0474248eb1 100644
--- a/net/netfilter/nft_objref.c
+++ b/net/netfilter/nft_objref.c
@@ -156,20 +156,12 @@ static int nft_objref_map_dump(struct sk_buff *skb, const struct nft_expr *expr)
return -1;
}
-static void nft_objref_map_activate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
+static void nft_objref_map_unbind(const struct nft_ctx *ctx,
+ const struct nft_expr *expr, bool event)
{
struct nft_objref_map *priv = nft_expr_priv(expr);
- nf_tables_rebind_set(ctx, priv->set, &priv->binding);
-}
-
-static void nft_objref_map_deactivate(const struct nft_ctx *ctx,
- const struct nft_expr *expr)
-{
- struct nft_objref_map *priv = nft_expr_priv(expr);
-
- nf_tables_unbind_set(ctx, priv->set, &priv->binding);
+ nf_tables_unbind_set(ctx, priv->set, &priv->binding, event);
}
static void nft_objref_map_destroy(const struct nft_ctx *ctx,
@@ -186,8 +178,7 @@ static const struct nft_expr_ops nft_objref_map_ops = {
.size = NFT_EXPR_SIZE(sizeof(struct nft_objref_map)),
.eval = nft_objref_map_eval,
.init = nft_objref_map_init,
- .activate = nft_objref_map_activate,
- .deactivate = nft_objref_map_deactivate,
+ .unbind = nft_objref_map_unbind,
.destroy = nft_objref_map_destroy,
.dump = nft_objref_map_dump,
};
--
2.11.0
