Skip to content

Commit f102d66

Browse files
Florian Westphalummakynes
Florian Westphal
authored and
ummakynes
committed
netfilter: nf_tables: use dedicated mutex to guard transactions
Continue to use nftnl subsys mutex to protect (un)registration of hook types, expressions and so on, but force batch operations to do their own locking. This allows distinct net namespaces to perform transactions in parallel. Signed-off-by: Florian Westphal <[email protected]> Signed-off-by: Pablo Neira Ayuso <[email protected]>
1 parent 2a43ecf commit f102d66

File tree

5 files changed

+77
-28
lines changed

5 files changed

+77
-28
lines changed

‎include/net/netns/nftables.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
struct netns_nftables {
88
struct list_head tables;
99
struct list_head commit_list;
10+
struct mutex commit_mutex;
1011
unsigned int base_seq;
1112
u8 gencursor;
1213
u8 validate_state;

‎net/netfilter/nf_tables_api.c

Lines changed: 69 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -480,12 +480,19 @@ static void nft_request_module(struct net *net, const char *fmt, ...)
480480
if (WARN(ret >= MODULE_NAME_LEN, "truncated: '%s' (len %d)", module_name, ret))
481481
return;
482482

483-
nfnl_unlock(NFNL_SUBSYS_NFTABLES);
483+
mutex_unlock(&net->nft.commit_mutex);
484484
request_module("%s", module_name);
485-
nfnl_lock(NFNL_SUBSYS_NFTABLES);
485+
mutex_lock(&net->nft.commit_mutex);
486486
}
487487
#endif
488488

