| Message ID | 20180429145633.12807-1-ap420073@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | fix module leak and use-after-free | expand |
Taehee Yoo <ap420073@gmail.com> wrote: > This patch adds the new release callback to release resources > allocated in nft_expr_type->select_ops. > This release callback can be used by error path in the > nf_tables_newrule routine. > Only the select_ops of the nft_compat.c allocates memory and holds > modules so far. Wouldn't it be simpler to just add the missing nft_xt_put() in nft_target_init()? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-04-30 3:03 GMT+09:00 Florian Westphal <fw@strlen.de>: > Taehee Yoo <ap420073@gmail.com> wrote: >> This patch adds the new release callback to release resources >> allocated in nft_expr_type->select_ops. >> This release callback can be used by error path in the >> nf_tables_newrule routine. >> Only the select_ops of the nft_compat.c allocates memory and holds >> modules so far. > > Wouldn't it be simpler to just add the missing nft_xt_put() > in nft_target_init()? > Thank you for your review! I think the putting nft_xt_put() into the nft_{target/match}_init() can't solve the below problem scenario. Below is what I experienced scenario. Before: $rmmod nft_counter Steps to reproduce: $iptables-compat -I OUTPUT -m cpu --cpu 0 When above command is given, a netlink message has two experssions that are the cpu compat and the nft_counter. The nft_expr_type_get() in the nf_tables_expr_parse() successes first expression then, calls select_ops callback. (allocates memory and holds module) But, second nft_expr_type_get() in the nf_tables_expr_parse() returns -EAGAIN because of request_module(). In that point, by the 'goto err1', the 'module_put(info[i].ops->type->owner)' is called. There is no release routine. If the nft_xt_put() is added into the nft_{target/match}_init(), above scenario still can't be solved. In order to reproduce above scenario, the nft_counter should be unloaded. In the second patch, you said that you can't reproduce this problem. If the nft_counter is unloaded, you can reproduce this problem. Could you please test this? Thank you! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Taehee Yoo <ap420073@gmail.com> wrote: > In the second patch, you said that you can't reproduce this problem. > If the nft_counter is unloaded, you can reproduce this problem. > Could you please test this? Ineed, that reproduces this. I think what nft_compat.c is doing in select_ops() is illegal, select_ops should not have any side effects. I'll see if we can reorganize this and defer module_get until we call ops->init(). -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Florian Westphal <fw@strlen.de> wrote: > Taehee Yoo <ap420073@gmail.com> wrote: > > In the second patch, you said that you can't reproduce this problem. > > If the nft_counter is unloaded, you can reproduce this problem. > > Could you please test this? > > Ineed, that reproduces this. > I think what nft_compat.c is doing in select_ops() is illegal, > select_ops should not have any side effects. > > I'll see if we can reorganize this and defer module_get until we > call ops->init(). I came up with this and it seems to work. Do you see any issues with this? Subject: netfilter: nf_tables: nft_compat: fix refcount leak on xt module Taehee Yoo reported following bug: iptables-compat -I OUTPUT -m cpu --cpu 0 iptables-compat -F lsmod |grep xt_cpu xt_cpu 16384 1 Quote: "When above command is given, a netlink message has two expressions that are the cpu compat and the nft_counter. The nft_expr_type_get() in the nf_tables_expr_parse() successes first expression then, calls select_ops callback. (allocates memory and holds module) But, second nft_expr_type_get() in the nf_tables_expr_parse() returns -EAGAIN because of request_module(). In that point, by the 'goto err1', the 'module_put(info[i].ops->type->owner)' is called. There is no release routine." The problem is that unlike all other expressions, nft_compat select_ops has side effects. 1. it allocates dynamic memory which holds an nft ops struct. In all other expressions, ops has static storage duration. 2. It grabs references to the xt module that it is supposed to invoke. Depending on where things go wrong, error unwinding doesn't always do the right thing. In the above scenario, a new nft_compat_expr is created and xt_cpu module gets loaded with a refcount of 1. Due to to -EAGAIN, the netlink messages get re-parsed. When that happens, nft_compat finds that xt_cpu is already present and increments module refcount again. This fixes the problem by making select_ops to have no visible side effects and removes all extra module_get/put. When select_ops creates a new nft_compat expression, the new expression has a refcount of 0, and the xt module gets its refcount incremented. When error happens, the next call finds existing entry, but will no longer increase the reference count -- the presence of existing nft_xt means we already hold a module reference. Because nft_xt_put is only called from nft_compat destroy hook, it will never see the initial zero reference count. ->destroy can only be called after ->init(), and that will increase the refcount. Lastly, we now free nft_xt struct with kfree_rcu. Else, we get use-after free in nf_tables_rule_destroy: while (expr != nft_expr_last(rule) && expr->ops) { nf_tables_expr_destroy(ctx, expr); expr = nft_expr_next(expr); // here nft_expr_next() dereferences expr->ops. This is safe for all users, as ops have static storage duration. In nft_compat case however, its ->destroy callback can free the memory that hold the ops structure. Reported-by: Taehee Yoo <ap420073@gmail.com> Signed-off-by: Florian Westphal <fw@strlen.de> --- net/netfilter/nft_compat.c | 94 +++++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 34 deletions(-) diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c index 8e23726b9081..6c96b32b4187 100644 --- a/net/netfilter/nft_compat.c +++ b/net/netfilter/nft_compat.c @@ -27,14 +27,26 @@ struct nft_xt { struct list_head head; struct nft_expr_ops ops; unsigned int refcnt; + + /* Unlike other expressions, ops doesn't have static storage duration + * here, but nft core assumes they do, in particular it assumes ops remain + * valid after calling ->destroy(), but in nft_compat case this could free + * ops as well. + * + * Its enough to delay free operation with kfree_rcu. + */ + struct rcu_head rcu_head; }; -static void nft_xt_put(struct nft_xt *xt) +static bool nft_xt_put(struct nft_xt *xt) { if (--xt->refcnt == 0) { list_del(&xt->head); - kfree(xt); + kfree_rcu(xt, rcu_head); + return true; } + + return false; } static int nft_compat_chain_validate_dependency(const char *tablename, @@ -226,6 +238,7 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr, struct xt_target *target = expr->ops->data; struct xt_tgchk_param par; size_t size = XT_ALIGN(nla_len(tb[NFTA_TARGET_INFO])); + struct nft_xt *nft_xt; u16 proto = 0; bool inv = false; union nft_entry e = {}; @@ -236,25 +249,22 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr, if (ctx->nla[NFTA_RULE_COMPAT]) { ret = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &proto, &inv); if (ret < 0) - goto err; + return ret; } nft_target_set_tgchk_param(&par, ctx, target, info, &e, proto, inv); ret = xt_check_target(&par, size, proto, inv); if (ret < 0) - goto err; + return ret; /* The standard target cannot be used */ - if (target->target == NULL) { - ret = -EINVAL; - goto err; - } + if (target->target == NULL) + return -EINVAL; + nft_xt = container_of(expr->ops, struct nft_xt, ops); + nft_xt->refcnt++; return 0; -err: - module_put(target->me); - return ret; } static void @@ -271,8 +281,8 @@ nft_target_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) if (par.target->destroy != NULL) par.target->destroy(&par); - nft_xt_put(container_of(expr->ops, struct nft_xt, ops)); - module_put(target->me); + if (nft_xt_put(container_of(expr->ops, struct nft_xt, ops))) + module_put(target->me); } static int nft_target_dump(struct sk_buff *skb, const struct nft_expr *expr) @@ -411,6 +421,7 @@ nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr, struct xt_match *match = expr->ops->data; struct xt_mtchk_param par; size_t size = XT_ALIGN(nla_len(tb[NFTA_MATCH_INFO])); + struct nft_xt *nft_xt; u16 proto = 0; bool inv = false; union nft_entry e = {}; @@ -421,19 +432,18 @@ nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr, if (ctx->nla[NFTA_RULE_COMPAT]) { ret = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &proto, &inv); if (ret < 0) - goto err; + return ret; } nft_match_set_mtchk_param(&par, ctx, match, info, &e, proto, inv); ret = xt_check_match(&par, size, proto, inv); if (ret < 0) - goto err; + return ret; + nft_xt = container_of(expr->ops, struct nft_xt, ops); + nft_xt->refcnt++; return 0; -err: - module_put(match->me); - return ret; } static void @@ -450,8 +460,8 @@ nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) if (par.match->destroy != NULL) par.match->destroy(&par); - nft_xt_put(container_of(expr->ops, struct nft_xt, ops)); - module_put(match->me); + if (nft_xt_put(container_of(expr->ops, struct nft_xt, ops))) + module_put(match->me); } static int nft_match_dump(struct sk_buff *skb, const struct nft_expr *expr) @@ -654,13 +664,8 @@ nft_match_select_ops(const struct nft_ctx *ctx, list_for_each_entry(nft_match, &nft_match_list, head) { struct xt_match *match = nft_match->ops.data; - if (nft_match_cmp(match, mt_name, rev, family)) { - if (!try_module_get(match->me)) - return ERR_PTR(-ENOENT); - - nft_match->refcnt++; + if (nft_match_cmp(match, mt_name, rev, family)) return &nft_match->ops; - } } match = xt_request_find_match(family, mt_name, rev); @@ -679,7 +684,7 @@ nft_match_select_ops(const struct nft_ctx *ctx, goto err; } - nft_match->refcnt = 1; + nft_match->refcnt = 0; nft_match->ops.type = &nft_match_type; nft_match->ops.size = NFT_EXPR_SIZE(XT_ALIGN(match->matchsize)); nft_match->ops.eval = nft_match_eval; @@ -739,13 +744,8 @@ nft_target_select_ops(const struct nft_ctx *ctx, list_for_each_entry(nft_target, &nft_target_list, head) { struct xt_target *target = nft_target->ops.data; - if (nft_target_cmp(target, tg_name, rev, family)) { - if (!try_module_get(target->me)) - return ERR_PTR(-ENOENT); - - nft_target->refcnt++; + if (nft_target_cmp(target, tg_name, rev, family)) return &nft_target->ops; - } } target = xt_request_find_target(family, tg_name, rev); @@ -764,7 +764,7 @@ nft_target_select_ops(const struct nft_ctx *ctx, goto err; } - nft_target->refcnt = 1; + 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; @@ -823,6 +823,32 @@ static int __init nft_compat_module_init(void) static void __exit nft_compat_module_exit(void) { + struct nft_xt *xt, *next; + + /* list should be empty here, it can be non-empty only in case there + * was an error that caused nft_xt expr to not be initialized fully + * and noone else requested the same expression later. + * + * In this case, the lists contain 0-refcount entries that still + * hold module reference. + */ + list_for_each_entry_safe(xt, next, &nft_target_list, head) { + struct xt_target *target = xt->ops.data; + + if (WARN_ON_ONCE(xt->refcnt)) + continue; + module_put(target->me); + kfree(xt); + } + + list_for_each_entry_safe(xt, next, &nft_match_list, head) { + struct xt_match *match = xt->ops.data; + + if (WARN_ON_ONCE(xt->refcnt)) + continue; + module_put(match->me); + kfree(xt); + } nfnetlink_subsys_unregister(&nfnl_compat_subsys); nft_unregister_expr(&nft_target_type); nft_unregister_expr(&nft_match_type);
2018-05-01 6:22 GMT+09:00 Florian Westphal <fw@strlen.de>: > Florian Westphal <fw@strlen.de> wrote: >> Taehee Yoo <ap420073@gmail.com> wrote: >> > In the second patch, you said that you can't reproduce this problem. >> > If the nft_counter is unloaded, you can reproduce this problem. >> > Could you please test this? >> >> Ineed, that reproduces this. >> I think what nft_compat.c is doing in select_ops() is illegal, >> select_ops should not have any side effects. >> >> I'll see if we can reorganize this and defer module_get until we >> call ops->init(). > > I came up with this and it seems to work. Do you see > any issues with this? > I have been testing this patch, As result, that works very well! My test cases and results are below 1: $rmmod nft_counter $iptables-compat -I OUTPUT -m cpu --cpu 0 $iptables-compat -F $lsmod | grep xt_cpu Module Size Used by xt_cpu 16384 0 2: $rmmod nft_counter $iptables-compat -I OUTPUT -m cpu --cpu 0 -m cpu --cpu 1 ... -m cpu --cpu 200 $iptables-compat -F $lsmod | grep xt_cpu Module Size Used by xt_cpu 16384 1 $rmmod nft_compat $lsmod | grep xt_cpu Module Size Used by xt_cpu 16384 0 Other test results are also good! (for example rule replace, ops->validate callback error scenario) And there is no warning(after-use-free-access and others). > Subject: netfilter: nf_tables: nft_compat: fix refcount leak on xt module > > Taehee Yoo reported following bug: > iptables-compat -I OUTPUT -m cpu --cpu 0 > iptables-compat -F > lsmod |grep xt_cpu > xt_cpu 16384 1 > > Quote: > "When above command is given, a netlink message has two expressions that > are the cpu compat and the nft_counter. > The nft_expr_type_get() in the nf_tables_expr_parse() successes > first expression then, calls select_ops callback. > (allocates memory and holds module) > But, second nft_expr_type_get() in the nf_tables_expr_parse() > returns -EAGAIN because of request_module(). > In that point, by the 'goto err1', > the 'module_put(info[i].ops->type->owner)' is called. > There is no release routine." > > The problem is that unlike all other expressions, > nft_compat select_ops has side effects. > > 1. it allocates dynamic memory which holds an nft ops struct. > In all other expressions, ops has static storage duration. > 2. It grabs references to the xt module that it is supposed to > invoke. > > Depending on where things go wrong, error unwinding doesn't > always do the right thing. > > In the above scenario, a new nft_compat_expr is created and > xt_cpu module gets loaded with a refcount of 1. > > Due to to -EAGAIN, the netlink messages get re-parsed. > When that happens, nft_compat finds that xt_cpu is already present > and increments module refcount again. > > This fixes the problem by making select_ops to have no visible > side effects and removes all extra module_get/put. > > When select_ops creates a new nft_compat expression, the new > expression has a refcount of 0, and the xt module gets its refcount > incremented. > > When error happens, the next call finds existing entry, but will no > longer increase the reference count -- the presence of existing > nft_xt means we already hold a module reference. > > Because nft_xt_put is only called from nft_compat destroy hook, > it will never see the initial zero reference count. > ->destroy can only be called after ->init(), and that will increase the > refcount. > > Lastly, we now free nft_xt struct with kfree_rcu. > Else, we get use-after free in nf_tables_rule_destroy: > > while (expr != nft_expr_last(rule) && expr->ops) { > nf_tables_expr_destroy(ctx, expr); > expr = nft_expr_next(expr); // here > > nft_expr_next() dereferences expr->ops. This is safe > for all users, as ops have static storage duration. > In nft_compat case however, its ->destroy callback can > free the memory that hold the ops structure. > > Reported-by: Taehee Yoo <ap420073@gmail.com> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/netfilter/nft_compat.c | 94 +++++++++++++++++++++++++++++----------------- > 1 file changed, 60 insertions(+), 34 deletions(-) > > diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c > index 8e23726b9081..6c96b32b4187 100644 > --- a/net/netfilter/nft_compat.c > +++ b/net/netfilter/nft_compat.c > @@ -27,14 +27,26 @@ struct nft_xt { > struct list_head head; > struct nft_expr_ops ops; > unsigned int refcnt; > + > + /* Unlike other expressions, ops doesn't have static storage duration > + * here, but nft core assumes they do, in particular it assumes ops remain > + * valid after calling ->destroy(), but in nft_compat case this could free > + * ops as well. > + * > + * Its enough to delay free operation with kfree_rcu. > + */ > + struct rcu_head rcu_head; > }; > > -static void nft_xt_put(struct nft_xt *xt) > +static bool nft_xt_put(struct nft_xt *xt) > { > if (--xt->refcnt == 0) { > list_del(&xt->head); > - kfree(xt); > + kfree_rcu(xt, rcu_head); > + return true; > } > + > + return false; > } > > static int nft_compat_chain_validate_dependency(const char *tablename, > @@ -226,6 +238,7 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr, > struct xt_target *target = expr->ops->data; > struct xt_tgchk_param par; > size_t size = XT_ALIGN(nla_len(tb[NFTA_TARGET_INFO])); > + struct nft_xt *nft_xt; > u16 proto = 0; > bool inv = false; > union nft_entry e = {}; > @@ -236,25 +249,22 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr, > if (ctx->nla[NFTA_RULE_COMPAT]) { > ret = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &proto, &inv); > if (ret < 0) > - goto err; > + return ret; > } > > nft_target_set_tgchk_param(&par, ctx, target, info, &e, proto, inv); > > ret = xt_check_target(&par, size, proto, inv); > if (ret < 0) > - goto err; > + return ret; > > /* The standard target cannot be used */ > - if (target->target == NULL) { > - ret = -EINVAL; > - goto err; > - } > + if (target->target == NULL) > + return -EINVAL; > > + nft_xt = container_of(expr->ops, struct nft_xt, ops); > + nft_xt->refcnt++; > return 0; > -err: > - module_put(target->me); > - return ret; > } > > static void > @@ -271,8 +281,8 @@ nft_target_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) > if (par.target->destroy != NULL) > par.target->destroy(&par); > > - nft_xt_put(container_of(expr->ops, struct nft_xt, ops)); > - module_put(target->me); > + if (nft_xt_put(container_of(expr->ops, struct nft_xt, ops))) > + module_put(target->me); > } > > static int nft_target_dump(struct sk_buff *skb, const struct nft_expr *expr) > @@ -411,6 +421,7 @@ nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr, > struct xt_match *match = expr->ops->data; > struct xt_mtchk_param par; > size_t size = XT_ALIGN(nla_len(tb[NFTA_MATCH_INFO])); > + struct nft_xt *nft_xt; > u16 proto = 0; > bool inv = false; > union nft_entry e = {}; > @@ -421,19 +432,18 @@ nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr, > if (ctx->nla[NFTA_RULE_COMPAT]) { > ret = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &proto, &inv); > if (ret < 0) > - goto err; > + return ret; > } > > nft_match_set_mtchk_param(&par, ctx, match, info, &e, proto, inv); > > ret = xt_check_match(&par, size, proto, inv); > if (ret < 0) > - goto err; > + return ret; > > + nft_xt = container_of(expr->ops, struct nft_xt, ops); > + nft_xt->refcnt++; > return 0; > -err: > - module_put(match->me); > - return ret; > } > > static void > @@ -450,8 +460,8 @@ nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) > if (par.match->destroy != NULL) > par.match->destroy(&par); > > - nft_xt_put(container_of(expr->ops, struct nft_xt, ops)); > - module_put(match->me); > + if (nft_xt_put(container_of(expr->ops, struct nft_xt, ops))) > + module_put(match->me); > } > > static int nft_match_dump(struct sk_buff *skb, const struct nft_expr *expr) > @@ -654,13 +664,8 @@ nft_match_select_ops(const struct nft_ctx *ctx, > list_for_each_entry(nft_match, &nft_match_list, head) { > struct xt_match *match = nft_match->ops.data; > > - if (nft_match_cmp(match, mt_name, rev, family)) { > - if (!try_module_get(match->me)) > - return ERR_PTR(-ENOENT); > - > - nft_match->refcnt++; > + if (nft_match_cmp(match, mt_name, rev, family)) > return &nft_match->ops; > - } > } > > match = xt_request_find_match(family, mt_name, rev); > @@ -679,7 +684,7 @@ nft_match_select_ops(const struct nft_ctx *ctx, > goto err; > } > > - nft_match->refcnt = 1; > + nft_match->refcnt = 0; > nft_match->ops.type = &nft_match_type; > nft_match->ops.size = NFT_EXPR_SIZE(XT_ALIGN(match->matchsize)); > nft_match->ops.eval = nft_match_eval; > @@ -739,13 +744,8 @@ nft_target_select_ops(const struct nft_ctx *ctx, > list_for_each_entry(nft_target, &nft_target_list, head) { > struct xt_target *target = nft_target->ops.data; > > - if (nft_target_cmp(target, tg_name, rev, family)) { > - if (!try_module_get(target->me)) > - return ERR_PTR(-ENOENT); > - > - nft_target->refcnt++; > + if (nft_target_cmp(target, tg_name, rev, family)) > return &nft_target->ops; > - } > } > > target = xt_request_find_target(family, tg_name, rev); > @@ -764,7 +764,7 @@ nft_target_select_ops(const struct nft_ctx *ctx, > goto err; > } > > - nft_target->refcnt = 1; > + 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; > @@ -823,6 +823,32 @@ static int __init nft_compat_module_init(void) > > static void __exit nft_compat_module_exit(void) > { > + struct nft_xt *xt, *next; > + > + /* list should be empty here, it can be non-empty only in case there > + * was an error that caused nft_xt expr to not be initialized fully > + * and noone else requested the same expression later. > + * > + * In this case, the lists contain 0-refcount entries that still > + * hold module reference. > + */ > + list_for_each_entry_safe(xt, next, &nft_target_list, head) { > + struct xt_target *target = xt->ops.data; > + > + if (WARN_ON_ONCE(xt->refcnt)) > + continue; > + module_put(target->me); > + kfree(xt); > + } > + > + list_for_each_entry_safe(xt, next, &nft_match_list, head) { > + struct xt_match *match = xt->ops.data; > + > + if (WARN_ON_ONCE(xt->refcnt)) > + continue; > + module_put(match->me); > + kfree(xt); > + } > nfnetlink_subsys_unregister(&nfnl_compat_subsys); > nft_unregister_expr(&nft_target_type); > nft_unregister_expr(&nft_match_type); > -- > 2.16.1 > -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h index cd368d1..2e71dc7 100644 --- a/include/net/netfilter/nf_tables.h +++ b/include/net/netfilter/nf_tables.h @@ -688,6 +688,7 @@ static inline void nft_set_gc_batch_add(struct nft_set_gc_batch *gcb, * struct nft_expr_type - nf_tables expression type * * @select_ops: function to select nft_expr_ops + * @release : function to release * @ops: default ops, used when no select_ops functions is present * @list: used internally * @name: Identifier @@ -700,6 +701,7 @@ static inline void nft_set_gc_batch_add(struct nft_set_gc_batch *gcb, struct nft_expr_type { const struct nft_expr_ops *(*select_ops)(const struct nft_ctx *, const struct nlattr * const tb[]); + void (*release)(const struct nft_expr_ops *ops); const struct nft_expr_ops *ops; struct list_head list; const char *name; diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c index 8e23726..31ffe27 100644 --- a/net/netfilter/nft_compat.c +++ b/net/netfilter/nft_compat.c @@ -236,25 +236,20 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr, if (ctx->nla[NFTA_RULE_COMPAT]) { ret = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &proto, &inv); if (ret < 0) - goto err; + return ret; } nft_target_set_tgchk_param(&par, ctx, target, info, &e, proto, inv); ret = xt_check_target(&par, size, proto, inv); if (ret < 0) - goto err; + return ret; /* The standard target cannot be used */ - if (target->target == NULL) { - ret = -EINVAL; - goto err; - } + if (!target->target) + return -EINVAL; return 0; -err: - module_put(target->me); - return ret; } static void @@ -268,11 +263,8 @@ nft_target_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) par.target = target; par.targinfo = info; par.family = ctx->family; - if (par.target->destroy != NULL) + if (par.target->destroy) par.target->destroy(&par); - - nft_xt_put(container_of(expr->ops, struct nft_xt, ops)); - module_put(target->me); } static int nft_target_dump(struct sk_buff *skb, const struct nft_expr *expr) @@ -421,19 +413,16 @@ nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr, if (ctx->nla[NFTA_RULE_COMPAT]) { ret = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &proto, &inv); if (ret < 0) - goto err; + return ret; } nft_match_set_mtchk_param(&par, ctx, match, info, &e, proto, inv); ret = xt_check_match(&par, size, proto, inv); if (ret < 0) - goto err; + return ret; return 0; -err: - module_put(match->me); - return ret; } static void @@ -447,11 +436,8 @@ nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr) par.match = match; par.matchinfo = info; par.family = ctx->family; - if (par.match->destroy != NULL) + if (par.match->destroy) par.match->destroy(&par); - - nft_xt_put(container_of(expr->ops, struct nft_xt, ops)); - module_put(match->me); } static int nft_match_dump(struct sk_buff *skb, const struct nft_expr *expr) @@ -674,7 +660,7 @@ nft_match_select_ops(const struct nft_ctx *ctx, /* This is the first time we use this match, allocate operations */ nft_match = kzalloc(sizeof(struct nft_xt), GFP_KERNEL); - if (nft_match == NULL) { + if (!nft_match) { err = -ENOMEM; goto err; } @@ -697,9 +683,18 @@ nft_match_select_ops(const struct nft_ctx *ctx, return ERR_PTR(err); } +static void nft_match_release(const struct nft_expr_ops *ops) +{ + struct xt_match *match = ops->data; + + nft_xt_put(container_of(ops, struct nft_xt, ops)); + module_put(match->me); +} + static struct nft_expr_type nft_match_type __read_mostly = { .name = "match", .select_ops = nft_match_select_ops, + .release = nft_match_release, .policy = nft_match_policy, .maxattr = NFTA_MATCH_MAX, .owner = THIS_MODULE, @@ -759,7 +754,7 @@ nft_target_select_ops(const struct nft_ctx *ctx, /* This is the first time we use this target, allocate operations */ nft_target = kzalloc(sizeof(struct nft_xt), GFP_KERNEL); - if (nft_target == NULL) { + if (!nft_target) { err = -ENOMEM; goto err; } @@ -786,9 +781,18 @@ nft_target_select_ops(const struct nft_ctx *ctx, return ERR_PTR(err); } +static void nft_target_release(const struct nft_expr_ops *ops) +{ + struct xt_target *target = ops->data; + + nft_xt_put(container_of(ops, struct nft_xt, ops)); + module_put(target->me); +} + static struct nft_expr_type nft_target_type __read_mostly = { .name = "target", .select_ops = nft_target_select_ops, + .release = nft_target_release, .policy = nft_target_policy, .maxattr = NFTA_TARGET_MAX, .owner = THIS_MODULE,
This patch adds the new release callback to release resources allocated in nft_expr_type->select_ops. This release callback can be used by error path in the nf_tables_newrule routine. Only the select_ops of the nft_compat.c allocates memory and holds modules so far. Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- include/net/netfilter/nf_tables.h | 2 ++ net/netfilter/nft_compat.c | 52 +++++++++++++++++++++------------------ 2 files changed, 30 insertions(+), 24 deletions(-)