Taehee Yoo <ap420073@xxxxxxxxx> wrote:
> index 83adabdec900..8f3114871d20 100644
> --- a/net/netfilter/nft_compat.c
> +++ b/net/netfilter/nft_compat.c
> @@ -29,6 +29,7 @@ struct nft_xt {
> struct list_head head;
> struct nft_expr_ops ops;
> unsigned int refcnt;
> + bool use;
>
> /* Unlike other expressions, ops doesn't have static storage duration.
> * nft core assumes they do. We use kfree_rcu so that nft core can
> @@ -48,7 +49,7 @@ struct nft_xt_match_priv {
> static bool nft_xt_put(struct nft_xt *xt)
> {
> mutex_lock(&nft_xt_lock);
> - if (--xt->refcnt == 0) {
> + if (--xt->refcnt == 0 && !xt->use) {
> list_del(&xt->head);
I don't doubt your findings, but I would prefer if we do not have to add
these kinds of conditionals here.
The problem is that when nft_xt_put is called, all side
effects are supposed to have been completed.
At this point, list_del() should not be allowed anymore,
it should have taken place while we were still under transaction
mutex.
Do you see any issues with this alternate approach?
It adds deactivate hook to list_del, so after transaction mutex is
released, next 'add' will create a new compat expression rather
than trying to use the one that is scheduled for free.
diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -27,6 +27,7 @@ struct nft_xt {
struct list_head head;
struct nft_expr_ops ops;
unsigned int refcnt;
+ unsigned int listcnt;
/* Unlike other expressions, ops doesn't have static storage duration.
* nft core assumes they do. We use kfree_rcu so that nft core can
@@ -43,10 +44,15 @@ struct nft_xt_match_priv {
void *info;
};
+static LIST_HEAD(nft_match_list);
+static LIST_HEAD(nft_target_list);
+static struct nft_expr_type nft_match_type;
+static struct nft_expr_type nft_target_type;
+
static bool nft_xt_put(struct nft_xt *xt)
{
if (--xt->refcnt == 0) {
- list_del(&xt->head);
+ WARN_ON_ONCE(!list_empty(&xt->head));
kfree_rcu(xt, rcu_head);
return true;
}
@@ -540,6 +546,40 @@ nft_match_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
__nft_match_destroy(ctx, expr, nft_expr_priv(expr));
}
+static void nft_compat_activate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr,
+ struct list_head *h)
+{
+ struct nft_xt *xt = container_of(expr->ops, struct nft_xt, ops);
+
+ if (xt->listcnt == 0)
+ list_add(&xt->head, h);
+
+ xt->listcnt++;
+}
+
+static void nft_compat_activate_mt(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ nft_compat_activate(ctx, expr, &nft_match_list);
+}
+
+static void nft_compat_activate_tg(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ nft_compat_activate(ctx, expr, &nft_target_list);
+}
+
+static void nft_compat_deactivate(const struct nft_ctx *ctx,
+ const struct nft_expr *expr)
+{
+ struct nft_xt *xt = container_of(expr->ops, struct nft_xt, ops);
+
+ WARN_ON_ONCE(xt->refcnt == 0);
+ if (--xt->listcnt == 0)
+ list_del_init(&xt->head);
+}
+
static void
nft_match_large_destroy(const struct nft_ctx *ctx, const struct nft_expr *expr)
{
@@ -734,10 +774,6 @@ static const struct nfnetlink_subsystem nfnl_compat_subsys = {
.cb = nfnl_nft_compat_cb,
};
-static LIST_HEAD(nft_match_list);
-
-static struct nft_expr_type nft_match_type;
-
static bool nft_match_cmp(const struct xt_match *match,
const char *name, u32 rev, u32 family)
{
@@ -794,6 +830,8 @@ nft_match_select_ops(const struct nft_ctx *ctx,
nft_match->ops.eval = nft_match_eval;
nft_match->ops.init = nft_match_init;
nft_match->ops.destroy = nft_match_destroy;
+ nft_match->ops.activate = nft_compat_activate_mt;
+ nft_match->ops.deactivate = nft_compat_deactivate;
nft_match->ops.dump = nft_match_dump;
nft_match->ops.validate = nft_match_validate;
nft_match->ops.data = match;
@@ -810,6 +848,7 @@ nft_match_select_ops(const struct nft_ctx *ctx,
nft_match->ops.size = matchsize;
+ nft_match->listcnt = 1;
list_add(&nft_match->head, &nft_match_list);
return &nft_match->ops;
@@ -826,10 +865,6 @@ static struct nft_expr_type nft_match_type __read_mostly = {
.owner = THIS_MODULE,
};
-static LIST_HEAD(nft_target_list);
-
-static struct nft_expr_type nft_target_type;
-
static bool nft_target_cmp(const struct xt_target *tg,
const char *name, u32 rev, u32 family)
{
@@ -898,6 +933,8 @@ nft_target_select_ops(const struct nft_ctx *ctx,
nft_target->ops.size = NFT_EXPR_SIZE(XT_ALIGN(target->targetsize));
nft_target->ops.init = nft_target_init;
nft_target->ops.destroy = nft_target_destroy;
+ nft_target->ops.activate = nft_compat_activate_tg;
+ nft_target->ops.deactivate = nft_compat_deactivate;
nft_target->ops.dump = nft_target_dump;
nft_target->ops.validate = nft_target_validate;
nft_target->ops.data = target;
@@ -907,6 +944,7 @@ nft_target_select_ops(const struct nft_ctx *ctx,
else
nft_target->ops.eval = nft_target_eval_xt;
+ nft_target->listcnt = 1;
list_add(&nft_target->head, &nft_target_list);
return &nft_target->ops;