489+
static void lockdep_nfnl_nft_mutex_not_held(void)
490+
{
491+
#ifdef CONFIG_PROVE_LOCKING
492+
WARN_ON_ONCE(lockdep_nfnl_is_held(NFNL_SUBSYS_NFTABLES));
493+
#endif
494+
}
495+
489496
static const struct nft_chain_type *
490497
nf_tables_chain_type_lookup(struct net *net, const struct nlattr *nla,
491498
u8 family, bool autoload)
@@ -495,6 +502,8 @@ nf_tables_chain_type_lookup(struct net *net, const struct nlattr *nla,
495502
type = __nf_tables_chain_type_lookup(nla, family);
496503
if (type != NULL)
497504
return type;
505+
506+
lockdep_nfnl_nft_mutex_not_held();
498507
#ifdef CONFIG_MODULES
499508
if (autoload) {
500509
nft_request_module(net, "nft-chain-%u-%.*s", family,
@@ -802,6 +811,7 @@ static int nf_tables_newtable(struct net *net, struct sock *nlsk,
802811
struct nft_ctx ctx;
803812
int err;
804813

814+
lockdep_assert_held(&net->nft.commit_mutex);
805815
attr = nla[NFTA_TABLE_NAME];
806816
table = nft_table_lookup(net, attr, family, genmask);
807817
if (IS_ERR(table)) {
@@ -1042,7 +1052,17 @@ nft_chain_lookup_byhandle(const struct nft_table *table, u64 handle, u8 genmask)
10421052
return ERR_PTR(-ENOENT);
10431053
}
10441054

1045-
static struct nft_chain *nft_chain_lookup(struct nft_table *table,
1055+
static bool lockdep_commit_lock_is_held(struct net *net)
1056+
{
1057+
#ifdef CONFIG_PROVE_LOCKING
1058+
return lockdep_is_held(&net->nft.commit_mutex);
1059+
#else
1060+
return true;
1061+
#endif
1062+
}
1063+
1064+
static struct nft_chain *nft_chain_lookup(struct net *net,
1065+
struct nft_table *table,
10461066
const struct nlattr *nla, u8 genmask)
10471067
{
10481068
char search[NFT_CHAIN_MAXNAMELEN + 1];
@@ -1055,7 +1075,7 @@ static struct nft_chain *nft_chain_lookup(struct nft_table *table,
10551075
nla_strlcpy(search, nla, sizeof(search));
10561076

10571077
WARN_ON(!rcu_read_lock_held() &&
1058-
!lockdep_nfnl_is_held(NFNL_SUBSYS_NFTABLES));
1078+
!lockdep_commit_lock_is_held(net));
10591079

10601080
chain = ERR_PTR(-ENOENT);
10611081
rcu_read_lock();
@@ -1295,7 +1315,7 @@ static int nf_tables_getchain(struct net *net, struct sock *nlsk,
12951315
return PTR_ERR(table);
12961316
}
12971317

1298-
chain = nft_chain_lookup(table, nla[NFTA_CHAIN_NAME], genmask);
1318+
chain = nft_chain_lookup(net, table, nla[NFTA_CHAIN_NAME], genmask);
12991319
if (IS_ERR(chain)) {
13001320
NL_SET_BAD_ATTR(extack, nla[NFTA_CHAIN_NAME]);
13011321
return PTR_ERR(chain);
@@ -1428,6 +1448,9 @@ static int nft_chain_parse_hook(struct net *net,
14281448
struct net_device *dev;
14291449
int err;
14301450

1451+
lockdep_assert_held(&net->nft.commit_mutex);
1452+
lockdep_nfnl_nft_mutex_not_held();
1453+
14311454
err = nla_parse_nested(ha, NFTA_HOOK_MAX, nla[NFTA_CHAIN_HOOK],
14321455
nft_hook_policy, NULL);
14331456
if (err < 0)
@@ -1662,7 +1685,8 @@ static int nf_tables_updchain(struct nft_ctx *ctx, u8 genmask, u8 policy,
16621685
nla[NFTA_CHAIN_NAME]) {
16631686
struct nft_chain *chain2;
16641687

1665-
chain2 = nft_chain_lookup(table, nla[NFTA_CHAIN_NAME], genmask);
1688+
chain2 = nft_chain_lookup(ctx->net, table,
1689+
nla[NFTA_CHAIN_NAME], genmask);
16661690
if (!IS_ERR(chain2))
16671691
return -EEXIST;
16681692
}
@@ -1724,6 +1748,8 @@ static int nf_tables_newchain(struct net *net, struct sock *nlsk,
17241748

17251749
create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
17261750

1751+
lockdep_assert_held(&net->nft.commit_mutex);
1752+
17271753
table = nft_table_lookup(net, nla[NFTA_CHAIN_TABLE], family, genmask);
17281754
if (IS_ERR(table)) {
17291755
NL_SET_BAD_ATTR(extack, nla[NFTA_CHAIN_TABLE]);
@@ -1742,7 +1768,7 @@ static int nf_tables_newchain(struct net *net, struct sock *nlsk,
17421768
}
17431769
attr = nla[NFTA_CHAIN_HANDLE];
17441770
} else {
1745-
chain = nft_chain_lookup(table, attr, genmask);
1771+
chain = nft_chain_lookup(net, table, attr, genmask);
17461772
if (IS_ERR(chain)) {
17471773
if (PTR_ERR(chain) != -ENOENT) {
17481774
NL_SET_BAD_ATTR(extack, attr);
@@ -1820,7 +1846,7 @@ static int nf_tables_delchain(struct net *net, struct sock *nlsk,
18201846
chain = nft_chain_lookup_byhandle(table, handle, genmask);
18211847
} else {
18221848
attr = nla[NFTA_CHAIN_NAME];
1823-
chain = nft_chain_lookup(table, attr, genmask);
1849+
chain = nft_chain_lookup(net, table, attr, genmask);
18241850
}
18251851
if (IS_ERR(chain)) {
18261852
NL_SET_BAD_ATTR(extack, attr);
@@ -1918,6 +1944,7 @@ static const struct nft_expr_type *nft_expr_type_get(struct net *net,
19181944
if (type != NULL && try_module_get(type->owner))
19191945
return type;
19201946

1947+
lockdep_nfnl_nft_mutex_not_held();
19211948
#ifdef CONFIG_MODULES
19221949
if (type == NULL) {
19231950
nft_request_module(net, "nft-expr-%u-%.*s", family,
@@ -2352,7 +2379,7 @@ static int nf_tables_getrule(struct net *net, struct sock *nlsk,
23522379
return PTR_ERR(table);
23532380
}
23542381

2355-
chain = nft_chain_lookup(table, nla[NFTA_RULE_CHAIN], genmask);
2382+
chain = nft_chain_lookup(net, table, nla[NFTA_RULE_CHAIN], genmask);
23562383
if (IS_ERR(chain)) {
23572384
NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_CHAIN]);
23582385
return PTR_ERR(chain);
@@ -2386,6 +2413,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
23862413
{
23872414
struct nft_expr *expr;
23882415

2416+
lockdep_assert_held(&ctx->net->nft.commit_mutex);
23892417
/*
23902418
* Careful: some expressions might not be initialized in case this
23912419
* is called on error from nf_tables_newrule().
@@ -2476,6 +2504,8 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
24762504
bool create;
24772505
u64 handle, pos_handle;
24782506

2507+
lockdep_assert_held(&net->nft.commit_mutex);
2508+
24792509
create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
24802510

24812511
table = nft_table_lookup(net, nla[NFTA_RULE_TABLE], family, genmask);
@@ -2484,7 +2514,7 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
24842514
return PTR_ERR(table);
24852515
}
24862516

2487-
chain = nft_chain_lookup(table, nla[NFTA_RULE_CHAIN], genmask);
2517+
chain = nft_chain_lookup(net, table, nla[NFTA_RULE_CHAIN], genmask);
24882518
if (IS_ERR(chain)) {
24892519
NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_CHAIN]);
24902520
return PTR_ERR(chain);
@@ -2684,7 +2714,8 @@ static int nf_tables_delrule(struct net *net, struct sock *nlsk,
26842714
}
26852715

26862716
if (nla[NFTA_RULE_CHAIN]) {
2687-
chain = nft_chain_lookup(table, nla[NFTA_RULE_CHAIN], genmask);
2717+
chain = nft_chain_lookup(net, table, nla[NFTA_RULE_CHAIN],
2718+
genmask);
26882719
if (IS_ERR(chain)) {
26892720
NL_SET_BAD_ATTR(extack, nla[NFTA_RULE_CHAIN]);
26902721
return PTR_ERR(chain);
@@ -2776,6 +2807,8 @@ nft_select_set_ops(const struct nft_ctx *ctx,
27762807
const struct nft_set_type *type;
27772808
u32 flags = 0;
27782809

2810+
lockdep_assert_held(&ctx->net->nft.commit_mutex);
2811+
lockdep_nfnl_nft_mutex_not_held();
27792812
#ifdef CONFIG_MODULES
27802813
if (list_empty(&nf_tables_set_types)) {
27812814
nft_request_module(ctx->net, "nft-set");
@@ -4820,6 +4853,7 @@ nft_obj_type_get(struct net *net, u32 objtype)
48204853
if (type != NULL && try_module_get(type->owner))
48214854
return type;
48224855

4856+
lockdep_nfnl_nft_mutex_not_held();
48234857
#ifdef CONFIG_MODULES
48244858
if (type == NULL) {
48254859
nft_request_module(net, "nft-obj-%u", objtype);
@@ -5379,6 +5413,7 @@ nft_flowtable_type_get(struct net *net, u8 family)
53795413
if (type != NULL && try_module_get(type->owner))
53805414
return type;
53815415

5416+
lockdep_nfnl_nft_mutex_not_held();
53825417
#ifdef CONFIG_MODULES
53835418
if (type == NULL) {
53845419
nft_request_module(net, "nf-flowtable-%u", family);
@@ -6232,9 +6267,9 @@ static void nf_tables_commit_chain_active(struct net *net, struct nft_chain *cha
62326267
next_genbit = nft_gencursor_next(net);
62336268

62346269
g0 = rcu_dereference_protected(chain->rules_gen_0,
6235-
lockdep_nfnl_is_held(NFNL_SUBSYS_NFTABLES));
6270+
lockdep_commit_lock_is_held(net));
62366271
g1 = rcu_dereference_protected(chain->rules_gen_1,
6237-
lockdep_nfnl_is_held(NFNL_SUBSYS_NFTABLES));
6272+
lockdep_commit_lock_is_held(net));
62386273

62396274
/* No changes to this chain? */
62406275
if (chain->rules_next == NULL) {
@@ -6442,6 +6477,7 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
64426477

64436478
nf_tables_commit_release(net);
64446479
nf_tables_gen_notify(net, skb, NFT_MSG_NEWGEN);
6480+
mutex_unlock(&net->nft.commit_mutex);
64456481

64466482
return 0;
64476483
}
@@ -6593,12 +6629,25 @@ static void nf_tables_cleanup(struct net *net)
65936629

65946630
static int nf_tables_abort(struct net *net, struct sk_buff *skb)
65956631
{
6596-
return __nf_tables_abort(net);
6632+
int ret = __nf_tables_abort(net);
6633+
6634+
mutex_unlock(&net->nft.commit_mutex);
6635+
6636+
return ret;
65976637
}
65986638

65996639
static bool nf_tables_valid_genid(struct net *net, u32 genid)
66006640
{
6601-
return genid == 0 || net->nft.base_seq == genid;
6641+
bool genid_ok;
6642+
6643+
mutex_lock(&net->nft.commit_mutex);
6644+
6645+
genid_ok = genid == 0 || net->nft.base_seq == genid;
6646+
if (!genid_ok)
6647+
mutex_unlock(&net->nft.commit_mutex);
6648+
6649+
/* else, commit mutex has to be released by commit or abort function */
6650+
return genid_ok;
66026651
}
66036652

66046653
static const struct nfnetlink_subsystem nf_tables_subsys = {
@@ -6937,8 +6986,8 @@ static int nft_verdict_init(const struct nft_ctx *ctx, struct nft_data *data,
69376986
case NFT_GOTO:
69386987
if (!tb[NFTA_VERDICT_CHAIN])
69396988
return -EINVAL;
6940-
chain = nft_chain_lookup(ctx->table, tb[NFTA_VERDICT_CHAIN],
6941-
genmask);
6989+
chain = nft_chain_lookup(ctx->net, ctx->table,
6990+
tb[NFTA_VERDICT_CHAIN], genmask);
69426991
if (IS_ERR(chain))
69436992
return PTR_ERR(chain);
69446993
if (nft_is_base_chain(chain))
@@ -7183,6 +7232,7 @@ static int __net_init nf_tables_init_net(struct net *net)
71837232
{
71847233
INIT_LIST_HEAD(&net->nft.tables);
71857234
INIT_LIST_HEAD(&net->nft.commit_list);
7235+
mutex_init(&net->nft.commit_mutex);
71867236
net->nft.base_seq = 1;
71877237
net->nft.validate_state = NFT_VALIDATE_SKIP;
71887238

@@ -7191,11 +7241,11 @@ static int __net_init nf_tables_init_net(struct net *net)
71917241

71927242
static void __net_exit nf_tables_exit_net(struct net *net)
71937243
{
7194-
nfnl_lock(NFNL_SUBSYS_NFTABLES);
7244+
mutex_lock(&net->nft.commit_mutex);
71957245
if (!list_empty(&net->nft.commit_list))
71967246
__nf_tables_abort(net);
71977247
__nft_release_tables(net);
7198-
nfnl_unlock(NFNL_SUBSYS_NFTABLES);
7248+
mutex_unlock(&net->nft.commit_mutex);
71997249
WARN_ON_ONCE(!list_empty(&net->nft.tables));
72007250
}
72017251

‎net/netfilter/nfnetlink.c

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -350,6 +350,8 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
350350
return kfree_skb(skb);
351351
}
352352

353+
nfnl_unlock(subsys_id);
354+
353355
while (skb->len >= nlmsg_total_size(0)) {
354356
int msglen, type;
355357

@@ -471,13 +473,8 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
471473
}
472474
done:
473475
if (status & NFNL_BATCH_REPLAY) {
474-
const struct nfnetlink_subsystem *ss2;
475-
476-
ss2 = nfnl_dereference_protected(subsys_id);
477-
if (ss2 == ss)
478-
ss->abort(net, oskb);
476+
ss->abort(net, oskb);
479477
nfnl_err_reset(&err_list);
480-
nfnl_unlock(subsys_id);
481478
kfree_skb(skb);
482479
module_put(ss->owner);
483480
goto replay;
@@ -497,7 +494,6 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct nlmsghdr *nlh,
497494
ss->cleanup(net);
498495

499496
nfnl_err_deliver(&err_list, oskb);
500-
nfnl_unlock(subsys_id);
501497
kfree_skb(skb);
502498
module_put(ss->owner);
503499
}

‎net/netfilter/nft_chain_filter.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ static int nf_tables_netdev_event(struct notifier_block *this,
322322
if (!ctx.net)
323323
return NOTIFY_DONE;
324324

325-
nfnl_lock(NFNL_SUBSYS_NFTABLES);
325+
mutex_lock(&ctx.net->nft.commit_mutex);
326326
list_for_each_entry(table, &ctx.net->nft.tables, list) {
327327
if (table->family != NFPROTO_NETDEV)
328328
continue;
@@ -337,7 +337,7 @@ static int nf_tables_netdev_event(struct notifier_block *this,
337337
nft_netdev_event(event, dev, &ctx);
338338
}
339339
}
340-
nfnl_unlock(NFNL_SUBSYS_NFTABLES);
340+
mutex_unlock(&ctx.net->nft.commit_mutex);
341341
put_net(ctx.net);
342342

343343
return NOTIFY_DONE;

‎net/netfilter/nft_dynset.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ static int nft_dynset_init(const struct nft_ctx *ctx,
118118
u64 timeout;
119119
int err;
120120

121+
lockdep_assert_held(&ctx->net->nft.commit_mutex);
122+
121123
if (tb[NFTA_DYNSET_SET_NAME] == NULL ||
122124
tb[NFTA_DYNSET_OP] == NULL ||
123125
tb[NFTA_DYNSET_SREG_KEY] == NULL)

0 commit comments

Comments
 (0)