Skip to content

Commit

Permalink
netfilter: nft_compat: use refcnt_t type for nft_xt reference count
Browse files Browse the repository at this point in the history
Using standard integer type was fine while all operations on it were
guarded by the nftnl subsys mutex.

This isn't true anymore:
1. transactions are guarded only by a pernet mutex, so concurrent
   rule manipulation in different netns is racy
2. the ->destroy hook runs from a work queue after the transaction
   mutex has been released already.

cpu0                           cpu1 (net 1)        cpu2 (net 2)
 kworker
    nft_compat->destroy        nft_compat->init    nft_compat->init
      if (--nft_xt->ref == 0)   nft_xt->ref++        nft_xt->ref++

Switch to refcount_t.  Doing this however only fixes a minor aspect,
nft_compat also performs linked-list operations in an unsafe way.

This is addressed in the next two patches.

Fixes: f102d66 ("netfilter: nf_tables: use dedicated mutex to guard transactions")
Fixes: 0935d55 ("netfilter: nf_tables: asynchronous release")
Reported-by: Taehee Yoo <ap420073@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
  • Loading branch information
Florian Westphal authored and Pablo Neira Ayuso committed Jan 18, 2019
1 parent 88a8121 commit 12c44ab
Showing 1 changed file with 8 additions and 8 deletions.
16 changes: 8 additions & 8 deletions net/netfilter/nft_compat.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
struct nft_xt {
struct list_head head;
struct nft_expr_ops ops;
unsigned int refcnt;
refcount_t refcnt;

/* Unlike other expressions, ops doesn't have static storage duration.
* nft core assumes they do. We use kfree_rcu so that nft core can
Expand All @@ -45,7 +45,7 @@ struct nft_xt_match_priv {

static bool nft_xt_put(struct nft_xt *xt)
{
if (--xt->refcnt == 0) {
if (refcount_dec_and_test(&xt->refcnt)) {
list_del(&xt->head);
kfree_rcu(xt, rcu_head);
return true;
Expand Down Expand Up @@ -273,7 +273,7 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
return -EINVAL;

nft_xt = container_of(expr->ops, struct nft_xt, ops);
nft_xt->refcnt++;
refcount_inc(&nft_xt->refcnt);
return 0;
}

Expand Down Expand Up @@ -486,7 +486,7 @@ __nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
return ret;

nft_xt = container_of(expr->ops, struct nft_xt, ops);
nft_xt->refcnt++;
refcount_inc(&nft_xt->refcnt);
return 0;
}

Expand Down Expand Up @@ -789,7 +789,7 @@ nft_match_select_ops(const struct nft_ctx *ctx,
goto err;
}

nft_match->refcnt = 0;
refcount_set(&nft_match->refcnt, 0);
nft_match->ops.type = &nft_match_type;
nft_match->ops.eval = nft_match_eval;
nft_match->ops.init = nft_match_init;
Expand Down Expand Up @@ -893,7 +893,7 @@ nft_target_select_ops(const struct nft_ctx *ctx,
goto err;
}

nft_target->refcnt = 0;
refcount_set(&nft_target->refcnt, 0);
nft_target->ops.type = &nft_target_type;
nft_target->ops.size = NFT_EXPR_SIZE(XT_ALIGN(target->targetsize));
nft_target->ops.init = nft_target_init;
Expand Down Expand Up @@ -964,7 +964,7 @@ static void __exit nft_compat_module_exit(void)
list_for_each_entry_safe(xt, next, &nft_target_list, head) {
struct xt_target *target = xt->ops.data;

if (WARN_ON_ONCE(xt->refcnt))
if (WARN_ON_ONCE(refcount_read(&xt->refcnt)))
continue;
module_put(target->me);
kfree(xt);
Expand All @@ -973,7 +973,7 @@ static void __exit nft_compat_module_exit(void)
list_for_each_entry_safe(xt, next, &nft_match_list, head) {
struct xt_match *match = xt->ops.data;

if (WARN_ON_ONCE(xt->refcnt))
if (WARN_ON_ONCE(refcount_read(&xt->refcnt)))
continue;
module_put(match->me);
kfree(xt);
Expand Down

0 comments on commit 12c44ab

Please sign in to comment.