Skip to content

Commit f6ac858

Browse files
ummakynesummakynes
committed
netfilter: nf_tables: unbind set in rule from commit path
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. Lookup is reverse since the transaction that adds the set is likely to be at the tail of the list. 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. This patch removes the assumption that both activate and deactivate callbacks need to be provided. Fixes: cd5125d ("netfilter: nf_tables: split set destruction in deactivate and destroy phase") Reported-by: Mikhail Morfikov <[email protected]> Signed-off-by: Pablo Neira Ayuso <[email protected]>
1 parent 4e35c1c commit f6ac858

File tree

7 files changed

+85
-83
lines changed

7 files changed

+85
-83
lines changed

‎include/net/netfilter/nf_tables.h

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -469,9 +469,7 @@ struct nft_set_binding {
469469
int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
470470
struct nft_set_binding *binding);
471471
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
472-
struct nft_set_binding *binding);
473-
void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
474-
struct nft_set_binding *binding);
472+
struct nft_set_binding *binding, bool commit);
475473
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set);
476474

477475
/**
@@ -721,6 +719,13 @@ struct nft_expr_type {
721719
#define NFT_EXPR_STATEFUL 0x1
722720
#define NFT_EXPR_GC 0x2
723721

722+
enum nft_trans_phase {
723+
NFT_TRANS_PREPARE,
724+
NFT_TRANS_ABORT,
725+
NFT_TRANS_COMMIT,
726+
NFT_TRANS_RELEASE
727+
};
728+
724729
/**
725730
* struct nft_expr_ops - nf_tables expression operations
726731
*
@@ -750,7 +755,8 @@ struct nft_expr_ops {
750755
void (*activate)(const struct nft_ctx *ctx,
751756
const struct nft_expr *expr);
752757
void (*deactivate)(const struct nft_ctx *ctx,
753-
const struct nft_expr *expr);
758+
const struct nft_expr *expr,
759+
enum nft_trans_phase phase);
754760
void (*destroy)(const struct nft_ctx *ctx,
755761
const struct nft_expr *expr);
756762
void (*destroy_clone)(const struct nft_ctx *ctx,
@@ -1323,12 +1329,15 @@ struct nft_trans_rule {
13231329
struct nft_trans_set {
13241330
struct nft_set *set;
13251331
u32 set_id;
1332+
bool bound;
13261333
};
13271334

13281335
#define nft_trans_set(trans) \
13291336
(((struct nft_trans_set *)trans->data)->set)
13301337
#define nft_trans_set_id(trans) \
13311338
(((struct nft_trans_set *)trans->data)->set_id)
1339+
#define nft_trans_set_bound(trans) \
1340+
(((struct nft_trans_set *)trans->data)->bound)
13321341

13331342
struct nft_trans_chain {
13341343
bool update;

‎net/netfilter/nf_tables_api.c

Lines changed: 41 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,23 @@ static void nft_trans_destroy(struct nft_trans *trans)
116116
kfree(trans);
117117
}
118118

119+
static void nft_set_trans_bind(const struct nft_ctx *ctx, struct nft_set *set)
120+
{
121+
struct net *net = ctx->net;
122+
struct nft_trans *trans;
123+
124+
if (!nft_set_is_anonymous(set))
125+
return;
126+
127+
list_for_each_entry_reverse(trans, &net->nft.commit_list, list) {
128+
if (trans->msg_type == NFT_MSG_NEWSET &&
129+
nft_trans_set(trans) == set) {
130+
nft_trans_set_bound(trans) = true;
131+
break;
132+
}
133+
}
134+
}
135+
119136
static int nf_tables_register_hook(struct net *net,
120137
const struct nft_table *table,
121138
struct nft_chain *chain)
@@ -211,18 +228,6 @@ static int nft_delchain(struct nft_ctx *ctx)
211228
return err;
212229
}
213230

214-
/* either expr ops provide both activate/deactivate, or neither */
215-
static bool nft_expr_check_ops(const struct nft_expr_ops *ops)
216-
{
217-
if (!ops)
218-
return true;
219-
220-
if (WARN_ON_ONCE((!ops->activate ^ !ops->deactivate)))
221-
return false;
222-
223-
return true;
224-
}
225-
226231
static void nft_rule_expr_activate(const struct nft_ctx *ctx,
227232
struct nft_rule *rule)
228233
{
@@ -238,14 +243,15 @@ static void nft_rule_expr_activate(const struct nft_ctx *ctx,
238243
}
239244

240245
static void nft_rule_expr_deactivate(const struct nft_ctx *ctx,
241-
struct nft_rule *rule)
246+
struct nft_rule *rule,
247+
enum nft_trans_phase phase)
242248
{
243249
struct nft_expr *expr;
244250

245251
expr = nft_expr_first(rule);
246252
while (expr != nft_expr_last(rule) && expr->ops) {
247253
if (expr->ops->deactivate)
248-
expr->ops->deactivate(ctx, expr);
254+
expr->ops->deactivate(ctx, expr, phase);
249255

250256
expr = nft_expr_next(expr);
251257
}
@@ -296,7 +302,7 @@ static int nft_delrule(struct nft_ctx *ctx, struct nft_rule *rule)
296302
nft_trans_destroy(trans);
297303
return err;
298304
}
299-
nft_rule_expr_deactivate(ctx, rule);
305+
nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_PREPARE);
300306

