Hi Phil,
On Thu, Feb 14, 2019 at 06:21:04PM +0100, Phil Sutter wrote:
> Hi Pablo,
>
> Originating from a problem with ebtables-nft user-defined chain
> policies, I made up the following use-case:
>
> | # iptables-nft -A FORWARD -j ACCEPT
> | # iptables-nft-restore --noflush <<EOF
> | *filter
> | -D FORWARD -j ACCEPT
> | -F
> | COMMIT
> | EOF
> | iptables-restore v1.8.2 (nf_tables):
> | line 3: RULE_FLUSH failed (No such file or directory): rule in chain FORWARD
>
> In case anyone reading this is not aware of it: In nftables, flushing a
> chain works by sending NFT_MSG_DELRULE message with just table and chain
> defined, no rule handle or position.
>
> The problem is that delete command in batch removes the rule, flush
> command then tries to delete the same rule again. Kernel returns -ENOENT
> in nf_tables_delrule_deactivate().
>
> The above use-case works with legacy iptables.
>
> Question is if I have to work around this in userspace or if we should
> make nf_tables_delrule_deactivate() return 0 even if given rule is not
> active? Downside is that second option would cause double deletion of
> same rule within a single batch to succeed.
>From the flush path, we can skip requests for deletion of rules that
have been already deleted, ie. we add no transaction since there is
already one in place.
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 5a92f23f179f..efc79422a8c7 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -288,11 +288,14 @@ static struct nft_trans *nft_trans_rule_add(struct nft_ctx *ctx, int msg_type,
return trans;
}
-static int nft_delrule(struct nft_ctx *ctx, struct nft_rule *rule)
+static int nft_delrule(struct nft_ctx *ctx, struct nft_rule *rule, bool flush)
{
struct nft_trans *trans;
int err;
+ if (flush && !nft_is_active_next(ctx->net, rule))
+ return 0;
+
trans = nft_trans_rule_add(ctx, NFT_MSG_DELRULE, rule);
if (trans == NULL)
return -ENOMEM;
@@ -313,7 +316,7 @@ static int nft_delrule_by_chain(struct nft_ctx *ctx)
int err;
list_for_each_entry(rule, &ctx->chain->rules, list) {
- err = nft_delrule(ctx, rule);
+ err = nft_delrule(ctx, rule, true);
if (err < 0)
return err;
}
@@ -1906,7 +1909,7 @@ static int nf_tables_delchain(struct net *net, struct sock *nlsk,
continue;
use--;
- err = nft_delrule(&ctx, rule);
+ err = nft_delrule(&ctx, rule, true);
if (err < 0)
return err;
}
@@ -2707,7 +2710,7 @@ static int nf_tables_newrule(struct net *net, struct sock *nlsk,
err = -ENOMEM;
goto err2;
}
- err = nft_delrule(&ctx, old_rule);
+ err = nft_delrule(&ctx, old_rule, false);
if (err < 0) {
nft_trans_destroy(trans);
goto err2;
@@ -2804,7 +2807,7 @@ static int nf_tables_delrule(struct net *net, struct sock *nlsk,
return PTR_ERR(rule);
}
- err = nft_delrule(&ctx, rule);
+ err = nft_delrule(&ctx, rule, false);
} else if (nla[NFTA_RULE_ID]) {
rule = nft_rule_lookup_byid(net, nla[NFTA_RULE_ID]);
if (IS_ERR(rule)) {
@@ -2812,7 +2815,7 @@ static int nf_tables_delrule(struct net *net, struct sock *nlsk,
return PTR_ERR(rule);
}
- err = nft_delrule(&ctx, rule);
+ err = nft_delrule(&ctx, rule, false);
} else {
err = nft_delrule_by_chain(&ctx);
}