301307
return 0;
302308
}
@@ -1929,9 +1935,6 @@ static int nf_tables_delchain(struct net *net, struct sock *nlsk,
19291935
*/
19301936
int nft_register_expr(struct nft_expr_type *type)
19311937
{
1932-
if (!nft_expr_check_ops(type->ops))
1933-
return -EINVAL;
1934-
19351938
nfnl_lock(NFNL_SUBSYS_NFTABLES);
19361939
if (type->family == NFPROTO_UNSPEC)
19371940
list_add_tail_rcu(&type->list, &nf_tables_expressions);
@@ -2079,10 +2082,6 @@ static int nf_tables_expr_parse(const struct nft_ctx *ctx,
20792082
err = PTR_ERR(ops);
20802083
goto err1;
20812084
}
2082-
if (!nft_expr_check_ops(ops)) {
2083-
err = -EINVAL;
2084-
goto err1;
2085-
}
20862085
} else
20872086
ops = type->ops;
20882087

@@ -2511,7 +2510,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
25112510
static void nf_tables_rule_release(const struct nft_ctx *ctx,
25122511
struct nft_rule *rule)
25132512
{
2514-
nft_rule_expr_deactivate(ctx, rule);
2513+
nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
25152514
nf_tables_rule_destroy(ctx, rule);
25162515
}
25172516

@@ -3708,39 +3707,30 @@ int nf_tables_bind_set(const struct nft_ctx *ctx, struct nft_set *set,
37083707
bind:
37093708
binding->chain = ctx->chain;
37103709
list_add_tail_rcu(&binding->list, &set->bindings);
3710+
nft_set_trans_bind(ctx, set);
3711+
37113712
return 0;
37123713
}
37133714
EXPORT_SYMBOL_GPL(nf_tables_bind_set);
37143715

3715-
void nf_tables_rebind_set(const struct nft_ctx *ctx, struct nft_set *set,
3716-
struct nft_set_binding *binding)
3717-
{
3718-
if (list_empty(&set->bindings) && nft_set_is_anonymous(set) &&
3719-
nft_is_active(ctx->net, set))
3720-
list_add_tail_rcu(&set->list, &ctx->table->sets);
3721-
3722-
list_add_tail_rcu(&binding->list, &set->bindings);
3723-
}
3724-
EXPORT_SYMBOL_GPL(nf_tables_rebind_set);
3725-
37263716
void nf_tables_unbind_set(const struct nft_ctx *ctx, struct nft_set *set,
3727-
struct nft_set_binding *binding)
3717+
struct nft_set_binding *binding, bool event)
37283718
{
37293719
list_del_rcu(&binding->list);
37303720

3731-
if (list_empty(&set->bindings) && nft_set_is_anonymous(set) &&
3732-
nft_is_active(ctx->net, set))
3721+
if (list_empty(&set->bindings) && nft_set_is_anonymous(set)) {
37333722
list_del_rcu(&set->list);
3723+
if (event)
3724+
nf_tables_set_notify(ctx, set, NFT_MSG_DELSET,
3725+
GFP_KERNEL);
3726+
}
37343727
}
37353728
EXPORT_SYMBOL_GPL(nf_tables_unbind_set);
37363729

37373730
void nf_tables_destroy_set(const struct nft_ctx *ctx, struct nft_set *set)
37383731
{
3739-
if (list_empty(&set->bindings) && nft_set_is_anonymous(set) &&
3740-
nft_is_active(ctx->net, set)) {
3741-
nf_tables_set_notify(ctx, set, NFT_MSG_DELSET, GFP_ATOMIC);
3732+
if (list_empty(&set->bindings) && nft_set_is_anonymous(set))
37423733
nft_set_destroy(set);
3743-
}
37443734
}
37453735
EXPORT_SYMBOL_GPL(nf_tables_destroy_set);
37463736

@@ -6535,6 +6525,9 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
65356525
nf_tables_rule_notify(&trans->ctx,
65366526
nft_trans_rule(trans),
65376527
NFT_MSG_DELRULE);
6528+
nft_rule_expr_deactivate(&trans->ctx,
6529+
nft_trans_rule(trans),
6530+
NFT_TRANS_COMMIT);
65386531
break;
65396532
case NFT_MSG_NEWSET:
65406533
nft_clear(net, nft_trans_set(trans));
@@ -6621,7 +6614,8 @@ static void nf_tables_abort_release(struct nft_trans *trans)
66216614
nf_tables_rule_destroy(&trans->ctx, nft_trans_rule(trans));
66226615
break;
66236616
case NFT_MSG_NEWSET:
6624-
nft_set_destroy(nft_trans_set(trans));
6617+
if (!nft_trans_set_bound(trans))
6618+
nft_set_destroy(nft_trans_set(trans));
66256619
break;
66266620
case NFT_MSG_NEWSETELEM:
66276621
nft_set_elem_destroy(nft_trans_elem_set(trans),
@@ -6682,7 +6676,9 @@ static int __nf_tables_abort(struct net *net)
66826676
case NFT_MSG_NEWRULE:
66836677
trans->ctx.chain->use--;
66846678
list_del_rcu(&nft_trans_rule(trans)->list);
6685-
nft_rule_expr_deactivate(&trans->ctx, nft_trans_rule(trans));
6679+
nft_rule_expr_deactivate(&trans->ctx,
6680+
nft_trans_rule(trans),
6681+
NFT_TRANS_ABORT);
66866682
break;
66876683
case NFT_MSG_DELRULE:
66886684
trans->ctx.chain->use++;
@@ -6692,7 +6688,8 @@ static int __nf_tables_abort(struct net *net)
66926688
break;
66936689
case NFT_MSG_NEWSET:
66946690
trans->ctx.table->use--;
6695-
list_del_rcu(&nft_trans_set(trans)->list);
6691+
if (!nft_trans_set_bound(trans))
6692+
list_del_rcu(&nft_trans_set(trans)->list);
66966693
break;
66976694
case NFT_MSG_DELSET:
66986695
trans->ctx.table->use++;

‎net/netfilter/nft_compat.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -587,10 +587,14 @@ static void nft_compat_activate_tg(const struct nft_ctx *ctx,
587587
}
588588

589589
static void nft_compat_deactivate(const struct nft_ctx *ctx,
590-
const struct nft_expr *expr)
590+
const struct nft_expr *expr,
591+
enum nft_trans_phase phase)
591592
{
592593
struct nft_xt *xt = container_of(expr->ops, struct nft_xt, ops);
593594

595+
if (phase == NFT_TRANS_COMMIT)
596+
return;
597+
594598
if (--xt->listcnt == 0)
595599
list_del_init(&xt->head);
596600
}

‎net/netfilter/nft_dynset.c

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -235,20 +235,17 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
235235
return err;
236236
}
237237

238-
static void nft_dynset_activate(const struct nft_ctx *ctx,
239-
const struct nft_expr *expr)
240-
{
241-
struct nft_dynset *priv = nft_expr_priv(expr);
242-
243-
nf_tables_rebind_set(ctx, priv->set, &priv->binding);
244-
}
245-
246238
static void nft_dynset_deactivate(const struct nft_ctx *ctx,
247-
const struct nft_expr *expr)
239+
const struct nft_expr *expr,
240+
enum nft_trans_phase phase)
248241
{
249242
struct nft_dynset *priv = nft_expr_priv(expr);
250243

251-
nf_tables_unbind_set(ctx, priv->set, &priv->binding);
244+
if (phase == NFT_TRANS_PREPARE)
245+
return;
246+
247+
nf_tables_unbind_set(ctx, priv->set, &priv->binding,
248+
phase == NFT_TRANS_COMMIT);
252249
}
253250

254251
static void nft_dynset_destroy(const struct nft_ctx *ctx,
@@ -296,7 +293,6 @@ static const struct nft_expr_ops nft_dynset_ops = {
296293
.eval = nft_dynset_eval,
297294
.init = nft_dynset_init,
298295
.destroy = nft_dynset_destroy,
299-
.activate = nft_dynset_activate,
300296
.deactivate = nft_dynset_deactivate,
301297
.dump = nft_dynset_dump,
302298
};

‎net/netfilter/nft_immediate.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,14 @@ static void nft_immediate_activate(const struct nft_ctx *ctx,
7272
}
7373

7474
static void nft_immediate_deactivate(const struct nft_ctx *ctx,
75-
const struct nft_expr *expr)
75+
const struct nft_expr *expr,
76+
enum nft_trans_phase phase)
7677
{
7778
const struct nft_immediate_expr *priv = nft_expr_priv(expr);
7879

80+
if (phase == NFT_TRANS_COMMIT)
81+
return;
82+
7983
return nft_data_release(&priv->data, nft_dreg_to_type(priv->dreg));
8084
}
8185

‎net/netfilter/nft_lookup.c

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -121,20 +121,17 @@ static int nft_lookup_init(const struct nft_ctx *ctx,
121121
return 0;
122122
}
123123

124-
static void nft_lookup_activate(const struct nft_ctx *ctx,
125-
const struct nft_expr *expr)
126-
{
127-
struct nft_lookup *priv = nft_expr_priv(expr);
128-
129-
nf_tables_rebind_set(ctx, priv->set, &priv->binding);
130-
}
131-
132124
static void nft_lookup_deactivate(const struct nft_ctx *ctx,
133-
const struct nft_expr *expr)
125+
const struct nft_expr *expr,
126+
enum nft_trans_phase phase)
134127
{
135128
struct nft_lookup *priv = nft_expr_priv(expr);
136129

137-
nf_tables_unbind_set(ctx, priv->set, &priv->binding);
130+
if (phase == NFT_TRANS_PREPARE)
131+
return;
132+
133+
nf_tables_unbind_set(ctx, priv->set, &priv->binding,
134+
phase == NFT_TRANS_COMMIT);
138135
}
139136

140137
static void nft_lookup_destroy(const struct nft_ctx *ctx,
@@ -225,7 +222,6 @@ static const struct nft_expr_ops nft_lookup_ops = {
225222
.size = NFT_EXPR_SIZE(sizeof(struct nft_lookup)),
226223
.eval = nft_lookup_eval,
227224
.init = nft_lookup_init,
228-
.activate = nft_lookup_activate,
229225
.deactivate = nft_lookup_deactivate,
230226
.destroy = nft_lookup_destroy,
231227
.dump = nft_lookup_dump,

‎net/netfilter/nft_objref.c

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -155,20 +155,17 @@ static int nft_objref_map_dump(struct sk_buff *skb, const struct nft_expr *expr)
155155
return -1;
156156
}
157157

158-
static void nft_objref_map_activate(const struct nft_ctx *ctx,
159-
const struct nft_expr *expr)
160-
{
161-
struct nft_objref_map *priv = nft_expr_priv(expr);
162-
163-
nf_tables_rebind_set(ctx, priv->set, &priv->binding);
164-
}
165-
166158
static void nft_objref_map_deactivate(const struct nft_ctx *ctx,
167-
const struct nft_expr *expr)
159+
const struct nft_expr *expr,
160+
enum nft_trans_phase phase)
168161
{
169162
struct nft_objref_map *priv = nft_expr_priv(expr);
170163

171-
nf_tables_unbind_set(ctx, priv->set, &priv->binding);
164+
if (phase == NFT_TRANS_PREPARE)
165+
return;
166+
167+
nf_tables_unbind_set(ctx, priv->set, &priv->binding,
168+
phase == NFT_TRANS_COMMIT);
172169
}
173170

174171
static void nft_objref_map_destroy(const struct nft_ctx *ctx,
@@ -185,7 +182,6 @@ static const struct nft_expr_ops nft_objref_map_ops = {
185182
.size = NFT_EXPR_SIZE(sizeof(struct nft_objref_map)),
186183
.eval = nft_objref_map_eval,
187184
.init = nft_objref_map_init,
188-
.activate = nft_objref_map_activate,
189185
.deactivate = nft_objref_map_deactivate,
190186
.destroy = nft_objref_map_destroy,
191187
.dump = nft_objref_map_dump,

0 commit comments

Comments
 (0)