Re: [iptables PATCH v3 03/21] xtables-restore: Review chain handling
On Thu, Dec 27, 2018 at 07:55:40PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 20, 2018 at 04:09:04PM +0100, Phil Sutter wrote:
> > There is no need to "delete" (actually, remove from cache) a chain if
> > noflush wasn't given: While handling the corresponding table line,
> > 'table_flush' callback has already taken care of that.
> >
> > Streamlining the code further, move syntax checks to the top. If these
> > concede, there are three cases to distinguish:
> >
> > A) Given chain name matches a builtin one in current table, so assume it
> >exists already and just set policy and counters.
> >
> > B) Noflush was given and the (custom) chain exists already, flush it.
> >
> > C) Custom chain was either flushed (noflush not given) or didn't exist
> >before, create it.
> >
> > Signed-off-by: Phil Sutter
> > ---
> > iptables/nft-shared.h | 2 --
> > iptables/xtables-restore.c | 68 +++---
> > 2 files changed, 19 insertions(+), 51 deletions(-)
> >
> > diff --git a/iptables/nft-shared.h b/iptables/nft-shared.h
> > index 388abb97303ab..019c1f20e2939 100644
> > --- a/iptables/nft-shared.h
> > +++ b/iptables/nft-shared.h
> > @@ -245,8 +245,6 @@ struct nft_xt_restore_cb {
> > void (*table_new)(struct nft_handle *h, const char *table);
> > struct nftnl_chain_list *(*chain_list)(struct nft_handle *h,
> >const char *table);
> > - void (*chain_del)(struct nftnl_chain_list *clist, const char *curtable,
> > - const char *chain);
>
> I added to this patch description that chain_del is basically dead
> code since d1eb4d587297.
Ah, yes. But still, I had problems getting my automatic rule adding from
cache approach working with it because there was some ordering issue
with implicit base chain creation and this removal from cache thing.
> Thanks for disentangling this part of the code, looks better now.
Yes, apart from it getting into my way whatever simplifies those restore
parsers is a good thing I guess. :)
Thanks, Phil
Re: [iptables PATCH v3 11/21] xtables: Implement per chain rule cache
Hi Pablo,
On Thu, Dec 27, 2018 at 08:41:34PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 20, 2018 at 04:09:12PM +0100, Phil Sutter wrote:
> > @@ -1195,12 +1182,14 @@ err:
> > return NULL;
> > }
> >
> > -static struct nftnl_rule_list *nft_rule_list_get(struct nft_handle *h);
> > +static struct nftnl_chain *
> > +nft_chain_find(struct nft_handle *h, const char *table, const char *chain);
> >
> > int
> > nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
> > void *data, uint64_t handle, bool verbose)
> > {
> > + struct nftnl_chain *c;
> > struct nftnl_rule *r;
> > int type;
> >
> > @@ -1228,10 +1217,9 @@ nft_rule_append(struct nft_handle *h, const char
> > *chain, const char *table,
> > if (verbose)
> > h->ops->print_rule(r, 0, FMT_PRINT_RULE);
> >
> > - if (!nft_rule_list_get(h))
> > - return 0;
> > -
> > - nftnl_rule_list_add_tail(r, h->rule_cache);
> > + c = nft_chain_find(h, table, chain);
> > + if (c)
> > + nftnl_chain_rule_add_tail(r, c);
>
> I think we always have a chain here.
Not at this stage of the series: When only appending a rule in a single
arptables-nft command, the cache has not been populated. This is not a
problem per se, so we must allow for the above to fail. The old common
rule cache didn't require the actual chain to be in cache, that's why
this check is new.
> > return 1;
>
> Because we return success in any case.
>
> [...]
> > @@ -2024,16 +2028,16 @@ next:
> > int nft_rule_check(struct nft_handle *h, const char *chain,
> >const char *table, void *data, bool verbose)
> > {
> > - struct nftnl_rule_list *list;
> > + struct nftnl_chain *c;
> > struct nftnl_rule *r;
> >
> > nft_fn = nft_rule_check;
> >
> > - list = nft_rule_list_get(h);
> > - if (list == NULL)
> > + c = nft_chain_find(h, table, chain);
> > + if (!c)
>
> errno = ENOENT?
>
> Or chain is assumed to be found always?
The check command is supported by ip(6)tables-nft only, do_parse() makes
sure the chain exists in final sanity checks. But yes, setting ENOENT
here would be more consistent so I'll send a follow-up which adds it.
[...]
> > return 0;
> > + }
> >
> > - r = nft_rule_find(h, list, chain, table, data, -1);
> > + r = nft_rule_find(h, c, data, -1);
> > if (r != NULL) {
> > ret =__nft_rule_del(h, r);
> > if (ret < 0)
> > @@ -2136,9 +2144,9 @@ int nft_rule_insert(struct nft_handle *h, const char
> > *chain,
> > goto err;
> >
> > if (handle)
> > - nftnl_rule_list_insert_at(new_rule, r);
> > + nftnl_chain_rule_insert_at(new_rule, r);
>
> I wonder why we need nftnl_chain_rule_insert_at() if there is no chain
> parameter, see below.
Yes, it is just to help readability - and of course because we need at
least *some* form of wrapper around list_add() due to the opaque struct
nftnl_rule.
[...]
> > @@ -2174,16 +2184,18 @@ int nft_rule_replace(struct nft_handle *h, const
> > char *chain,
> > const char *table, void *data, int rulenum, bool verbose)
> > {
> > int ret = 0;
> > + struct nftnl_chain *c;
> > struct nftnl_rule *r;
> > - struct nftnl_rule_list *list;
> >
> > nft_fn = nft_rule_replace;
> >
> > - list = nft_rule_list_get(h);
> > - if (list == NULL)
> > + c = nft_chain_find(h, table, chain);
> > + if (!c) {
> > + errno = ENOENT;
> > return 0;
> > + }
> >
> > - r = nft_rule_find(h, list, chain, table, data, rulenum);
> > + r = nft_rule_find(h, c, data, rulenum);
> > if (r != NULL) {
> > DEBUGP("replacing rule with handle=%llu\n",
> > (unsigned long long)
>
> Here in nft_rule_replace() I can see we still use
> nftnl_rule_list_del() to remove an entry from the cache.
>
> No problem, since internally nftnl_rule_list_del() does the same as
> nftnl_chain_rule_del().
>
> We can probably deprecate nftnl_rule_list at some point, and rely
> entirely on nftnl_chain_rule_add() and so on.
Hmm. Yes, this is indeed a shortcoming of my patch to libnftnl: I did
not introduce a function to remove a rule from the chain's rule list.
I'll send a follow-up to fix this.
> [...]
> > @@ -3143,19 +3106,8 @@ static int nft_is_expr_compatible(struct nftnl_expr
> > *exp
Re: [iptables PATCH v3 16/21] xtables: Optimize user-defined chain deletion
Hi Pablo,
On Thu, Dec 27, 2018 at 08:48:00PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 20, 2018 at 04:09:17PM +0100, Phil Sutter wrote:
> [...]
> > + if (chain) {
> > + c = nftnl_chain_list_lookup_byname(list, chain);
> > + if (!c) {
> > + errno = ENOENT;
> > + return 0;
> > + }
> > + d.builtin_err = -2;
> > + ret = __nft_chain_user_del(c, &d);
> > + if (ret == -2)
>
> We can probably do an upfront check for built-in chain to avoid this
> special code? __nft_chain_user_del() is only called from
> nft_chain_user_del().
Well, __nft_chain_user_del() is either called for a user-specified chain
or for all chains of the given table. While that builtin_err field could
be avoided by having the nft_chain_builtin() check in the first case
before dispatching to __nft_chain_user_del(), in the second case
__nft_chain_user_del() still has to do the check so it doesn't try to
delete builtin chains by accident. By using this conditional error code,
the nft_chain_builtin() check doesn't happen twice when deleting a
user-specified chain.
If you think it's not worth it, just let me know and I'll send a
follow-up which removes it (and introduces the unavoidable double
check).
Cheers, Phil
Re: [iptables PATCH v3 17/21] xtables: Optimize list command with given chain
On Thu, Dec 27, 2018 at 08:54:08PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Dec 20, 2018 at 04:09:18PM +0100, Phil Sutter wrote:
> > Make use of nftnl_chain_list_lookup_byname() even if not listing a
> > specific rule. Introduce __nft_print_header() to consolidate chain value
> > extraction for printing with ops->print_header().
> >
> > Signed-off-by: Phil Sutter
> > ---
> > iptables/nft.c | 78 +-
> > 1 file changed, 32 insertions(+), 46 deletions(-)
> >
> > diff --git a/iptables/nft.c b/iptables/nft.c
> > index 250cae0a34e37..b11c390edcc10 100644
> > --- a/iptables/nft.c
> > +++ b/iptables/nft.c
> > @@ -2247,6 +2247,24 @@ static int nft_rule_count(struct nft_handle *h,
> > struct nftnl_chain *c)
> > return rule_ctr;
> > }
> >
> > +static void __nft_print_header(struct nft_handle *h,
> > + const struct nft_family_ops *ops,
> > + struct nftnl_chain *c, unsigned int format)
> > +{
> > + const char *chain_name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
> > + uint32_t policy = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
> > + bool basechain = !!nftnl_chain_get(c, NFTNL_CHAIN_HOOKNUM);
> > + uint32_t refs = nftnl_chain_get_u32(c, NFTNL_CHAIN_USE);
> > + uint32_t entries = nft_rule_count(h, c);
> > + struct xt_counters ctrs = {
> > + .pcnt = nftnl_chain_get_u64(c, NFTNL_CHAIN_PACKETS),
> > + .bcnt = nftnl_chain_get_u64(c, NFTNL_CHAIN_BYTES),
> > + };
>
> Maybe we can introduce a container structure for this.
>
> > + ops->print_header(format, chain_name, policy_name[policy],
> > + &ctrs, basechain, refs - entries, entries);
>
> So we can pass it to ->print_header.
>
> I would have preferred you add this in a initial patch, makes it
> harder to review. Please do so in the future.
Sorry for the inconvenience this caused.
I don't quite get your idea: Would you like to have a function
extracting the chain data into that container structure so
nft_rule_list() still calls ops->print_header() directly?
I can still do that, just let me know please.
Cheers, Phil
Re: [iptables PATCH v3 21/21] xtables: Do not change ruleset while listing
On Thu, Dec 27, 2018 at 08:59:09PM +0100, Pablo Neira Ayuso wrote: > On Thu, Dec 20, 2018 at 04:09:22PM +0100, Phil Sutter wrote: > > When only listing rules, avoid to create the basic ruleset. Initializing > > the latter is still needed so that a completely empty ruleset does not > > lead to no output. But with builtin chains being added to cache > > immediately, there is no need to push the changes to the kernel anymore. > > Avoid this by calling nft_abort() in the right spots. > > > > Signed-off-by: Phil Sutter > > --- > > iptables/xtables-arp.c | 1 + > > iptables/xtables-eb.c | 1 + > > iptables/xtables.c | 4 > > 3 files changed, 6 insertions(+) > > > > diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c > > index 2f369d9aadb01..10cc4c9fbc875 100644 > > --- a/iptables/xtables-arp.c > > +++ b/iptables/xtables-arp.c > > @@ -1366,6 +1366,7 @@ int do_commandarp(struct nft_handle *h, int argc, > > char *argv[], char **table, > >options&OPT_NUMERIC, > >/*options&OPT_EXPANDED*/0, > >options&OPT_LINENUMBERS); > > + nft_abort(h); > > Hm, this call to nft_abort() is confusing. Listing does not require a > batch mode. The problem I'm trying to ship around is that we unconditionally create the compat ruleset if not present yet. And in order to list the empty chains if user calls e.g. 'iptables -L' with a completely empty ruleset, we need to add the compat ruleset at least to the internal cache. But this also creates the batch jobs which are not required when merely listing the ruleset. The alternative to just removing the batch jobs via nft_abort() before the obligatory call to nft_commit() would be to distinguish between ruleset listing and other tasks in nft_xt_builtin_init(). For the sake of simplicity, I didn't choose that option. > This patch is new in the batch IIRC, will keep this back until we > discuss if there is a better fix. Yes, it is a follow-up made possible by the no longer required early commit the rule insert position fix allowed for. The larger goal behind this is to reduce (needless) impact of various commands on the ruleset, in this case the creation of tables/chains in list commands. At some point I would like to have iptables-nft create only those parts of compat ruleset which are required for the command at hand. E.g.: | # iptables-nft -A INPUT -j ACCEPT Should not create any chains other than 'ip filter INPUT'. This came from a complaint that 'iptables-nft -F' creates things where it should instead remove them. With legacy iptables, users could call 'iptables -F; iptables -X' and then unload netfilter kernel modules. With iptables-nft, this is not possible. Cheers, Phil
[libnftnl PATCH] src: chain: Add missing nftnl_chain_rule_del()
Although identical to nftnl_rule_list_del(), this function adheres to
the common naming style of per chain rule list routines introduced
earlier, therefore helps with deprecating the global rule list API at a
later point.
Fixes: e33798478176f ("chain: Support per chain rules list")
Signed-off-by: Phil Sutter
---
include/libnftnl/chain.h | 1 +
src/chain.c | 6 ++
2 files changed, 7 insertions(+)
diff --git a/include/libnftnl/chain.h b/include/libnftnl/chain.h
index 64e10e91aaefe..163a824c457c2 100644
--- a/include/libnftnl/chain.h
+++ b/include/libnftnl/chain.h
@@ -56,6 +56,7 @@ int32_t nftnl_chain_get_s32(const struct nftnl_chain *c,
uint16_t attr);
uint64_t nftnl_chain_get_u64(const struct nftnl_chain *c, uint16_t attr);
void nftnl_chain_rule_add(struct nftnl_rule *rule, struct nftnl_chain *c);
+void nftnl_chain_rule_del(struct nftnl_rule *rule);
void nftnl_chain_rule_add_tail(struct nftnl_rule *rule, struct nftnl_chain *c);
void nftnl_chain_rule_insert_at(struct nftnl_rule *rule, struct nftnl_rule
*pos);
diff --git a/src/chain.c b/src/chain.c
index 03eeb655ae572..5f8eb5ca93e95 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -429,6 +429,12 @@ void nftnl_chain_rule_add(struct nftnl_rule *rule, struct
nftnl_chain *c)
list_add(&rule->head, &c->rule_list);
}
+EXPORT_SYMBOL(nftnl_chain_rule_del);
+void nftnl_chain_rule_del(struct nftnl_rule *r)
+{
+ list_del(&r->head);
+}
+
EXPORT_SYMBOL(nftnl_chain_rule_add_tail);
void nftnl_chain_rule_add_tail(struct nftnl_rule *rule, struct nftnl_chain *c)
{
--
2.19.0
[PATCH] net: nf_tables: Fix for endless loop when dumping ruleset
__nf_tables_dump_rules() stores the current idx value into cb->args[0]
before returning to caller. With multiple chains present, cb->args[0] is
therefore updated after each chain's rules have been traversed. This
though causes the final nf_tables_dump_rules() run (which should return
an skb->len of zero since no rules are left to dump) to continue dumping
rules for each but the first chain. Fix this by moving the cb->args[0]
update to nf_tables_dump_rules().
With no final action to be performed anymore in
__nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc'
variable - instead return the appropriate value directly.
Fixes: 241faeceb849c ("netfilter: nf_tables: Speed up selective rule dumps")
Signed-off-by: Phil Sutter
---
net/netfilter/nf_tables_api.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 09ad796d9904e..68d55f122259e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2304,7 +2304,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
struct net *net = sock_net(skb->sk);
unsigned int s_idx = cb->args[0];
const struct nft_rule *rule;
- int rc = 1;
list_for_each_entry_rcu(rule, &chain->rules, list) {
if (!nft_is_active(net, rule))
@@ -2321,16 +2320,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
NLM_F_MULTI | NLM_F_APPEND,
table->family,
table, chain, rule) < 0)
- goto out_unfinished;
+ return 1;
nl_dump_check_consistent(cb, nlmsg_hdr(skb));
cont:
(*idx)++;
}
- rc = 0;
-out_unfinished:
- cb->args[0] = *idx;
- return rc;
+ return 0;
}
static int nf_tables_dump_rules(struct sk_buff *skb,
@@ -2382,6 +2378,8 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
}
done:
rcu_read_unlock();
+
+ cb->args[0] = idx;
return skb->len;
}
--
2.19.0
[iptables PATCH v4 3/5] xtables: Set errno in nft_rule_check() if chain not found
With this, the explicit check for chain existence can be removed from
xtables.c since all related commands do this now.
Note that this effectively changes the error message printed by
iptables-nft when given a non-existing chain, but the new error
message(s) conform with those printed by legacy iptables.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 12 +++-
iptables/xtables.c | 4
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index dafb879ebd6f0..1ce1ecdd276be 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2007,17 +2007,19 @@ int nft_rule_check(struct nft_handle *h, const char
*chain,
c = nft_chain_find(h, table, chain);
if (!c)
- return 0;
+ goto fail_enoent;
r = nft_rule_find(h, c, data, -1);
- if (r == NULL) {
- errno = ENOENT;
- return 0;
- }
+ if (r == NULL)
+ goto fail_enoent;
+
if (verbose)
h->ops->print_rule(r, 0, FMT_PRINT_RULE);
return 1;
+fail_enoent:
+ errno = ENOENT;
+ return 0;
}
int nft_rule_delete(struct nft_handle *h, const char *chain,
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 24a6e234bcf4b..da11e8cc159a0 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1064,10 +1064,6 @@ void do_parse(struct nft_handle *h, int argc, char
*argv[],
p->chain);
}
- if (!p->xlate && !nft_chain_exists(h, p->table, p->chain))
- xtables_error(OTHER_PROBLEM,
- "Chain '%s' does not exist", p->chain);
-
if (!p->xlate && !cs->target && strlen(cs->jumpto) > 0 &&
!nft_chain_exists(h, p->table, cs->jumpto))
xtables_error(PARAMETER_PROBLEM,
--
2.19.0
[iptables PATCH v4 1/5] nft: Simplify nft_is_chain_compatible()
Make use of nft_{table,chain}_builtin_find() instead of open-coding the
list traversal. Since code is pretty obvious now, drop the comments
added earlier.
Fixes: e774b15299c27 ("nft: Review is_*_compatible() routines")
Signed-off-by: Phil Sutter
---
iptables/nft.c | 34 +-
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index 1fd3837f2d334..25e538b7e35d7 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3077,11 +3077,12 @@ static int nft_is_rule_compatible(struct nftnl_rule
*rule, void *data)
static int nft_is_chain_compatible(struct nftnl_chain *c, void *data)
{
- const struct builtin_chain *chains = NULL, *chain = NULL;
- const char *table, *name, *type;
+ const struct builtin_table *table;
+ const struct builtin_chain *chain;
+ const char *tname, *cname, *type;
struct nft_handle *h = data;
enum nf_inet_hooks hook;
- int i, prio;
+ int prio;
if (nftnl_rule_foreach(c, nft_is_rule_compatible, NULL))
return -1;
@@ -3089,33 +3090,16 @@ static int nft_is_chain_compatible(struct nftnl_chain
*c, void *data)
if (!nft_chain_builtin(c))
return 0;
- /* find chain's table in builtin tables */
- table = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
- for (i = 0; i < NFT_TABLE_MAX; i++) {
- const char *cur_table = h->tables[i].name;
-
- if (!cur_table || strcmp(cur_table, table))
- continue;
-
- chains = h->tables[i].chains;
- break;
- }
- if (!chains)
+ tname = nftnl_chain_get_str(c, NFTNL_CHAIN_TABLE);
+ table = nft_table_builtin_find(h, tname);
+ if (!table)
return -1;
- /* find chain in builtin chain list */
- name = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
- for (i = 0; i < NF_INET_NUMHOOKS && chains[i].name; i++) {
- if (strcmp(name, chains[i].name))
- continue;
-
- chain = &chains[i];
- break;
- }
+ cname = nftnl_chain_get_str(c, NFTNL_CHAIN_NAME);
+ chain = nft_chain_builtin_find(table, cname);
if (!chain)
return -1;
- /* compare properties */
type = nftnl_chain_get_str(c, NFTNL_CHAIN_TYPE);
prio = nftnl_chain_get_u32(c, NFTNL_CHAIN_PRIO);
hook = nftnl_chain_get_u32(c, NFTNL_CHAIN_HOOKNUM);
--
2.19.0
[iptables PATCH v4 4/5] xtables: Fix for inserting rule at wrong position
iptables-restore allows to insert rules at a certain position which is
problematic for iptables-nft to realize since rule position is not
determined by number but handle of previous or following rule.
To fix this, make use of the rule cache (which holds in-kernel rules
with their associated handle already). Insert new rules at the right
position into that cache, then at commit time (i.e., in nft_action())
traverse each chain's rule list for new rules to add:
* New rules at beginning of list are inserted in reverse order without
reference of another rule, so that each consecutive rule is added
before all previous ones.
* Each time an "old" (i.e., in-kernel) rule is encountered, its handle
is stored (overwriting any previous ones').
* New rules following an old one are appended to the old one (by
specifying its handle) in reverse order, so that each consecutive rule
is inserted between the old one and previously appended ones.
For this to function, built-in chains need to be in cache. Consequently,
batch jobs with type NFT_COMPAT_CHAIN_ADD must not delete their payload.
A nice side-effect of that is when initializing builtin tables/chains,
no explicit call to nft_commit() is required anymore.
Signed-off-by: Phil Sutter
---
iptables/nft.c| 182 +++---
.../ipt-restore/0003-restore-ordering_0 | 94 +
.../testcases/iptables/0005-rule-replace_0| 38
3 files changed, 243 insertions(+), 71 deletions(-)
create mode 100755
iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
create mode 100755 iptables/tests/shell/testcases/iptables/0005-rule-replace_0
diff --git a/iptables/nft.c b/iptables/nft.c
index 1ce1ecdd276be..ab1a361905287 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -644,6 +644,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
return;
batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
+ nftnl_chain_list_add_tail(c, h->table[table->type].chain_cache);
}
/* find if built-in table already exists */
@@ -1189,7 +1190,6 @@ nft_rule_append(struct nft_handle *h, const char *chain,
const char *table,
{
struct nftnl_chain *c;
struct nftnl_rule *r;
- int type;
/* If built-in chains don't exist for this table, create them */
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -1203,21 +1203,21 @@ nft_rule_append(struct nft_handle *h, const char
*chain, const char *table,
if (handle > 0) {
nftnl_rule_set(r, NFTNL_RULE_HANDLE, &handle);
- type = NFT_COMPAT_RULE_REPLACE;
- } else
- type = NFT_COMPAT_RULE_APPEND;
-
- if (batch_rule_add(h, type, r) < 0) {
- nftnl_rule_free(r);
- return 0;
+ if (batch_rule_add(h, NFT_COMPAT_RULE_REPLACE, r) < 0) {
+ nftnl_rule_free(r);
+ return 0;
+ }
}
if (verbose)
h->ops->print_rule(r, 0, FMT_PRINT_RULE);
c = nft_chain_find(h, table, chain);
- if (c)
- nftnl_chain_rule_add_tail(r, c);
+ if (!c) {
+ errno = ENOENT;
+ return 0;
+ }
+ nftnl_chain_rule_add_tail(r, c);
return 1;
}
@@ -2050,37 +2050,11 @@ int nft_rule_delete(struct nft_handle *h, const char
*chain,
return ret;
}
-static struct nftnl_rule *
-nft_rule_add(struct nft_handle *h, const char *chain,
-const char *table, struct iptables_command_state *cs,
-uint64_t handle, bool verbose)
-{
- struct nftnl_rule *r;
-
- r = nft_rule_new(h, chain, table, cs);
- if (r == NULL)
- return NULL;
-
- if (handle > 0)
- nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, handle);
-
- if (batch_rule_add(h, NFT_COMPAT_RULE_INSERT, r) < 0) {
- nftnl_rule_free(r);
- return NULL;
- }
-
- if (verbose)
- h->ops->print_rule(r, 0, FMT_PRINT_RULE);
-
- return r;
-}
-
int nft_rule_insert(struct nft_handle *h, const char *chain,
const char *table, void *data, int rulenum, bool verbose)
{
- struct nftnl_rule *r, *new_rule;
+ struct nftnl_rule *r = NULL, *new_rule;
struct nftnl_chain *c;
- uint64_t handle = 0;
/* If built-in chains don't exist for this table, create them */
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2095,31 +2069,23 @@ int nft_rule_insert(struct nft_handle *h, const char
*chain,
}
if (rulenum > 0) {
- r = nft_rule_find(h, c, data, rulenum);
+ r = nft_rule_find(h, c, data, rulenum - 1);
if (r == NULL) {
- /* special case: iptab
[iptables PATCH v4 5/5] xtables: Do not change ruleset while listing
When only listing rules, avoid to create the basic ruleset. Initializing
the latter is still needed so that a completely empty ruleset does not
lead to no output. But with builtin chains being added to cache
immediately, there is no need to push the changes to the kernel anymore.
Avoid this by calling nft_abort() in the right spots.
Signed-off-by: Phil Sutter
---
iptables/xtables-arp.c | 1 +
iptables/xtables-eb.c | 1 +
iptables/xtables.c | 4
3 files changed, 6 insertions(+)
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 2f369d9aadb01..10cc4c9fbc875 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -1366,6 +1366,7 @@ int do_commandarp(struct nft_handle *h, int argc, char
*argv[], char **table,
options&OPT_NUMERIC,
/*options&OPT_EXPANDED*/0,
options&OPT_LINENUMBERS);
+ nft_abort(h);
break;
case CMD_FLUSH:
ret = nft_rule_flush(h, chain, *table, options & OPT_VERBOSE);
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 16d874120c0bb..a9a6fccb53c6a 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -1288,6 +1288,7 @@ print_zero:
/*flags&OPT_EXPANDED*/0,
flags&LIST_N,
flags&LIST_C);
+ nft_abort(h);
}
if (flags & OPT_ZERO) {
ret = nft_chain_zero_counters(h, chain, *table, 0);
diff --git a/iptables/xtables.c b/iptables/xtables.c
index da11e8cc159a0..28223e8edc799 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1139,6 +1139,8 @@ int do_commandx(struct nft_handle *h, int argc, char
*argv[], char **table,
cs.options & OPT_NUMERIC,
cs.options & OPT_EXPANDED,
cs.options & OPT_LINENUMBERS);
+ if (p.command == CMD_LIST)
+ nft_abort(h);
if (ret && (p.command & CMD_ZERO)) {
ret = nft_chain_zero_counters(h, p.chain, p.table,
cs.options & OPT_VERBOSE);
@@ -1154,6 +1156,8 @@ int do_commandx(struct nft_handle *h, int argc, char
*argv[], char **table,
case CMD_LIST_RULES|CMD_ZERO_NUM:
ret = list_rules(h, p.chain, p.table, p.rulenum,
cs.options & OPT_VERBOSE);
+ if (p.command == CMD_LIST_RULES)
+ nft_abort(h);
if (ret && (p.command & CMD_ZERO)) {
ret = nft_chain_zero_counters(h, p.chain, p.table,
cs.options & OPT_VERBOSE);
--
2.19.0
[iptables PATCH v4 2/5] nft: Simplify flush_chain_cache()
With all the checks for 'tablename' being non-NULL, this code was rather
stupid and really hard to read. And the fix is indeed quite simple: If a
table name was given, use nft_table_builtin_find() and just flush its
chain cache. Otherwise iterate over all builtin tables without any
conditionals for 'tablename'.
Fixes: d4b0d248cc057 ("nft: Reduce indenting level in flush_chain_cache()")
Signed-off-by: Phil Sutter
---
iptables/nft.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index 25e538b7e35d7..dafb879ebd6f0 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -793,27 +793,25 @@ static int __flush_chain_cache(struct nftnl_chain *c,
void *data)
static void flush_chain_cache(struct nft_handle *h, const char *tablename)
{
+ const struct builtin_table *table;
int i;
+ if (tablename) {
+ table = nft_table_builtin_find(h, tablename);
+ if (!table || !h->table[table->type].chain_cache)
+ return;
+ nftnl_chain_list_foreach(h->table[table->type].chain_cache,
+__flush_chain_cache, NULL);
+ return;
+ }
+
for (i = 0; i < NFT_TABLE_MAX; i++) {
if (h->tables[i].name == NULL)
continue;
- if (tablename && strcmp(h->tables[i].name, tablename))
+ if (!h->table[i].chain_cache)
continue;
- if (!h->table[i].chain_cache) {
- if (tablename)
- return;
- continue;
- }
-
- if (tablename) {
- nftnl_chain_list_foreach(h->table[i].chain_cache,
-__flush_chain_cache, NULL);
- return;
- }
-
nftnl_chain_list_free(h->table[i].chain_cache);
h->table[i].chain_cache = NULL;
}
--
2.19.0
[iptables PATCH v4 0/5] Separate rule cache per chain et al.
Admittedly, the subject is not quite related anymore yet I kept it to
make reviewers recognize this as v4 of the series with same subject.
Contained in here are the leftover patches not applied from v3 (patches
4 and 5) which remain unchanged (apart from being reapplied on top of
the others) and three new ones addressing feedback from v3 (I put those
first because they are follow-ups to already applied ones):
* Patch 1 changes nft_is_chain_compatible() to not open-code builtin
table and chain traversal as suggested in review of commit
e774b15299c27 ("nft: Review is_*_compatible() routines").
* Patch 2 eliminates the weird code I seemed to have written with
crossed-eyes after reading too many lines of diff output (commit
d4b0d248cc057 ("nft: Reduce indenting level in flush_chain_cache()").
* I found patch 3 in my stack of yet to be submitted patches when
addressing feedback for commit 947c51c95edbb ("xtables: Implement per
chain rule cache").
Phil Sutter (5):
nft: Simplify nft_is_chain_compatible()
nft: Simplify flush_chain_cache()
xtables: Set errno in nft_rule_check() if chain not found
xtables: Fix for inserting rule at wrong position
xtables: Do not change ruleset while listing
iptables/nft.c| 252 ++
.../ipt-restore/0003-restore-ordering_0 | 94 +++
.../testcases/iptables/0005-rule-replace_0| 38 +++
iptables/xtables-arp.c| 1 +
iptables/xtables-eb.c | 1 +
iptables/xtables.c| 8 +-
6 files changed, 276 insertions(+), 118 deletions(-)
create mode 100755
iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
create mode 100755 iptables/tests/shell/testcases/iptables/0005-rule-replace_0
--
2.19.0
Re: [PATCH] net: nf_tables: Fix for endless loop when dumping ruleset
Hi Florian, On Sun, Dec 30, 2018 at 08:10:28PM +0100, Florian Westphal wrote: > Phil Sutter wrote: > > __nf_tables_dump_rules() stores the current idx value into cb->args[0] > > before returning to caller. With multiple chains present, cb->args[0] is > > therefore updated after each chain's rules have been traversed. This > > though causes the final nf_tables_dump_rules() run (which should return > > an skb->len of zero since no rules are left to dump) to continue dumping > > rules for each but the first chain. Fix this by moving the cb->args[0] > > update to nf_tables_dump_rules(). > > > > With no final action to be performed anymore in > > __nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc' > > variable - instead return the appropriate value directly. > > Looks good, but I think this is a bug too: > >list = rhltable_lookup(&table->chains_ht, ctx->chain, > nft_chain_ht_params); >if (!list) > goto done; > > I think this should move to next table instead. Hmm. Yes, assuming that specifying no table but only chain is a valid use-case, this should indeed continue with the next table. I'll send a v2 which includes that fix as well. > (Its not related to the bug at hand though). And not easy to trigger since all known users pass either both table and chain or none of them. :) Thanks, Phil
[PATCH v2] net: nf_tables: Fix speedup of selective rule dumps
Fixed commit had two problems. The significant one being an endless loop
when dumping a ruleset containing multiple non-empty chains:
__nf_tables_dump_rules() stores the current idx value into cb->args[0]
before returning to caller. With multiple chains present, cb->args[0] is
therefore updated after each chain's rules have been traversed. This
though causes the final nf_tables_dump_rules() run (which should return
an skb->len of zero since no rules are left to dump) to continue dumping
rules for each but the first chain. Fix this by moving the cb->args[0]
update to nf_tables_dump_rules().
With no final action to be performed anymore in
__nf_tables_dump_rules(), drop 'out_unfinished' jump label and 'rc'
variable - instead return the appropriate value directly.
The second problem was inability to find given chain under certain
circumstances:
If no table but only chain name was specified in user's request,
nf_tables_dump_rules() returns after not finding the given chain in the
first table instead of trying other ones. Fix this by continuing the
table traversal, but make sure the loop is still left immediately if a
table name was given.
Fixes: 241faeceb849c ("netfilter: nf_tables: Speed up selective rule dumps")
Signed-off-by: Phil Sutter
---
Changes since v1:
- Fix second potential issue as well, adjust commit message accordingly.
In fact, the second problem fixed here is for a use-case nf_tables
shouldn't even support: Dumping rules of a chain without specifying the
table may lead to unexpected results if multiple chains with same name
in different tables are present. Which chain's rules are dumped depends
entirely on the ordering of tables in kernel's table list. The latter is
subject to change if e.g. the data structure is changed from list to
rhltable.
---
net/netfilter/nf_tables_api.c | 14 ++
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 09ad796d9904e..d6be77618d556 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2304,7 +2304,6 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
struct net *net = sock_net(skb->sk);
unsigned int s_idx = cb->args[0];
const struct nft_rule *rule;
- int rc = 1;
list_for_each_entry_rcu(rule, &chain->rules, list) {
if (!nft_is_active(net, rule))
@@ -2321,16 +2320,13 @@ static int __nf_tables_dump_rules(struct sk_buff *skb,
NLM_F_MULTI | NLM_F_APPEND,
table->family,
table, chain, rule) < 0)
- goto out_unfinished;
+ return 1;
nl_dump_check_consistent(cb, nlmsg_hdr(skb));
cont:
(*idx)++;
}
- rc = 0;
-out_unfinished:
- cb->args[0] = *idx;
- return rc;
+ return 0;
}
static int nf_tables_dump_rules(struct sk_buff *skb,
@@ -2360,7 +2356,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
list = rhltable_lookup(&table->chains_ht, ctx->chain,
nft_chain_ht_params);
if (!list)
- goto done;
+ goto next_table;
rhl_for_each_entry_rcu(chain, tmp, list, rhlhead) {
if (!nft_is_active(net, chain))
@@ -2376,12 +2372,14 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
if (__nf_tables_dump_rules(skb, &idx, cb, table, chain))
goto done;
}
-
+next_table:
if (ctx && ctx->table)
break;
}
done:
rcu_read_unlock();
+
+ cb->args[0] = idx;
return skb->len;
}
--
2.19.0
Re: [iptables PATCH v4 4/5] xtables: Fix for inserting rule at wrong position
Hi Pablo, On Thu, Jan 10, 2019 at 12:50:34AM +0100, Pablo Neira Ayuso wrote: > On Sun, Dec 30, 2018 at 08:06:11PM +0100, Phil Sutter wrote: > > iptables-restore allows to insert rules at a certain position which is > > problematic for iptables-nft to realize since rule position is not > > determined by number but handle of previous or following rule. > > Looking at this late postprocessing in nft_action()... a bit of > brainstorming. > > Why not same approach as in rule_translate_index() in nftables? I > guess answer is this is also incomplete, right? Yes, that is not sufficient, see below for an example. > If that is not enough, we could add support for NFTA_RULE_ID to > nf_tables_newrule(). This NFTA_RULE_ID attribute provides a way to > refer to rules coming in this batch. It is similar to NFTA_SET_ID, > however, currently it is only supported from the rule deletion path. > This ID is internal to the transaction - we can freely allocate IDs > for the new rules coming in the batch. My concern is that we may have > problems later on if we do not address limitations in the kernel > interface. That might help, yes. Though depending on how this will be applied and iptables-restore input, we may still run into ugly issues. > Or thinking if we can do this with the existing interface... see > below. > > > To fix this, make use of the rule cache (which holds in-kernel rules > > with their associated handle already). Insert new rules at the right > > position into that cache, then at commit time (i.e., in nft_action()) > > traverse each chain's rule list for new rules to add: > > > > * New rules at beginning of list are inserted in reverse order without > > reference of another rule, so that each consecutive rule is added > > before all previous ones. > > I think this refers to rules like: > > iptables -I chain 1 ... > > This case we can just handle it by doing plain rule insertion. Not quite, a user may simply do: -I chain 1 -I chain 1 Legacy code inserts them in order (i.e., rule 2 follows rule 1) while the simple approach you have in mind reverses the ordering. > > * Each time an "old" (i.e., in-kernel) rule is encountered, its handle > > is stored (overwriting any previous ones'). > > > > * New rules following an old one are appended to the old one (by > > specifying its handle) in reverse order, so that each consecutive rule > > is inserted between the old one and previously appended ones. > > For existing rules, we already have an absolute reference through the > handle, then the NLM_F_APPEND flag tells if we want to place it before > or after that rule. > > What scenario is forcing us to do this late postprocessing below that > we cannot handle with the existing interface? :-) Kernel has this: -A chain # handle 10 -A chain # handle 20 User does: -I chain 2 -I chain 3 Handling rule a in iptables-nft-restore is easy, just insert before handle 20 or append to handle 10. If we don't add this rule to cache, rule b will be appended to ruleset (since the rulenum it refers to is larger than the number of rules in cache), although it should be inserted in between rule a and rule 2. So we need to add new rules to cache, otherwise following rule's numbers refer to the wrong rule. Now consider a cache of: - rule 1 # handle 10 - rule 2 # new - rule 3 # new And we parse this command: -I chain 3 Due to missing handle, we can neither append to rule 2 nor insert before rule 3. How do we get rule 4 in between rule 2 and rule 3? Given the freedom users have when creating input to iptables-nft-restore, all this quickly turns into a can of worms. At least I am not able to keep all possible situations in mind in order to get the code right. That's why I chose the most straight forward approach that occurred to me: Put all rules in cache at the right spot and do the insert/append with handle dance later when all rules are in place. Cheers, Phil
Re: [iptables PATCH v4 5/5] xtables: Do not change ruleset while listing
On Thu, Jan 10, 2019 at 01:28:22AM +0100, Pablo Neira Ayuso wrote: > On Sun, Dec 30, 2018 at 08:06:12PM +0100, Phil Sutter wrote: > > diff --git a/iptables/xtables.c b/iptables/xtables.c > > index da11e8cc159a0..28223e8edc799 100644 > > --- a/iptables/xtables.c > > +++ b/iptables/xtables.c > > @@ -1139,6 +1139,8 @@ int do_commandx(struct nft_handle *h, int argc, char > > *argv[], char **table, > >cs.options & OPT_NUMERIC, > >cs.options & OPT_EXPANDED, > >cs.options & OPT_LINENUMBERS); > > + if (p.command == CMD_LIST) > > + nft_abort(h); > > Goal of this patch is to reset the batch of any pending object to be > added before -L call? The goal is to prevent the unconditional commit (e.g. in xtables_main()) from adding builtin tables/chains when listing only. In the past, we had to commit early after creating builtin tables/chains to make them appear in ruleset listing. A problem we faced with multiple times was that 'nft flush ruleset ; iptables-nft -L' did not print anything. Fix was to commit early (commit a4e78370af849) and flush cache so it's refreshed (commit 206033ede9461). Since builtin tables/chains are manually added to cache, the early commit and cache flush is no longer required. But after listing the empty ruleset, the final commit still adds the builtin tables/chains although not required for list command. This patch eliminates those pointless batch jobs. Cheers, Phil
Re: [iptables PATCH v4 3/5] xtables: Set errno in nft_rule_check() if chain not found
Hi Pablo, On Wed, Jan 09, 2019 at 05:25:45PM +0100, Pablo Neira Ayuso wrote: > On Sun, Dec 30, 2018 at 08:06:10PM +0100, Phil Sutter wrote: > [...] > > diff --git a/iptables/xtables.c b/iptables/xtables.c > > index 24a6e234bcf4b..da11e8cc159a0 100644 > > --- a/iptables/xtables.c > > +++ b/iptables/xtables.c > > @@ -1064,10 +1064,6 @@ void do_parse(struct nft_handle *h, int argc, char > > *argv[], > >p->chain); > > } > > > > - if (!p->xlate && !nft_chain_exists(h, p->table, p->chain)) > > - xtables_error(OTHER_PROBLEM, > > - "Chain '%s' does not exist", p->chain); > > After this chunk is applied I get this error: > > # iptables-nft -I test > iptables: Index of insertion too big. > > We can probably get this aligned with legacy, ie. > > iptables: No chain/target/match by that name. I have a patch series dealing with any error message misalignment between legacy and nft variants waiting to be submitted upstream. I just didn't want to send too many series depending upon each other at once. So you may consider this virtually fixed already. :) Cheers, Phil
Re: [iptables PATCH v4 4/5] xtables: Fix for inserting rule at wrong position
Hi Pablo, On Thu, Jan 10, 2019 at 11:53:15PM +0100, Pablo Neira Ayuso wrote: > On Thu, Jan 10, 2019 at 11:17:36AM +0100, Phil Sutter wrote: > > On Thu, Jan 10, 2019 at 12:50:34AM +0100, Pablo Neira Ayuso wrote: > > > On Sun, Dec 30, 2018 at 08:06:11PM +0100, Phil Sutter wrote: [...] > > > If that is not enough, we could add support for NFTA_RULE_ID to > > > nf_tables_newrule(). This NFTA_RULE_ID attribute provides a way to > > > refer to rules coming in this batch. It is similar to NFTA_SET_ID, > > > however, currently it is only supported from the rule deletion path. > > > This ID is internal to the transaction - we can freely allocate IDs > > > for the new rules coming in the batch. My concern is that we may have > > > problems later on if we do not address limitations in the kernel > > > interface. > > > > That might help, yes. Though depending on how this will be applied and > > iptables-restore input, we may still run into ugly issues. > > I'm under the impression that the problem is the lack of handle for > rules that are not in the kernel, see below. Yes, this is probably the best choice. > > > Or thinking if we can do this with the existing interface... see > > > below. > > > > > > > To fix this, make use of the rule cache (which holds in-kernel rules > > > > with their associated handle already). Insert new rules at the right > > > > position into that cache, then at commit time (i.e., in nft_action()) > > > > traverse each chain's rule list for new rules to add: > > > > > > > > * New rules at beginning of list are inserted in reverse order without > > > > reference of another rule, so that each consecutive rule is added > > > > before all previous ones. > > > > > > I think this refers to rules like: > > > > > > iptables -I chain 1 ... > > > > > > This case we can just handle it by doing plain rule insertion. > > > > Not quite, a user may simply do: > > > > -I chain 1 > > -I chain 1 > > > > Legacy code inserts them in order (i.e., rule 2 follows rule 1) while > > the simple approach you have in mind reverses the ordering. > > I might be misunderstanding anything from above, but legacy is > reversing the ordering here: Oh, crap. You're right of course, I wrote the above having my postprocessing algorithm in mind: When adding the above two rules to the batch, they will be reversed (so cache contains them in correct order). During postprocessing, I have to reconstruct the user-provided ordering, i.e. do the insert without reference in reverse. Of course this is not required when sticking to the old code which generates the batch jobs right away. [...] > I think we have two scenarios: > > #1 normal iptables-restore: in this case, kernel rules are ignored >since we assume an implicit flush. In this case, we need a variant >of batch_rule_add_at() that allows us to place the rules at the >right position. Since this is the same as restore with --noflush and an empty ruleset in kernel, there is no need to treat this separately - the existing code which creates a flush job and removes any existing rules from cache is fine. > #2 --noflush is specified: in this case, two sub-scenarios: > > #2.1 we find an existing rule with a handle at the position, we use the > rule handle that we found as NFTA_RULE_POSITION to place the rule. > We can use the existing batch_rule_add(). > > #2.2 we find a new rule at the position that has no handle, we use the > batch_rule_add_at() variant that allows us to place the rule at > the right position. The batch_rule_add_at() variant takes the > rule we found as parameter, so we can find it in the batch > through pointer. Sounds good, I'll give it a go. Thanks, Phil
[libnftnl PATCH] src: chain: Fix nftnl_chain_rule_insert_at()
Extrapolating from iptables nomenclature, one would expect that "insert"
means to prepend the new item to the referenced one, not append. Change
nftnl_chain_rule_insert_at() to do just that and introduce
nftnl_chain_rule_append_at() to insert a rule after the referenced one.
Signed-off-by: Phil Sutter
---
Given that per chain rule API has not yet made it into a release, I
guess we can still do that. If not, please feel free to reject and I'll
add two symbols with new names for insert/append operations and leave
the existing code alone.
---
include/libnftnl/chain.h | 1 +
src/chain.c | 6 ++
src/libnftnl.map | 1 +
3 files changed, 8 insertions(+)
diff --git a/include/libnftnl/chain.h b/include/libnftnl/chain.h
index 163a824c457c2..31b48cf32bed8 100644
--- a/include/libnftnl/chain.h
+++ b/include/libnftnl/chain.h
@@ -59,6 +59,7 @@ void nftnl_chain_rule_add(struct nftnl_rule *rule, struct
nftnl_chain *c);
void nftnl_chain_rule_del(struct nftnl_rule *rule);
void nftnl_chain_rule_add_tail(struct nftnl_rule *rule, struct nftnl_chain *c);
void nftnl_chain_rule_insert_at(struct nftnl_rule *rule, struct nftnl_rule
*pos);
+void nftnl_chain_rule_append_at(struct nftnl_rule *rule, struct nftnl_rule
*pos);
struct nlmsghdr;
diff --git a/src/chain.c b/src/chain.c
index 5f8eb5ca93e95..26f9b9d61053a 100644
--- a/src/chain.c
+++ b/src/chain.c
@@ -443,6 +443,12 @@ void nftnl_chain_rule_add_tail(struct nftnl_rule *rule,
struct nftnl_chain *c)
EXPORT_SYMBOL(nftnl_chain_rule_insert_at);
void nftnl_chain_rule_insert_at(struct nftnl_rule *rule, struct nftnl_rule
*pos)
+{
+ list_add_tail(&rule->head, &pos->head);
+}
+
+EXPORT_SYMBOL(nftnl_chain_rule_append_at);
+void nftnl_chain_rule_append_at(struct nftnl_rule *rule, struct nftnl_rule
*pos)
{
list_add(&rule->head, &pos->head);
}
diff --git a/src/libnftnl.map b/src/libnftnl.map
index 0d3be32263eee..695b40ea920b5 100644
--- a/src/libnftnl.map
+++ b/src/libnftnl.map
@@ -341,6 +341,7 @@ LIBNFTNL_12 {
nftnl_chain_rule_add;
nftnl_chain_rule_add_tail;
nftnl_chain_rule_insert_at;
+ nftnl_chain_rule_append_at;
nftnl_rule_foreach;
nftnl_rule_iter_create;
nftnl_rule_iter_next;
--
2.20.1
[PATCH] netfilter: nf_tables: Support RULE_ID reference in new rule
To allow for a batch to contain rules in arbitrary ordering, introduce
NFTA_RULE_POSITION_ID attribute which works just like NFTA_RULE_POSITION
but contains the ID of another rule within the same batch. This helps
iptables-nft-restore handling dumps with mixed insert/append commands
correctly.
Note that NFTA_RULE_POSITION takes precedence over
NFTA_RULE_POSITION_ID, so if the former is present, the latter is
ignored.
Signed-off-by: Phil Sutter
---
include/uapi/linux/netfilter/nf_tables.h | 2 ++
net/netfilter/nf_tables_api.c| 9 +
2 files changed, 11 insertions(+)
diff --git a/include/uapi/linux/netfilter/nf_tables.h
b/include/uapi/linux/netfilter/nf_tables.h
index b7dcce68e190f..8ff1f6b24de13 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -219,6 +219,7 @@ enum nft_chain_attributes {
* @NFTA_RULE_POSITION: numeric handle of the previous rule (NLA_U64)
* @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
* @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
+ * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule
(NLA_U32)
*/
enum nft_rule_attributes {
NFTA_RULE_UNSPEC,
@@ -231,6 +232,7 @@ enum nft_rule_attributes {
NFTA_RULE_USERDATA,
NFTA_RULE_PAD,
NFTA_RULE_ID,
+ NFTA_RULE_POSITION_ID,
__NFTA_RULE_MAX
};
#define NFTA_RULE_MAX (__NFTA_RULE_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d6be77618d556..9b7abc1f2ef70 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2565,6 +2565,9 @@ static int nft_table_validate(struct net *net, const
struct nft_table *table)
return 0;
}
+static struct nft_rule *nft_rule_lookup_byid(const struct net *net,
+const struct nlattr *nla);
+
#define NFT_RULE_MAXEXPRS 128
static int nf_tables_newrule(struct net *net, struct sock *nlsk,
@@ -2634,6 +2637,12 @@ static int nf_tables_newrule(struct net *net, struct
sock *nlsk,
NL_SET_BAD_ATTR(extack,
nla[NFTA_RULE_POSITION]);
return PTR_ERR(old_rule);
}
+ } else if (nla[NFTA_RULE_POSITION_ID]) {
+ old_rule = nft_rule_lookup_byid(net,
nla[NFTA_RULE_POSITION_ID]);
+ if (IS_ERR(old_rule)) {
+ NL_SET_BAD_ATTR(extack,
nla[NFTA_RULE_POSITION_ID]);
+ return PTR_ERR(old_rule);
+ }
}
}
--
2.20.1
[libnftnl PATCH] src: rule: Support NFTA_RULE_POSITION_ID attribute
Signed-off-by: Phil Sutter
---
Note: This obviously requires related kernel patch sent earlier
(netfilter: nf_tables: Support RULE_ID reference in new rule).
---
include/libnftnl/rule.h | 1 +
include/linux/netfilter/nf_tables.h | 2 ++
include/rule.h | 1 +
src/rule.c | 20
4 files changed, 24 insertions(+)
diff --git a/include/libnftnl/rule.h b/include/libnftnl/rule.h
index 8501c86e03613..78bfead132368 100644
--- a/include/libnftnl/rule.h
+++ b/include/libnftnl/rule.h
@@ -28,6 +28,7 @@ enum nftnl_rule_attr {
NFTNL_RULE_POSITION,
NFTNL_RULE_USERDATA,
NFTNL_RULE_ID,
+ NFTNL_RULE_POSITION_ID,
__NFTNL_RULE_MAX
};
#define NFTNL_RULE_MAX (__NFTNL_RULE_MAX - 1)
diff --git a/include/linux/netfilter/nf_tables.h
b/include/linux/netfilter/nf_tables.h
index 40c71c9f97a9c..91309ff971b03 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -218,6 +218,7 @@ enum nft_chain_attributes {
* @NFTA_RULE_POSITION: numeric handle of the previous rule (NLA_U64)
* @NFTA_RULE_USERDATA: user data (NLA_BINARY, NFT_USERDATA_MAXLEN)
* @NFTA_RULE_ID: uniquely identifies a rule in a transaction (NLA_U32)
+ * @NFTA_RULE_POSITION_ID: transaction unique identifier of the previous rule
(NLA_U32)
*/
enum nft_rule_attributes {
NFTA_RULE_UNSPEC,
@@ -230,6 +231,7 @@ enum nft_rule_attributes {
NFTA_RULE_USERDATA,
NFTA_RULE_PAD,
NFTA_RULE_ID,
+ NFTA_RULE_POSITION_ID,
__NFTA_RULE_MAX
};
#define NFTA_RULE_MAX (__NFTA_RULE_MAX - 1)
diff --git a/include/rule.h b/include/rule.h
index 5edcb6cfcd65c..036c7225d3ed0 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -11,6 +11,7 @@ struct nftnl_rule {
uint64_thandle;
uint64_tposition;
uint32_tid;
+ uint32_tposition_id;
struct {
void*data;
uint32_tlen;
diff --git a/src/rule.c b/src/rule.c
index e5d21ef9ff96a..8173fcdd863d9 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -87,6 +87,7 @@ void nftnl_rule_unset(struct nftnl_rule *r, uint16_t attr)
case NFTNL_RULE_POSITION:
case NFTNL_RULE_FAMILY:
case NFTNL_RULE_ID:
+ case NFTNL_RULE_POSITION_ID:
break;
case NFTNL_RULE_USERDATA:
xfree(r->user.data);
@@ -103,6 +104,7 @@ static uint32_t nftnl_rule_validate[NFTNL_RULE_MAX + 1] = {
[NFTNL_RULE_FAMILY] = sizeof(uint32_t),
[NFTNL_RULE_POSITION] = sizeof(uint64_t),
[NFTNL_RULE_ID] = sizeof(uint32_t),
+ [NFTNL_RULE_POSITION_ID]= sizeof(uint32_t),
};
EXPORT_SYMBOL(nftnl_rule_set_data);
@@ -158,6 +160,9 @@ int nftnl_rule_set_data(struct nftnl_rule *r, uint16_t attr,
case NFTNL_RULE_ID:
memcpy(&r->id, data, sizeof(r->id));
break;
+ case NFTNL_RULE_POSITION_ID:
+ memcpy(&r->position_id, data, sizeof(r->position_id));
+ break;
}
r->flags |= (1 << attr);
return 0;
@@ -222,6 +227,9 @@ const void *nftnl_rule_get_data(const struct nftnl_rule *r,
uint16_t attr,
case NFTNL_RULE_ID:
*data_len = sizeof(uint32_t);
return &r->id;
+ case NFTNL_RULE_POSITION_ID:
+ *data_len = sizeof(uint32_t);
+ return &r->position_id;
}
return NULL;
}
@@ -313,6 +321,8 @@ void nftnl_rule_nlmsg_build_payload(struct nlmsghdr *nlh,
struct nftnl_rule *r)
}
if (r->flags & (1 << NFTNL_RULE_ID))
mnl_attr_put_u32(nlh, NFTA_RULE_ID, htonl(r->id));
+ if (r->flags & (1 << NFTNL_RULE_POSITION_ID))
+ mnl_attr_put_u32(nlh, NFTA_RULE_POSITION_ID,
htonl(r->position_id));
}
EXPORT_SYMBOL(nftnl_rule_add_expr);
@@ -352,6 +362,7 @@ static int nftnl_rule_parse_attr_cb(const struct nlattr
*attr, void *data)
abi_breakage();
break;
case NFTA_RULE_ID:
+ case NFTA_RULE_POSITION_ID:
if (mnl_attr_validate(attr, MNL_TYPE_U32) < 0)
abi_breakage();
break;
@@ -483,6 +494,10 @@ int nftnl_rule_nlmsg_parse(const struct nlmsghdr *nlh,
struct nftnl_rule *r)
r->id = ntohl(mnl_attr_get_u32(tb[NFTA_RULE_ID]));
r->flags |= (1 << NFTNL_RULE_ID);
}
+ if (tb[NFTA_RULE_POSITION_ID]) {
+ r->position_id =
ntohl(mnl_attr_get_u32(tb[NFTA_RULE_POSITION_ID]));
+ r->flags |= (1 << NFTNL_RULE_POSITION_ID);
+ }
r->family = nfg->nfgen_family;
r->flags |= (1 << NFTNL_RULE_FAMILY);
@@ -566,6 +581,11 @@
Re: [iptables PATCH v4 5/5] xtables: Do not change ruleset while listing
Hi, On Fri, Jan 11, 2019 at 12:33:10AM +0100, Pablo Neira Ayuso wrote: > On Fri, Jan 11, 2019 at 12:16:33AM +0100, Florian Westphal wrote: > > Pablo Neira Ayuso wrote: > > > I think we need to add the built-in chains when listing if we want to > > > emulate the iptables-legacy behaviour. Listing via -L implies table > > > autoload, ie. > > > > > > # iptables-legacy -L -t raw > > > > > > pulls in the raw table and its chains. > > > > Yes, but I think thats a bug :-) > > OK, but that buggy behaviour has been there since the beginning IIRC :-) Hmm. Can you imagine a use-case where this would matter? I mean, behaviour would only divert as long as the user didn't add a rule yet. Is there a (user-relevant) functional difference between no table/chains and table with empty chains? The only use-case I could imagine is to check for given table type support by checking the list command's return code. > > I would prefer if iptables-nft would NOT do that, and instead > > list nothing, with exit value of 0. > > People should be using iptables-save instead for listing anyway, so I > don't mind if this is changed. We explicitly fixed for no output in list command on empty ruleset, so that is worth keeping IMO. Regarding the abort or avoid commit change, I don't have any good reasons for pushing it other than that it's not needed. So if you don't think it's a good idea or not worth the risk, no big deal for me. Cheers, Phil
[iptables PATCH 3/3] xtables: Fix for inserting rule at wrong position
iptables-restore allows to insert rules at a certain position which is
problematic for iptables-nft to realize since rule position is not
determined by number but handle of previous or following rule and in
case the rules surrounding the new one are new as well, they don't have
a handle to refer to yet.
Fix this by making use of NFTNL_RULE_POSITION_ID attribute: When
inserting before a rule which does not have a handle, refer to it using
its NFTNL_RULE_ID value. If the latter doesn't exist either, assign a
new one to it.
The last used rule ID value is tracked in a new field of struct
nft_handle which is incremented before each use.
Signed-off-by: Phil Sutter
---
iptables/nft.c| 30 +++--
iptables/nft.h| 1 +
.../ipt-restore/0003-restore-ordering_0 | 117 ++
.../testcases/iptables/0005-rule-replace_0| 38 ++
4 files changed, 176 insertions(+), 10 deletions(-)
create mode 100755
iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
create mode 100755 iptables/tests/shell/testcases/iptables/0005-rule-replace_0
diff --git a/iptables/nft.c b/iptables/nft.c
index ee3d142b25247..83d373c95ce9e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2063,16 +2063,30 @@ int nft_rule_delete(struct nft_handle *h, const char
*chain,
static struct nftnl_rule *
nft_rule_add(struct nft_handle *h, const char *chain,
const char *table, struct iptables_command_state *cs,
-uint64_t handle, bool verbose)
+struct nftnl_rule *ref, bool verbose)
{
struct nftnl_rule *r;
+ uint64_t ref_id;
r = nft_rule_new(h, chain, table, cs);
if (r == NULL)
return NULL;
- if (handle > 0)
- nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, handle);
+ if (ref) {
+ ref_id = nftnl_rule_get_u64(ref, NFTNL_RULE_HANDLE);
+ if (ref_id > 0) {
+ nftnl_rule_set_u64(r, NFTNL_RULE_POSITION, ref_id);
+ DEBUGP("adding after rule handle %"PRIu64"\n", ref_id);
+ } else {
+ ref_id = nftnl_rule_get_u32(ref, NFTNL_RULE_ID);
+ if (!ref_id) {
+ ref_id = ++h->rule_id;
+ nftnl_rule_set_u32(ref, NFTNL_RULE_ID, ref_id);
+ }
+ nftnl_rule_set_u32(r, NFTNL_RULE_POSITION_ID, ref_id);
+ DEBUGP("adding after rule ID %"PRIu64"\n", ref_id);
+ }
+ }
if (batch_rule_add(h, NFT_COMPAT_RULE_INSERT, r) < 0) {
nftnl_rule_free(r);
@@ -2088,9 +2102,8 @@ nft_rule_add(struct nft_handle *h, const char *chain,
int nft_rule_insert(struct nft_handle *h, const char *chain,
const char *table, void *data, int rulenum, bool verbose)
{
- struct nftnl_rule *r, *new_rule;
+ struct nftnl_rule *r = NULL, *new_rule;
struct nftnl_chain *c;
- uint64_t handle = 0;
/* If built-in chains don't exist for this table, create them */
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2118,16 +2131,13 @@ int nft_rule_insert(struct nft_handle *h, const char
*chain,
errno = ENOENT;
goto err;
}
-
- handle = nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE);
- DEBUGP("adding after rule handle %"PRIu64"\n", handle);
}
- new_rule = nft_rule_add(h, chain, table, data, handle, verbose);
+ new_rule = nft_rule_add(h, chain, table, data, r, verbose);
if (!new_rule)
goto err;
- if (handle)
+ if (r)
nftnl_chain_rule_insert_at(new_rule, r);
else
nftnl_chain_rule_add(new_rule, c);
diff --git a/iptables/nft.h b/iptables/nft.h
index 97d73c8b534be..0726923a63dd4 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -32,6 +32,7 @@ struct nft_handle {
struct mnl_socket *nl;
uint32_tportid;
uint32_tseq;
+ uint32_trule_id;
struct list_headobj_list;
int obj_list_num;
struct nftnl_batch *batch;
diff --git a/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
new file mode 100755
index 0..51f2422e15259
--- /dev/null
+++ b/iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0
@@ -0,0 +1,117 @@
+#!/bin/bash
+
+# Make sure iptables-restore does the right thing
+# when encountering INSERT rules with index.
+
+set -e
+
+# show rules, drop uninteresting policy settings
+ipt_show() {
+ $XT_MULTI iptables -S | grep -v '^-P'
+}
+
+# basic issue reproducer
+
+$XT_MULTI iptables-restore <
[iptables PATCH 1/3] nft: Add new builtin chains to cache immediately
Newly created builtin chains missing from cache was the sole reason for
the immediate calls to nft_commit(). With nft_chain_builtin_add()
inserting the new chain into the table's chain list, this is not needed
anymore. Just make sure batch_obj_del() doesn't free the payload of
NFT_COMPAT_CHAIN_ADD jobs since it contains the new chain which has
been added to cache.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 30 +-
1 file changed, 9 insertions(+), 21 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index 1ce1ecdd276be..73a99e5d8813e 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -644,6 +644,7 @@ static void nft_chain_builtin_add(struct nft_handle *h,
return;
batch_chain_add(h, NFT_COMPAT_CHAIN_ADD, c);
+ nftnl_chain_list_add_tail(c, h->table[table->type].chain_cache);
}
/* find if built-in table already exists */
@@ -1216,8 +1217,11 @@ nft_rule_append(struct nft_handle *h, const char *chain,
const char *table,
h->ops->print_rule(r, 0, FMT_PRINT_RULE);
c = nft_chain_find(h, table, chain);
- if (c)
- nftnl_chain_rule_add_tail(r, c);
+ if (!c) {
+ errno = ENOENT;
+ return 0;
+ }
+ nftnl_chain_rule_add_tail(r, c);
return 1;
}
@@ -2280,16 +2284,8 @@ int nft_rule_list(struct nft_handle *h, const char
*chain, const char *table,
bool found = false;
/* If built-in chains don't exist for this table, create them */
- if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0) {
+ if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
nft_xt_builtin_init(h, table);
- /* Force table and chain creation, otherwise first iptables -L
-* lists no table/chains.
-*/
- if (!list_empty(&h->obj_list)) {
- nft_commit(h);
- flush_chain_cache(h, NULL);
- }
- }
ops = nft_family_ops_lookup(h->family);
@@ -2395,16 +2391,8 @@ int nft_rule_list_save(struct nft_handle *h, const char
*chain,
int ret = 0;
/* If built-in chains don't exist for this table, create them */
- if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0) {
+ if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
nft_xt_builtin_init(h, table);
- /* Force table and chain creation, otherwise first iptables -L
-* lists no table/chains.
-*/
- if (!list_empty(&h->obj_list)) {
- nft_commit(h);
- flush_chain_cache(h, NULL);
- }
- }
if (!nft_is_table_compatible(h, table)) {
xtables_error(OTHER_PROBLEM, "table `%s' is incompatible, use
'nft' tool.\n", table);
@@ -2523,8 +2511,8 @@ static void batch_obj_del(struct nft_handle *h, struct
obj_update *o)
break;
case NFT_COMPAT_CHAIN_ZERO:
case NFT_COMPAT_CHAIN_USER_ADD:
- break;
case NFT_COMPAT_CHAIN_ADD:
+ break;
case NFT_COMPAT_CHAIN_USER_DEL:
case NFT_COMPAT_CHAIN_USER_FLUSH:
case NFT_COMPAT_CHAIN_UPDATE:
--
2.20.1
[iptables PATCH 0/3] xtables: Fix for inserting rule at wrong position
This series contains an improved approach at fixing the rule ordering issue when using rule insert (-I) commands in iptables-nft-restore input. This much less intrusive implementation leverages the pending support for NFTA_RULE_POSITION_ID to link new rules to other new ones in the same batch. A requirement for this to work is that all new rules are added to cache immediately, which in turn requires that newly added builtin chains are put in cache as well. This is achieved by patch 1. Patch 2 fixes the position of replaced rules in cache. It is possible to combine rule replace (-R) commands with rule insert (-I) ones in the same dump file, so the replaced ones need to be put in correct position in case a later rule should be inserted before them. Patch 3 finally adds the relevant code to reference a new rule from another new one. Phil Sutter (3): nft: Add new builtin chains to cache immediately xtables: Fix position of replaced rules in cache xtables: Fix for inserting rule at wrong position iptables/nft.c| 84 ++--- iptables/nft.h| 3 +- .../ipt-restore/0003-restore-ordering_0 | 117 ++ .../testcases/iptables/0005-rule-replace_0| 38 ++ iptables/xtables-arp.c| 2 +- iptables/xtables-eb.c | 2 +- iptables/xtables.c| 4 +- 7 files changed, 202 insertions(+), 48 deletions(-) create mode 100755 iptables/tests/shell/testcases/ipt-restore/0003-restore-ordering_0 create mode 100755 iptables/tests/shell/testcases/iptables/0005-rule-replace_0 -- 2.20.1
[iptables PATCH 2/3] xtables: Fix position of replaced rules in cache
When replacing a rule, the replacement was simply appended to the
chain's rule list. Instead, insert it where the rule it replaces was.
This also fixes for zero counters command to remove the old rule from
cache.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 34 +-
iptables/nft.h | 2 +-
iptables/xtables-arp.c | 2 +-
iptables/xtables-eb.c | 2 +-
iptables/xtables.c | 4 ++--
5 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index 73a99e5d8813e..ee3d142b25247 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1186,7 +1186,7 @@ nft_chain_find(struct nft_handle *h, const char *table,
const char *chain);
int
nft_rule_append(struct nft_handle *h, const char *chain, const char *table,
- void *data, uint64_t handle, bool verbose)
+ void *data, struct nftnl_rule *ref, bool verbose)
{
struct nftnl_chain *c;
struct nftnl_rule *r;
@@ -1202,8 +1202,9 @@ nft_rule_append(struct nft_handle *h, const char *chain,
const char *table,
if (r == NULL)
return 0;
- if (handle > 0) {
- nftnl_rule_set(r, NFTNL_RULE_HANDLE, &handle);
+ if (ref) {
+ nftnl_rule_set_u64(r, NFTNL_RULE_HANDLE,
+ nftnl_rule_get_u64(ref, NFTNL_RULE_HANDLE));
type = NFT_COMPAT_RULE_REPLACE;
} else
type = NFT_COMPAT_RULE_APPEND;
@@ -1216,12 +1217,17 @@ nft_rule_append(struct nft_handle *h, const char
*chain, const char *table,
if (verbose)
h->ops->print_rule(r, 0, FMT_PRINT_RULE);
- c = nft_chain_find(h, table, chain);
- if (!c) {
- errno = ENOENT;
- return 0;
+ if (ref) {
+ nftnl_chain_rule_insert_at(r, ref);
+ nftnl_chain_rule_del(r);
+ } else {
+ c = nft_chain_find(h, table, chain);
+ if (!c) {
+ errno = ENOENT;
+ return 0;
+ }
+ nftnl_chain_rule_add_tail(r, c);
}
- nftnl_chain_rule_add_tail(r, c);
return 1;
}
@@ -2107,7 +2113,7 @@ int nft_rule_insert(struct nft_handle *h, const char
*chain,
r = nft_rule_find(h, c, data, rulenum - 1);
if (r != NULL)
return nft_rule_append(h, chain, table, data,
- 0, verbose);
+ NULL, verbose);
errno = ENOENT;
goto err;
@@ -2179,11 +2185,7 @@ int nft_rule_replace(struct nft_handle *h, const char
*chain,
(unsigned long long)
nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE));
- nftnl_rule_list_del(r);
-
- ret = nft_rule_append(h, chain, table, data,
- nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE),
- verbose);
+ ret = nft_rule_append(h, chain, table, data, r, verbose);
} else
errno = ENOENT;
@@ -2459,9 +2461,7 @@ int nft_rule_zero_counters(struct nft_handle *h, const
char *chain,
cs.counters.pcnt = cs.counters.bcnt = 0;
- ret = nft_rule_append(h, chain, table, &cs,
- nftnl_rule_get_u64(r, NFTNL_RULE_HANDLE),
- false);
+ ret = nft_rule_append(h, chain, table, &cs, r, false);
error:
return ret;
diff --git a/iptables/nft.h b/iptables/nft.h
index dfdffd69342db..97d73c8b534be 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -98,7 +98,7 @@ bool nft_chain_exists(struct nft_handle *h, const char
*table, const char *chain
*/
struct nftnl_rule;
-int nft_rule_append(struct nft_handle *h, const char *chain, const char
*table, void *data, uint64_t handle, bool verbose);
+int nft_rule_append(struct nft_handle *h, const char *chain, const char
*table, void *data, struct nftnl_rule *ref, bool verbose);
int nft_rule_insert(struct nft_handle *h, const char *chain, const char
*table, void *data, int rulenum, bool verbose);
int nft_rule_check(struct nft_handle *h, const char *chain, const char *table,
void *data, bool verbose);
int nft_rule_delete(struct nft_handle *h, const char *chain, const char
*table, void *data, bool verbose);
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 2f369d9aadb01..57e717fa901a1 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -826,7 +826,7 @@ append_entry(struct nft_handle *h,
for (j = 0; j < ndaddrs; j++) {
cs->arp.arp.tgt.s_addr = daddrs[j].s_addr;
if (append) {
- ret = nft_ru
[nft PATCH] src: Quote user-defined names
Nftables claims to allow arbitrary names for ruleset elements (tables,
chains, objects) but suffers from the known problem of lex/yacc trying
to interpret those as keywords. As a workaround, users may quote their
names. Sadly this wasn't supported in most cases and this patch lifts
this restriction.
In order to not print rulesets which are not accepted anymore by 'nft
-f' command, unconditionally quote all names on output.
Note that the same problem existed for interface names. I've tested for
those to work in both netdev family chains and flowtable definitions,
though automatic testing is troublesome since they must exist (and I'm
not sure if test scripts should call iproute2 to add an interface with a
crafted name).
Signed-off-by: Phil Sutter
---
src/parser_bison.y| 3 +-
src/rule.c| 28 +--
.../shell/testcases/nft-f/0018quoted-names_0 | 19 +
3 files changed, 35 insertions(+), 15 deletions(-)
create mode 100755 tests/shell/testcases/nft-f/0018quoted-names_0
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 02a373cb2289a..fb9b0fcf89baf 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -1782,7 +1782,7 @@ flowtable_list_expr : flowtable_expr_member
| flowtable_list_expr COMMA opt_newline
;
-flowtable_expr_member : STRING
+flowtable_expr_member : string
{
$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
current_scope(state),
@@ -1989,6 +1989,7 @@ chain_policy : ACCEPT { $$ =
NF_ACCEPT; }
;
identifier : STRING
+ | QUOTED_STRING
;
string : STRING
diff --git a/src/rule.c b/src/rule.c
index 73b78c75a267a..f845b5d097c3a 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -433,9 +433,9 @@ static void set_print_declaration(const struct set *set,
nft_print(octx, " %s", opts->family);
if (opts->table != NULL)
- nft_print(octx, " %s", opts->table);
+ nft_print(octx, " \"%s\"", opts->table);
- nft_print(octx, " %s {", set->handle.set.name);
+ nft_print(octx, " \"%s\" {", set->handle.set.name);
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64, set->handle.handle.id);
@@ -1062,7 +1062,7 @@ static void chain_print_declaration(const struct chain
*chain,
{
char priobuf[STD_PRIO_BUFSIZE];
- nft_print(octx, "\tchain %s {", chain->handle.chain.name);
+ nft_print(octx, "\tchain \"%s\" {", chain->handle.chain.name);
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64, chain->handle.handle.id);
nft_print(octx, "\n");
@@ -1222,7 +1222,7 @@ static void table_print(const struct table *table, struct
output_ctx *octx)
const char *delim = "";
const char *family = family2str(table->handle.family);
- nft_print(octx, "table %s %s {", family, table->handle.table.name);
+ nft_print(octx, "table %s \"%s\" {", family, table->handle.table.name);
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64, table->handle.handle.id);
nft_print(octx, "\n");
@@ -1748,7 +1748,7 @@ static void obj_print_data(const struct obj *obj,
{
switch (obj->type) {
case NFT_OBJECT_COUNTER:
- nft_print(octx, " %s {", obj->handle.obj.name);
+ nft_print(octx, " \"%s\" {", obj->handle.obj.name);
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64,
obj->handle.handle.id);
nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab);
@@ -1763,7 +1763,7 @@ static void obj_print_data(const struct obj *obj,
const char *data_unit;
uint64_t bytes;
- nft_print(octx, " %s {", obj->handle.obj.name);
+ nft_print(octx, " \"%s\" {", obj->handle.obj.name);
if (nft_output_handle(octx))
nft_print(octx, " # handle %" PRIu64,
obj->handle.handle.id);
nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab);
@@ -1780,14 +1780,14 @@ static void obj_print_data(const struct obj *obj,
}
break;
case NFT_OBJECT_SECMARK:
- n
[iptables PATCH] utils: Add a manpage for nfbpf_compile
Content is rather sparse, but still better than no manpage at all.
Cc: Willem de Bruijn
Signed-off-by: Phil Sutter
---
configure.ac | 3 +-
utils/.gitignore | 1 +
utils/Makefile.am| 3 +-
utils/nfbpf_compile.8.in | 70
4 files changed, 75 insertions(+), 2 deletions(-)
create mode 100644 utils/nfbpf_compile.8.in
diff --git a/configure.ac b/configure.ac
index 448ec918fd89b..e6c9832fa43ba 100644
--- a/configure.ac
+++ b/configure.ac
@@ -252,7 +252,8 @@ AC_CONFIG_FILES([Makefile extensions/GNUmakefile
include/Makefile
libxtables/Makefile utils/Makefile
include/xtables-version.h include/iptables/internal.h
iptables/xtables-monitor.8
- utils/nfnl_osf.8])
+ utils/nfnl_osf.8
+ utils/nfbpf_compile.8])
AC_OUTPUT
diff --git a/utils/.gitignore b/utils/.gitignore
index 7c6afbf4e6a52..6300812b1701b 100644
--- a/utils/.gitignore
+++ b/utils/.gitignore
@@ -1,3 +1,4 @@
/nfnl_osf
/nfnl_osf.8
/nfbpf_compile
+/nfbpf_compile.8
diff --git a/utils/Makefile.am b/utils/Makefile.am
index 80029e303ff3b..d09a69749b85f 100644
--- a/utils/Makefile.am
+++ b/utils/Makefile.am
@@ -17,6 +17,7 @@ nfnl_osf_LDADD = ${libnfnetlink_LIBS}
endif
if ENABLE_BPFC
+man_MANS += nfbpf_compile.8
sbin_PROGRAMS += nfbpf_compile
nfbpf_compile_LDADD = -lpcap
endif
@@ -26,4 +27,4 @@ sbin_PROGRAMS += nfsynproxy
nfsynproxy_LDADD = -lpcap
endif
-CLEANFILES = nfnl_osf.8
+CLEANFILES = nfnl_osf.8 nfbpf_compile.8
diff --git a/utils/nfbpf_compile.8.in b/utils/nfbpf_compile.8.in
new file mode 100644
index 0..d02979a5143ef
--- /dev/null
+++ b/utils/nfbpf_compile.8.in
@@ -0,0 +1,70 @@
+.TH NFBPF_COMPILE 8 "" "@PACKAGE_STRING@" "@PACKAGE_STRING@"
+
+.SH NAME
+nfbpf_compile \- generate bytecode for use with xt_bpf
+.SH SYNOPSIS
+
+.ad l
+.in +8
+.ti -8
+.B nfbpf_compile
+[
+.I LLTYPE
+]
+.I PROGRAM
+
+.ti -8
+.I LLTYPE
+:= {
+.BR EN10MB " | " RAW " | " SLIP " | "
+.I ...
+}
+
+.SH DESCRIPTION
+The
+.B nfbpf_compile
+utility aids in generating BPF byte code suitable for passing to
+the iptables
+.B bpf
+match.
+
+.SH OPTIONS
+
+.TP
+.I LLTYPE
+Link-layer header type to operate on. This is a name as defined in
+.RB < pcap/dlt.h >
+but with the leading
+.B DLT_
+prefix stripped. For use with iptables,
+.B RAW
+should be the right choice (it's also the default if not specified).
+
+.TP
+.I PROGRAM
+The BPF expression to compile, see
+.BR pcap-filter (7)
+for a description of the language.
+
+.SH EXIT STATUS
+The program returns 0 on success, 1 otherwise.
+
+.SH EXAMPLE
+Match incoming TCP packets with size bigger than 100 bytes:
+.P
+.in +8
+.EE
+bpf=$(nfbpf_compile 'tcp and greater 100')
+.br
+iptables -A INPUT -m bpf --bytecode "$bpf" -j ACCEPT
+.RE
+.P
+The description of
+.B bpf
+match in
+.BR iptables-extensions (8)
+lists a few more examples.
+
+.SH SEE ALSO
+.BR iptables-extensions (8),
+.BR pcap-filter (7)
--
2.20.1
Re: [nft PATCH] src: Quote user-defined names
On Wed, Jan 16, 2019 at 08:19:00PM +0100, Pablo Neira Ayuso wrote: > Hi Phil, > > On Wed, Jan 16, 2019 at 07:46:13PM +0100, Phil Sutter wrote: > > Nftables claims to allow arbitrary names for ruleset elements (tables, > > chains, objects) but suffers from the known problem of lex/yacc trying > > to interpret those as keywords. As a workaround, users may quote their > > names. Sadly this wasn't supported in most cases and this patch lifts > > this restriction. > > > > In order to not print rulesets which are not accepted anymore by 'nft > > -f' command, unconditionally quote all names on output. > > > > Note that the same problem existed for interface names. I've tested for > > those to work in both netdev family chains and flowtable definitions, > > though automatic testing is troublesome since they must exist (and I'm > > not sure if test scripts should call iproute2 to add an interface with a > > crafted name). > > This is what we are supporting natively, probably not well documented: > > commit 57ecffc9d1e551ecc0546806ca9c008e93c2ecf3 > Author: Pablo Neira Ayuso > Date: Tue Aug 16 23:22:51 2016 +0200 > > scanner: allow strings starting by underscores and dots > > POSIX.1-2008 (which is simultaneously IEEE Std 1003.1-2008) says: > "The set of characters from which portable filenames are constructed. > > A B C D E F G H I J K L M N O P Q R S T U V W X Y Z > a b c d e f g h i j k l m n o p q r s t u v w x y z > 0 1 2 3 4 5 6 7 8 9 . _ -" > > I think we can just document this or you need this sort of > flexibility. We can also allow for keywords to be used as names, which > is what is left behind... > > We can of course decide to go for quotes as you propose, this was so > far the only exception since all other user-defined values from rules > are always assumed to enclosed in quotes. My point is that users don't necessarily know what names are forbidden (i.e., keywords) and which are not. Suggesting to prefix names by underscore "just in case" or "if you get a weird error message" is not the best option in my opinion. That said, my solution of "quote names unless you know what you're doing" is not much better to be fair. :) I really don't know what's the best way to handle this, given that we can't work around this quirk in lex/yacc without turning all into a mess. The problem I'm facing is simply that users file tickets because they are not aware of the problem and hence think that nft not accepting certain names is simply a bug one should fix. Cheers, Phil
Re: [iptables PATCH] utils: Add a manpage for nfbpf_compile
Hi Willem, On Thu, Jan 17, 2019 at 11:07:18AM -0500, Willem de Bruijn wrote: > On Wed, Jan 16, 2019 at 4:48 PM Phil Sutter wrote: > > > > Content is rather sparse, but still better than no manpage at all. > > > > Cc: Willem de Bruijn > > Signed-off-by: Phil Sutter > > Thanks for adding this, Phil. > > Content looks great to me. One inline question. Thanks for your review! > > diff --git a/utils/nfbpf_compile.8.in b/utils/nfbpf_compile.8.in > > new file mode 100644 > > index 0..d02979a5143ef > > --- /dev/null > > +++ b/utils/nfbpf_compile.8.in > > @@ -0,0 +1,70 @@ > > +.TH NFBPF_COMPILE 8 "" "@PACKAGE_STRING@" "@PACKAGE_STRING@" > > Are these @PACKAGE_STRING@ tokens placeholders? Yes, they are. Also as indicated by the filename, configure script will fill them in while removing the suffix from filename. This is analogous to how nfnl_osf manpage is integrated into iptables sources. Cheers, Phil
Re: [iptables PATCH 1/3] nft: Add new builtin chains to cache immediately
Hi Pablo,
On Fri, Jan 18, 2019 at 02:46:44AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Jan 15, 2019 at 11:23:03PM +0100, Phil Sutter wrote:
[...]
> > @@ -1216,8 +1217,11 @@ nft_rule_append(struct nft_handle *h, const char
> > *chain, const char *table,
> > h->ops->print_rule(r, 0, FMT_PRINT_RULE);
> >
> > c = nft_chain_find(h, table, chain);
> > - if (c)
> > - nftnl_chain_rule_add_tail(r, c);
> > + if (!c) {
> > + errno = ENOENT;
> > + return 0;
> > + }
> > + nftnl_chain_rule_add_tail(r, c);
>
> Next time, please don't add unrelated changes, this renders 'git
> annotate' useless, thanks.
Oh, you're right. This chunk is merely related to the first one. I'll
spend more attention to those things in future.
Sorry, Phil
[iptables PATCH 1/2] ebtables: Fix rule listing with counters
This is a partial revert of commit 583b27eabcad6 ("ebtables-save: add -c
option, using xtables-style counters") which broke ruleset listing with
'--Lc' flag turned on:
| # ebtables-nft -L --Lc
| Bridge table: filter
|
| Bridge chain: INPUT, entries: 0, policy: ACCEPT
|
| Bridge chain: FORWARD, entries: 2, policy: ACCEPT
| -j foo
| , pcnt = 0 -- bcnt = 0-j ACCEPT
| , pcnt = 0 -- bcnt = 0
| Bridge chain: OUTPUT, entries: 0, policy: ACCEPT
|
| Bridge chain: foo, entries: 1, policy: RETURN
| -j ACCEPT
| , pcnt = 0 -- bcnt = 0%
(That percentage sign means no newline after last line of output and
doesn't belong to ebtables-nft's output.)
Problem was that nft_bridge_print_rule() printed the counters after
nft_bridge_save_rule() had already printed the newline character.
Note also that there is no need to remove FMT_EBT_SAVE bit from 'format'
variable: It is set only by ebtables-nft-save which doesn't call
nft_bridge_print_rule().
Fixes: 583b27eabcad6 ("ebtables-save: add -c option, using xtables-style
counters")
Signed-off-by: Phil Sutter
---
iptables/nft-bridge.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index ad583a60c424d..61c82f72c7740 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -469,6 +469,11 @@ static void nft_bridge_save_rule(const void *data,
unsigned int format)
(uint64_t)cs->counters.pcnt,
(uint64_t)cs->counters.bcnt);
+ if (!(format & FMT_NOCOUNTS))
+ printf(" , pcnt = %"PRIu64" -- bcnt = %"PRIu64"",
+ (uint64_t)cs->counters.pcnt,
+ (uint64_t)cs->counters.bcnt);
+
if (!(format & FMT_NONEWLINE))
fputc('\n', stdout);
}
@@ -482,11 +487,7 @@ static void nft_bridge_print_rule(struct nftnl_rule *r,
unsigned int num,
printf("%d ", num);
nft_rule_to_ebtables_command_state(r, &cs);
- nft_bridge_save_rule(&cs, format & ~FMT_EBT_SAVE);
- if (!(format & FMT_NOCOUNTS))
- printf(" , pcnt = %"PRIu64" -- bcnt = %"PRIu64"",
- (uint64_t)cs.counters.pcnt,
- (uint64_t)cs.counters.bcnt);
+ nft_bridge_save_rule(&cs, format);
ebt_cs_clean(&cs);
}
--
2.20.1
[iptables PATCH 0/2] ebtables-nft output fixes
The first patch fixes a clear bug introduced when adding support for xtables-style counters to ebtables-nft-save. The second patch is a bit more controversial, as it undoes an intentional change for the sake of output compatibility with legacy ebtables. Phil Sutter (2): ebtables: Fix rule listing with counters Revert "ebtables: use extrapositioned negation consistently" extensions/libebt_802_3.c| 4 ++-- extensions/libebt_802_3.t| 2 +- extensions/libebt_arp.c | 14 +++--- extensions/libebt_arp.t | 9 - extensions/libebt_ip.c | 16 extensions/libebt_ip.t | 8 +++- extensions/libebt_ip6.c | 14 +++--- extensions/libebt_ip6.t | 8 +++- extensions/libebt_mark_m.c | 2 +- extensions/libebt_mark_m.t | 4 ++-- extensions/libebt_pkttype.c | 5 + extensions/libebt_pkttype.t | 8 ++-- extensions/libebt_standard.t | 4 extensions/libebt_stp.c | 5 ++--- extensions/libebt_vlan.c | 13 - extensions/libebt_vlan.t | 12 ++-- iptables/nft-bridge.c| 17 + 17 files changed, 58 insertions(+), 87 deletions(-) -- 2.20.1
[iptables PATCH 2/2] Revert "ebtables: use extrapositioned negation consistently"
This reverts commit 5f508b76a0cebaf91965ffa678089222e2d47964.
While attempts at unifying syntax between arp-, eb- and iptables-nft
increase the opportunity for more code-sharing, they are problematic
when it comes to compatibility. Accepting the old syntax on input helps,
but due to the fact that neither arptables nor ebtables support --check
command we must expect for users to test existence of a rule by
comparing input with output. If that happens in a script, deviating from
the old syntax in output has a high chance of breaking it.
Therefore revert Florian's patch changing inversion character position
in output and review the old code for consistency - the only thing
changed on top of the actual revert is ebtables' own copy of
print_iface() to make it adhere to the intrapositioned negation scheme
used throughout ebtables.
Signed-off-by: Phil Sutter
---
extensions/libebt_802_3.c| 4 ++--
extensions/libebt_802_3.t| 2 +-
extensions/libebt_arp.c | 14 +++---
extensions/libebt_arp.t | 9 -
extensions/libebt_ip.c | 16
extensions/libebt_ip.t | 8 +++-
extensions/libebt_ip6.c | 14 +++---
extensions/libebt_ip6.t | 8 +++-
extensions/libebt_mark_m.c | 2 +-
extensions/libebt_mark_m.t | 4 ++--
extensions/libebt_pkttype.c | 5 +
extensions/libebt_pkttype.t | 8 ++--
extensions/libebt_standard.t | 4
extensions/libebt_stp.c | 5 ++---
extensions/libebt_vlan.c | 13 -
extensions/libebt_vlan.t | 12 ++--
iptables/nft-bridge.c| 6 +++---
17 files changed, 52 insertions(+), 82 deletions(-)
diff --git a/extensions/libebt_802_3.c b/extensions/libebt_802_3.c
index 9e91d05262591..f05d02ead5a4a 100644
--- a/extensions/libebt_802_3.c
+++ b/extensions/libebt_802_3.c
@@ -98,15 +98,15 @@ static void br802_3_print(const void *ip, const struct
xt_entry_match *match,
struct ebt_802_3_info *info = (struct ebt_802_3_info *)match->data;
if (info->bitmask & EBT_802_3_SAP) {
+ printf("--802_3-sap ");
if (info->invflags & EBT_802_3_SAP)
printf("! ");
- printf("--802_3-sap ");
printf("0x%.2x ", info->sap);
}
if (info->bitmask & EBT_802_3_TYPE) {
+ printf("--802_3-type ");
if (info->invflags & EBT_802_3_TYPE)
printf("! ");
- printf("--802_3-type ");
printf("0x%.4x ", ntohs(info->type));
}
}
diff --git a/extensions/libebt_802_3.t b/extensions/libebt_802_3.t
index 61081bd6983a8..ddfb2f0a72baf 100644
--- a/extensions/libebt_802_3.t
+++ b/extensions/libebt_802_3.t
@@ -1,3 +1,3 @@
:INPUT,FORWARD,OUTPUT
-! --802_3-sap 0x0a -j CONTINUE;=;OK
+--802_3-sap ! 0x0a -j CONTINUE;=;OK
--802_3-type 0x000a -j RETURN;=;OK
diff --git a/extensions/libebt_arp.c b/extensions/libebt_arp.c
index c1b0ab1db0cf1..a062b7e7e5864 100644
--- a/extensions/libebt_arp.c
+++ b/extensions/libebt_arp.c
@@ -338,51 +338,51 @@ static void brarp_print(const void *ip, const struct
xt_entry_match *match, int
if (arpinfo->bitmask & EBT_ARP_OPCODE) {
int opcode = ntohs(arpinfo->opcode);
+ printf("--arp-op ");
if (arpinfo->invflags & EBT_ARP_OPCODE)
printf("! ");
- printf("--arp-op ");
if (opcode > 0 && opcode <= ARRAY_SIZE(opcodes))
printf("%s ", opcodes[opcode - 1]);
else
printf("%d ", opcode);
}
if (arpinfo->bitmask & EBT_ARP_HTYPE) {
+ printf("--arp-htype ");
if (arpinfo->invflags & EBT_ARP_HTYPE)
printf("! ");
- printf("--arp-htype ");
printf("%d ", ntohs(arpinfo->htype));
}
if (arpinfo->bitmask & EBT_ARP_PTYPE) {
+ printf("--arp-ptype ");
if (arpinfo->invflags & EBT_ARP_PTYPE)
printf("! ");
- printf("--arp-ptype ");
printf("0x%x ", ntohs(arpinfo->ptype));
}
if (arpinfo->bitmask & EBT_ARP_SRC_IP) {
+ printf("--arp-ip-src ");
if (arpinfo->invflags & EBT_ARP_SRC_IP)
printf("! ");
- printf("--arp-ip-src ");
printf("%s%s ", xtables_ipaddr_to_numeric((const struct
in_addr*) &arpinfo->saddr),
xtables_ipmask_to_
Re: [iptables PATCH 2/2] Revert "ebtables: use extrapositioned negation consistently"
On Mon, Jan 21, 2019 at 06:23:42PM +0100, Florian Westphal wrote: > Phil Sutter wrote: > > This reverts commit 5f508b76a0cebaf91965ffa678089222e2d47964. > > > > While attempts at unifying syntax between arp-, eb- and iptables-nft > > increase the opportunity for more code-sharing, they are problematic > > when it comes to compatibility. Accepting the old syntax on input helps, > > but due to the fact that neither arptables nor ebtables support --check > > command we must expect for users to test existence of a rule by > > comparing input with output. If that happens in a script, deviating from > > the old syntax in output has a high chance of breaking it. > > Is there a known script that is affected? I guess some CI test script is since that's where the ticket came from. ;) > We broke this in iptables in even worse way, as we even do not support > -i ! "foo" anymore (you get a syntax error). Well, the relevant difference here is that with iptables, you may use '-C' to check for your rule but have to parse regular list output with arptables/ebtables instead. So output format is a tad more important with those tools. > Do you think adding a warning on -i ! "foo" would help? Well, downstream we would rather make use of release notes to inform users I guess. > The many syntax deviations between the flavours is not nice at all, > making this more consistent would be a nice thing imo. The bright side here is that at least for now no shared code is affected. So we may stick with the quirky ebtables syntax without cost at this point. BTW: What about changing legacy ebtables code to align its syntax more with iptables one? I know that "thou shall not touch the legacy". Though deviating ebtables-nft from ebtables-legacy means users would have to adapt - although we seem to pretend they can't when it comes to changing legacy code. Don't get me wrong, I'm open for anything but appreciate if things are done consistently. Cheers, Phil
[iptables PATCH 1/3] nft: Fix potential memleaks in nft_*_rule_find()
These functions parse an nftnl_rule into a local instance of
iptables_command_state which potentially allocates memory (for matches
or target), so call ops->clear_cs() before returning to caller.
Signed-off-by: Phil Sutter
---
iptables/nft-arp.c| 12
iptables/nft-bridge.c | 14 +-
iptables/nft-shared.c | 14 +-
3 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 399f83afff5cc..5f311753b939d 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -646,20 +646,24 @@ static bool nft_arp_rule_find(struct nft_family_ops *ops,
struct nftnl_rule *r,
{
const struct iptables_command_state *cs = data;
struct iptables_command_state this = {};
+ bool ret = false;
/* Delete by matching rule case */
nft_rule_to_iptables_command_state(r, &this);
if (!nft_arp_is_same(&cs->arp, &this.arp))
- return false;
+ goto out;
if (!compare_targets(cs->target, this.target))
- return false;
+ goto out;
if (this.jumpto && strcmp(cs->jumpto, this.jumpto) != 0)
- return false;
+ goto out;
- return true;
+ ret = true;
+out:
+ ops->clear_cs(&this);
+ return ret;
}
static void nft_arp_save_chain(const struct nftnl_chain *c, const char *policy)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index ad583a60c424d..cbd671c38d2e1 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -547,30 +547,34 @@ static bool nft_bridge_rule_find(struct nft_family_ops
*ops, struct nftnl_rule *
{
struct iptables_command_state *cs = data;
struct iptables_command_state this = {};
+ bool ret = false;
nft_rule_to_ebtables_command_state(r, &this);
DEBUGP("comparing with... ");
if (!nft_bridge_is_same(cs, &this))
- return false;
+ goto out;
if (!compare_matches(cs->matches, this.matches)) {
DEBUGP("Different matches\n");
- return false;
+ goto out;
}
if (!compare_targets(cs->target, this.target)) {
DEBUGP("Different target\n");
- return false;
+ goto out;
}
if (cs->jumpto != NULL && strcmp(cs->jumpto, this.jumpto) != 0) {
DEBUGP("Different verdict\n");
- return false;
+ goto out;
}
- return true;
+ ret = true;
+out:
+ ops->clear_cs(&this);
+ return ret;
}
static int xlate_ebmatches(const struct iptables_command_state *cs, struct
xt_xlate *xl)
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 05b091ea2c0d5..ca4e593656562 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -961,6 +961,7 @@ bool nft_ipv46_rule_find(struct nft_family_ops *ops,
struct nftnl_rule *r, void *data)
{
struct iptables_command_state *cs = data, this = {};
+ bool ret = false;
nft_rule_to_iptables_command_state(r, &this);
@@ -969,24 +970,27 @@ bool nft_ipv46_rule_find(struct nft_family_ops *ops,
nft_rule_print_save(r, NFT_RULE_APPEND, 0);
#endif
if (!ops->is_same(cs, &this))
- return false;
+ goto out;
if (!compare_matches(cs->matches, this.matches)) {
DEBUGP("Different matches\n");
- return false;
+ goto out;
}
if (!compare_targets(cs->target, this.target)) {
DEBUGP("Different target\n");
- return false;
+ goto out;
}
if (strcmp(cs->jumpto, this.jumpto) != 0) {
DEBUGP("Different verdict\n");
- return false;
+ goto out;
}
- return true;
+ ret = true;
+out:
+ ops->clear_cs(&this);
+ return ret;
}
void nft_check_xt_legacy(int family, bool is_ipt_save)
--
2.20.1
[iptables PATCH 2/3] xtables: Fix for crash when comparing rules with standard target
When parsing an nftnl_rule with a standard verdict,
nft_rule_to_iptables_command_state() initialized cs->target but didn't
care about cs->target->t. When later comparing that rule to another,
compare_targets() crashed due to unconditional access to t's fields.
Signed-off-by: Phil Sutter
---
iptables/nft-shared.c | 19 ---
.../testcases/iptables/0005-delete-rules_0| 7 +++
2 files changed, 23 insertions(+), 3 deletions(-)
create mode 100755 iptables/tests/shell/testcases/iptables/0005-delete-rules_0
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index ca4e593656562..caed347ddb724 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -660,12 +660,25 @@ void nft_rule_to_iptables_command_state(const struct
nftnl_rule *r,
match->m = m;
}
- if (cs->target != NULL)
+ if (cs->target != NULL) {
cs->jumpto = cs->target->name;
- else if (cs->jumpto != NULL)
+ } else if (cs->jumpto != NULL) {
+ struct xt_entry_target *t;
+ uint32_t size;
+
cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
- else
+ if (!cs->target)
+ return;
+
+ size = XT_ALIGN(sizeof(struct xt_entry_target)) +
cs->target->size;
+ t = xtables_calloc(1, size);
+ t->u.target_size = size;
+ t->u.user.revision = cs->target->revision;
+ strcpy(t->u.user.name, cs->jumpto);
+ cs->target->t = t;
+ } else {
cs->jumpto = "";
+ }
}
void nft_clear_iptables_command_state(struct iptables_command_state *cs)
diff --git a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
new file mode 100755
index 0..9312fd53c3437
--- /dev/null
+++ b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
@@ -0,0 +1,7 @@
+#!/bin/bash
+
+# test for crash when comparing rules with standard target
+
+$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j DROP
+$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j REJECT
+[[ $? -eq 1 ]] || exit 1
--
2.20.1
[iptables PATCH 0/3] xtables: Fix multiple issues in rule matching code
Patch 1 is a requirement to cover for memleaks created by the latter ones, but is categorically correct even by itself. Patches 2 and 3 fix actual bugs. Phil Sutter (3): nft: Fix potential memleaks in nft_*_rule_find() xtables: Fix for crash when comparing rules with standard target xtables: Fix for false-positive rule matching iptables/nft-arp.c| 12 -- iptables/nft-bridge.c | 23 --- iptables/nft-shared.c | 41 +++ .../testcases/iptables/0005-delete-rules_0| 14 +++ libxtables/xtables.c | 18 +++- 5 files changed, 89 insertions(+), 19 deletions(-) create mode 100755 iptables/tests/shell/testcases/iptables/0005-delete-rules_0 -- 2.20.1
[iptables PATCH 3/3] xtables: Fix for false-positive rule matching
When comparing two rules with non-standard targets, differences in
targets' payloads wasn't respected.
The cause is a rather hideous one: Unlike xtables_find_match(),
xtables_find_target() did not care whether the found target was already
in use or not, so the same target instance was assigned to both rules
and therefore payload comparison happened over the same memory location.
With legacy iptables it is not possible to reuse a target: The only case
where two rules (i.e., iptables_command_state instances) could exist at
the same time is when comparing rules, but that's handled using libiptc.
Signed-off-by: Phil Sutter
---
iptables/nft-bridge.c | 9 +
iptables/nft-shared.c | 8 +++-
.../testcases/iptables/0005-delete-rules_0 | 7 +++
libxtables/xtables.c | 18 +-
4 files changed, 40 insertions(+), 2 deletions(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index cbd671c38d2e1..9b0c6932ccca5 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -45,6 +45,15 @@ void ebt_cs_clean(struct iptables_command_state *cs)
free(m);
m = nm;
}
+
+ if (cs->target) {
+ free(cs->target->t);
+
+ if (cs->target == cs->target->next) {
+ free(cs->target);
+ cs->target = NULL;
+ }
+ }
}
static void ebt_print_mac(const unsigned char *mac)
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index caed347ddb724..d24b27ec146bc 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -684,8 +684,14 @@ void nft_rule_to_iptables_command_state(const struct
nftnl_rule *r,
void nft_clear_iptables_command_state(struct iptables_command_state *cs)
{
xtables_rule_matches_free(&cs->matches);
- if (cs->target)
+ if (cs->target) {
free(cs->target->t);
+
+ if (cs->target == cs->target->next) {
+ free(cs->target);
+ cs->target = NULL;
+ }
+ }
}
void print_header(unsigned int format, const char *chain, const char *pol,
diff --git a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
index 9312fd53c3437..5038cbce5a5cf 100755
--- a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
+++ b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
@@ -5,3 +5,10 @@
$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j DROP
$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j REJECT
[[ $? -eq 1 ]] || exit 1
+
+# test incorrect deletion of rules with deviating payload
+# in non-standard target
+
+$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j MARK --set-mark 23
+$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j MARK --set-mark 42
+[[ $? -eq 1 ]] || exit 1
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index ea9bb102c8eb4..895f6988eaf57 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -756,8 +756,24 @@ xtables_find_target(const char *name, enum xtables_tryload
tryload)
}
for (ptr = xtables_targets; ptr; ptr = ptr->next) {
- if (extension_cmp(name, ptr->name, ptr->family))
+ if (extension_cmp(name, ptr->name, ptr->family)) {
+ struct xtables_target *clone;
+
+ /* First target of this type: */
+ if (ptr->t == NULL)
+ break;
+
+ /* Second and subsequent clones */
+ clone = xtables_malloc(sizeof(struct xtables_target));
+ memcpy(clone, ptr, sizeof(struct xtables_target));
+ clone->udata = NULL;
+ clone->tflags = 0;
+ /* This is a clone: */
+ clone->next = clone;
+
+ ptr = clone;
break;
+ }
}
#ifndef NO_SHARED_LIBS
--
2.20.1
Re: [RFC nft] evaluate: kill anon sets with one element
Hi,
On Fri, Jan 25, 2019 at 04:59:46PM +0100, Florian Westphal wrote:
> pretends that
> ip saddr { 1.1.1.1 }
>
> is
> ip saddr 1.1.1.1
>
> Needs more work (breaks dumps in test cases afaics).
> Is that a good idea in first place?
>
> I see various new users adopting
>
> { single-value }
>
> probably due to copying/adapting a "{ foo, bar }" rule to "{ foo }"...
I think it's a good idea. Just like with auto-merging of ranges,
anonymous sets may (and IMHO should) optimize themselves as much as
possible.
Cheers, Phil
Re: [iptables PATCH 3/3] xtables: Fix for inserting rule at wrong position
On Mon, Jan 28, 2019 at 11:29:09AM +0100, Pablo Neira Ayuso wrote: > On Tue, Jan 15, 2019 at 11:23:05PM +0100, Phil Sutter wrote: > > iptables-restore allows to insert rules at a certain position which is > > problematic for iptables-nft to realize since rule position is not > > determined by number but handle of previous or following rule and in > > case the rules surrounding the new one are new as well, they don't have > > a handle to refer to yet. > > > > Fix this by making use of NFTNL_RULE_POSITION_ID attribute: When > > inserting before a rule which does not have a handle, refer to it using > > its NFTNL_RULE_ID value. If the latter doesn't exist either, assign a > > new one to it. > > > > The last used rule ID value is tracked in a new field of struct > > nft_handle which is incremented before each use. > > Much nicer, thanks a lot for reworking this, Phil. Yes, your RULE_POSITION suggestion was priceless, thanks a lot! > Can you see any more problems with rule insertions at positions via > iptables-restore? I can't think of any, but that doesn't necessarily mean there are none. :) > I guess we'll need a patch for nft too, will you give it a shot? I guess it is possible to create the same problems with 'nft -f' and separate 'add rule' calls containing 'index' reference. Though according to nft.8, 'index' has to be that of an existing rule. So in that sense it's rather a missing feature and I would prefer to treat it as such. Cheers, Phil
Re: [PATCH nft] include: add cplusplus guards for extern
On Mon, Jan 28, 2019 at 10:46:31AM +0100, Pablo Neira Ayuso wrote: > Signed-off-by: Pablo Neira Ayuso Acked-by: Phil Sutter
Re: [PATCH nf-next] netfilter: nf_tables: add NFTA_RULE_POSITION_ID to nla_policy
On Tue, Jan 29, 2019 at 09:15:02AM +0100, Florian Westphal wrote:
> Fixes: 75dd48e2e420a ("netfilter: nf_tables: Support RULE_ID reference in new
> rule")
> Reported-by: Cong Wang
> Signed-off-by: Florian Westphal
Acked-by: Phil Sutter
Thanks for taking care of this one!
Cheers, Phil
Re: [iptables PATCH] xtables: Speed up chain deletion in large rulesets
Hi Pablo, On Fri, Dec 21, 2018 at 02:13:54PM +0100, Phil Sutter wrote: > On Fri, Dec 21, 2018 at 12:41:31PM +0100, Pablo Neira Ayuso wrote: > > On Wed, Dec 12, 2018 at 08:04:12PM +0100, Phil Sutter wrote: > > > Kernel prefers to identify chain by handle if it was given which causes > > > manual traversal of the chain list. In contrast, chain lookup by name in > > > kernel makes use of a hash table so is considerably faster. Force this > > > code path by removing the cached chain's handle when removing it. > > > > > > Signed-off-by: Phil Sutter > > > --- > > > iptables/nft.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/iptables/nft.c b/iptables/nft.c > > > index 5ef3a75efcde5..8ff21e09f0344 100644 > > > --- a/iptables/nft.c > > > +++ b/iptables/nft.c > > > @@ -1643,6 +1643,7 @@ static int __nft_chain_user_del(struct nftnl_chain > > > *c, void *data) > > > fprintf(stdout, "Deleting chain `%s'\n", > > > nftnl_chain_get_str(c, NFTNL_CHAIN_NAME)); > > > > > > + nftnl_chain_unset(c, NFTNL_CHAIN_HANDLE); > > > > LGTM. > > > > We can probably add a hashtable for chain handle lookups in the > > kernel too so we can re-enable this in the future. > > Florian suggested to use an rbtree and some seqcount trickery to avoid > concurrency issues. I already started with that but decided I need to > learn more about kernel locking mechanisms before I submit something > stupid. :) I haven't found time yet to work on improved chain lookup by handle in kernel. So for the time being, would you mind applying this iptables patch? It doesn't hurt even with a later improved chain lookup by handle in place. If that is turning out to be even quicker than the current chain lookup by name via hashtable, we may drop this patch again. Thanks, Phil
[iptables PATCH 7/7] tests: shell: Add arptables-nft verbose output test
With arptables-nft output being in a very good state now, add a test to
ensure it stays that way.
Signed-off-by: Phil Sutter
---
.../arptables/0003-arptables-verbose-output_0 | 64 +++
1 file changed, 64 insertions(+)
create mode 100755
iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
diff --git
a/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
b/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
new file mode 100755
index 0..35126fa7d717c
--- /dev/null
+++ b/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
@@ -0,0 +1,64 @@
+#!/bin/bash
+
+set -e
+set -x
+
+# there is no legacy backend to test
+[[ $XT_MULTI == */xtables-nft-multi ]] || { echo "skip $XT_MULTI"; exit 0; }
+
+$XT_MULTI arptables -N foo
+
+# check verbose output matches expectations
+
+RULE1='-i eth23 -j ACCEPT'
+VOUT1='-j ACCEPT -i eth23 -o *'
+
+RULE2='-i eth23'
+VOUT2='-i eth23 -o *'
+
+RULE3='-i eth23 -j MARK --set-mark 42'
+VOUT3='-j MARK -i eth23 -o * --set-mark 42'
+
+RULE4='-o eth23 -j CLASSIFY --set-class 23:42'
+VOUT4='-j CLASSIFY -i * -o eth23 --set-class 23:42'
+
+RULE5='-o eth23 -j foo'
+VOUT5='-j foo -i * -o eth23'
+
+RULE6='-o eth23 -j mangle --mangle-ip-s 10.0.0.1'
+VOUT6='-j mangle -i * -o eth23 --mangle-ip-s 10.0.0.1'
+
+diff -u -Z <(echo -e "$VOUT1") <($XT_MULTI arptables -v -A INPUT $RULE1)
+diff -u -Z <(echo -e "$VOUT2") <($XT_MULTI arptables -v -A INPUT $RULE2)
+diff -u -Z <(echo -e "$VOUT3") <($XT_MULTI arptables -v -A INPUT $RULE3)
+diff -u -Z <(echo -e "$VOUT4") <($XT_MULTI arptables -v -A OUTPUT $RULE4)
+diff -u -Z <(echo -e "$VOUT5") <($XT_MULTI arptables -v -A OUTPUT $RULE5)
+diff -u -Z <(echo -e "$VOUT6") <($XT_MULTI arptables -v -A foo $RULE6)
+
+EXPECT='Chain INPUT (policy ACCEPT 0 packets, 0 bytes)
+-j ACCEPT -i eth23 -o *, pcnt=0 -- bcnt=0
+-i eth23 -o *, pcnt=0 -- bcnt=0
+-j MARK -i eth23 -o * --set-mark 42, pcnt=0 -- bcnt=0
+
+Chain OUTPUT (policy ACCEPT 0 packets, 0 bytes)
+-j CLASSIFY -i * -o eth23 --set-class 23:42, pcnt=0 -- bcnt=0
+-j foo -i * -o eth23, pcnt=0 -- bcnt=0
+
+Chain foo (1 references)
+-j mangle -i * -o eth23 --mangle-ip-s 10.0.0.1, pcnt=0 -- bcnt=0'
+
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI arptables -v -n -L)
+
+EXPECT='*filter
+:INPUT ACCEPT
+:OUTPUT ACCEPT
+:foo -
+-A INPUT -j ACCEPT -i eth23
+-A INPUT -i eth23
+-A INPUT -j MARK -i eth23 --set-mark 42
+-A OUTPUT -j CLASSIFY -o eth23 --set-class 23:42
+-A OUTPUT -j foo -o eth23
+-A foo -j mangle -o eth23 --mangle-ip-s 10.0.0.1
+'
+
+diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI arptables-save)
--
2.20.1
[iptables PATCH 6/7] arptables-nft: Don't print default h-len/h-type values
Default values for --h-len and --h-type being printed for rules where
user didn't provide them is unexpected and confusing. The drawback is
the opposite: If user provided either of them with their default value,
they are later omitted when listing rules. Though since unlike legacy
arptables we can't distinguish between not specified and specified with
default value, we can't fix both - so choose to optimize for the more
likely case.
Fixes: 5aecb2d8bfdda ("arptables: pre-init hlen and ethertype")
Signed-off-by: Phil Sutter
---
iptables/nft-arp.c| 4 +--
.../arptables/0001-arptables-save-restore_0 | 32 +--
.../0002-arptables-restore-defaults_0 | 6 ++--
3 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 3dc0b9534e747..438646de6d45d 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -537,7 +537,7 @@ after_devsrc:
after_devdst:
- if (fw->arp.arhln_mask != 0) {
+ if (fw->arp.arhln_mask != 255 || fw->arp.arhln != 6) {
printf("%s%s", sep, fw->arp.invflags & ARPT_INV_ARPHLN
? "! " : "");
printf("--h-length %d", fw->arp.arhln);
@@ -561,7 +561,7 @@ after_devdst:
sep = " ";
}
- if (fw->arp.arhrd_mask != 0) {
+ if (fw->arp.arhrd_mask != 65535 || fw->arp.arhrd != htons(1)) {
uint16_t tmp = ntohs(fw->arp.arhrd);
printf("%s%s", sep, fw->arp.invflags & ARPT_INV_ARPHRD
diff --git
a/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
b/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
index 0664e3b38d5e8..e10f61cc8f95b 100755
--- a/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
+++ b/iptables/tests/shell/testcases/arptables/0001-arptables-save-restore_0
@@ -35,22 +35,22 @@ DUMP='*filter
:INPUT ACCEPT
:OUTPUT DROP
:foo -
--A INPUT -j ACCEPT -s 10.0.0.0/8 --h-length 6 --h-type 1
--A INPUT -j ACCEPT -d 192.168.123.1 --h-length 6 --h-type 1
--A INPUT -j ACCEPT --src-mac fe:ed:ba:be:00:01 --h-length 6 --h-type 1
--A INPUT -j ACCEPT --dst-mac fe:ed:ba:be:00:01 --h-length 6 --h-type 1
--A INPUT -j foo --h-length 6 --h-type 1
--A INPUT --h-length 6 --h-type 1
--A OUTPUT -j ACCEPT -o lo --h-length 6 --h-type 1
--A OUTPUT -j mangle -o eth134 --h-length 6 --h-type 1 --mangle-ip-s 10.0.0.1
--A OUTPUT -j CLASSIFY -o eth432 --h-length 6 --h-type 1 --set-class feed:babe
--A OUTPUT -j CLASSIFY -o eth432 --h-length 6 --opcode 1 --h-type 1 --set-class
feed:babe
--A foo -j ACCEPT -i lo --h-length 6 --h-type 1
--A foo -j ACCEPT --h-length 6 --h-type 1
--A foo -j MARK --h-length 6 --h-type 1 --set-mark 12345
--A foo -j ACCEPT --h-length 6 --opcode 1 --h-type 1
--A foo -j ACCEPT --h-length 6 --h-type 1 --proto-type 0x800
--A foo -j ACCEPT -i lo --h-length 6 --opcode 1 --h-type 1 --proto-type 0x800
+-A INPUT -j ACCEPT -s 10.0.0.0/8
+-A INPUT -j ACCEPT -d 192.168.123.1
+-A INPUT -j ACCEPT --src-mac fe:ed:ba:be:00:01
+-A INPUT -j ACCEPT --dst-mac fe:ed:ba:be:00:01
+-A INPUT -j foo
+-A INPUT
+-A OUTPUT -j ACCEPT -o lo
+-A OUTPUT -j mangle -o eth134 --mangle-ip-s 10.0.0.1
+-A OUTPUT -j CLASSIFY -o eth432 --set-class feed:babe
+-A OUTPUT -j CLASSIFY -o eth432 --opcode 1 --set-class feed:babe
+-A foo -j ACCEPT -i lo
+-A foo -j ACCEPT
+-A foo -j MARK --set-mark 12345
+-A foo -j ACCEPT --opcode 1
+-A foo -j ACCEPT --proto-type 0x800
+-A foo -j ACCEPT -i lo --opcode 1 --proto-type 0x800
'
diff -u <(echo -e "$DUMP") <($XT_MULTI arptables-save)
diff --git
a/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
b/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
index d742c3d506305..b2ed95e87bb40 100755
--- a/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
+++ b/iptables/tests/shell/testcases/arptables/0002-arptables-restore-defaults_0
@@ -11,7 +11,7 @@ set -e
DUMP='*filter
:OUTPUT ACCEPT
-A OUTPUT -j mangle --mangle-ip-s 10.0.0.1
--A OUTPUT -j mangle --h-length 6 --h-type 1 --mangle-ip-d 10.0.0.2
+-A OUTPUT -j mangle --mangle-ip-d 10.0.0.2
'
# note how mangle-ip-s is unset in second rule
@@ -19,8 +19,8 @@ DUMP='*filter
EXPECT='*filter
:INPUT ACCEPT
:OUTPUT ACCEPT
--A OUTPUT -j mangle --h-length 6 --h-type 1 --mangle-ip-s 10.0.0.1
--A OUTPUT -j mangle --h-length 6 --h-type 1 --mangle-ip-d 10.0.0.2
+-A OUTPUT -j mangle --mangle-ip-s 10.0.0.1
+-A OUTPUT -j mangle --mangle-ip-d 10.0.0.2
'
$XT_MULTI arptables -F
--
2.20.1
[iptables PATCH 1/7] arptables-nft: Fix listing rules without target
Don't try to print cs.jumpto if it is an empty string, otherwise listing
(and verbose output) contains '-j' flag without argument.
Signed-off-by: Phil Sutter
---
iptables/nft-arp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 399f83afff5cc..5567daa7ad530 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -598,7 +598,7 @@ nft_arp_print_rule(struct nftnl_rule *r, unsigned int num,
unsigned int format)
nft_rule_to_iptables_command_state(r, &cs);
- if (cs.jumpto)
+ if (strlen(cs.jumpto))
printf("-j %s ", cs.jumpto);
nft_arp_print_rule_details(&cs.arp, format);
print_matches_and_target(&cs, format);
--
2.20.1
[iptables PATCH 5/7] arptables-nft-save: Fix position of -j option
Legacy arptables-save (just like arptables itself) prints verdict as
first option, then matches and finally any target options.
To achieve this without introducing double/trailing spaces everywhere,
integrate target ('-j') option printing into
nft_arp_print_rule_details() and make it print separating whitespace
before each option.
In nft_arp_save_rule(), replace the call to save_matches_and_target() by
by a direct call to cs->target->save() since the former prints '-j'
option itself. Since there are no match extensions in arptables, any
other code from that function is not needed.
Signed-off-by: Phil Sutter
---
iptables/nft-arp.c| 65 +++
.../arptables/0001-arptables-save-restore_0 | 32 -
.../0002-arptables-restore-defaults_0 | 6 +-
3 files changed, 58 insertions(+), 45 deletions(-)
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index b0405be8b2a22..3dc0b9534e747 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -434,14 +434,21 @@ static void nft_arp_print_header(unsigned int format,
const char *chain,
}
}
-static void nft_arp_print_rule_details(const struct arpt_entry *fw,
+static void nft_arp_print_rule_details(const struct iptables_command_state *cs,
unsigned int format)
{
+ const struct arpt_entry *fw = &cs->arp;
char buf[BUFSIZ];
char iface[IFNAMSIZ+2];
+ const char *sep = "";
int print_iface = 0;
int i;
+ if (strlen(cs->jumpto)) {
+ printf("%s-j %s", sep, cs->jumpto);
+ sep = " ";
+ }
+
iface[0] = '\0';
if (fw->arp.iniface[0] != '\0') {
@@ -453,9 +460,11 @@ static void nft_arp_print_rule_details(const struct
arpt_entry *fw,
if (format & FMT_NUMERIC) strcat(iface, "*");
else strcat(iface, "any");
}
- if (print_iface)
- printf("%s-i %s ", fw->arp.invflags & ARPT_INV_VIA_IN ?
+ if (print_iface) {
+ printf("%s%s-i %s", sep, fw->arp.invflags & ARPT_INV_VIA_IN ?
"! " : "", iface);
+ sep = " ";
+ }
print_iface = 0;
iface[0] = '\0';
@@ -469,12 +478,14 @@ static void nft_arp_print_rule_details(const struct
arpt_entry *fw,
if (format & FMT_NUMERIC) strcat(iface, "*");
else strcat(iface, "any");
}
- if (print_iface)
- printf("%s-o %s ", fw->arp.invflags & ARPT_INV_VIA_OUT ?
+ if (print_iface) {
+ printf("%s%s-o %s", sep, fw->arp.invflags & ARPT_INV_VIA_OUT ?
"! " : "", iface);
+ sep = " ";
+ }
if (fw->arp.smsk.s_addr != 0L) {
- printf("%s", fw->arp.invflags & ARPT_INV_SRCIP
+ printf("%s%s", sep, fw->arp.invflags & ARPT_INV_SRCIP
? "! " : "");
if (format & FMT_NUMERIC)
sprintf(buf, "%s", addr_to_dotted(&(fw->arp.src)));
@@ -482,7 +493,8 @@ static void nft_arp_print_rule_details(const struct
arpt_entry *fw,
sprintf(buf, "%s", addr_to_anyname(&(fw->arp.src)));
strncat(buf, mask_to_dotted(&(fw->arp.smsk)),
sizeof(buf) - strlen(buf) - 1);
- printf("-s %s ", buf);
+ printf("-s %s", buf);
+ sep = " ";
}
for (i = 0; i < ARPT_DEV_ADDR_LEN_MAX; i++)
@@ -490,16 +502,16 @@ static void nft_arp_print_rule_details(const struct
arpt_entry *fw,
break;
if (i == ARPT_DEV_ADDR_LEN_MAX)
goto after_devsrc;
- printf("%s", fw->arp.invflags & ARPT_INV_SRCDEVADDR
+ printf("%s%s", sep, fw->arp.invflags & ARPT_INV_SRCDEVADDR
? "! " : "");
printf("--src-mac ");
print_mac_and_mask((unsigned char *)fw->arp.src_devaddr.addr,
(unsigned char *)fw->arp.src_devaddr.mask, ETH_ALEN);
- printf(" ");
+ sep = " ";
after_devsrc:
if (fw->arp.tmsk.s_addr != 0L) {
- printf("%s", fw->arp.invflags & ARPT_INV_TGTIP
+ printf("%s%s", sep, fw->arp.invflags & ARPT_INV_TGTIP
? "! " : "");
if (format & FMT_NUMERIC)
sprintf(buf,
[iptables PATCH 0/7] Align arptables-nft output with legacy
This series is the result of a comparison between legacy and nft arptables outputs in verbose mode, ruleset listing and dumps (through arptables-save). Foremost, this fixes a few real bugs: * Stale printing of '-j' for rules without target (patch 1). * Mark value was parsed in decimal but legacy arptables assumed hex input at all times (patch 2). Aligning arptables-nft output with legacy one is rather important IMO since there is no way to check existence of a rule (like with 'iptables -C'), so one should expect existence of scripts parsing list/save output. Therefore I think it is acceptable to carry quite a bit of extra code in MARK and CLASSIFY targets. Patch 6 might seem like a convenience change, but there is a hidden problem it fixes: --h-len and --h-type options were not printed if they were zero, but a dump not containing them would cause them to reset to their default value (6 and 1) in affected rules. Phil Sutter (7): arptables-nft: Fix listing rules without target arptables-nft: Fix MARK target parsing and printing arptables-nft: Fix CLASSIFY target printing arptables-nft: Remove space between *cnt= and value arptables-nft-save: Fix position of -j option arptables-nft: Don't print default h-len/h-type values tests: shell: Add arptables-nft verbose output test extensions/libxt_CLASSIFY.c | 59 +--- extensions/libxt_MARK.c | 95 +++ iptables/nft-arp.c| 73 -- .../arptables/0001-arptables-save-restore_0 | 32 +++ .../0002-arptables-restore-defaults_0 | 6 +- .../arptables/0003-arptables-verbose-output_0 | 64 + 6 files changed, 267 insertions(+), 62 deletions(-) create mode 100755 iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0 -- 2.20.1
[iptables PATCH 2/7] arptables-nft: Fix MARK target parsing and printing
Legacy arptables parses mark values in hex no matter if prefixed with
'0x' or not. Sadly, this is not easily achievable with guided option
parser. Hence fall back to the old 'parse' callback. The introduced
target definition is valid only for revision 2, but that's consistent
with legacy arptables.
When printing, use --set-mark option instead of --set-xmark.
Signed-off-by: Phil Sutter
---
extensions/libxt_MARK.c | 95 +++
.../arptables/0001-arptables-save-restore_0 | 2 +-
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/extensions/libxt_MARK.c b/extensions/libxt_MARK.c
index 43aa977924b12..b765af6c35304 100644
--- a/extensions/libxt_MARK.c
+++ b/extensions/libxt_MARK.c
@@ -1,3 +1,4 @@
+#include
#include
#include
#include
@@ -245,6 +246,87 @@ static void mark_tg_save(const void *ip, const struct
xt_entry_target *target)
printf(" --set-xmark 0x%x/0x%x", info->mark, info->mask);
}
+static void mark_tg_arp_save(const void *ip, const struct xt_entry_target
*target)
+{
+ const struct xt_mark_tginfo2 *info = (const void *)target->data;
+
+ if (info->mark == 0)
+ printf(" --and-mark %x", (unsigned int)(uint32_t)~info->mask);
+ else if (info->mark == info->mask)
+ printf(" --or-mark %x", info->mark);
+ else
+ printf(" --set-mark %x", info->mark);
+}
+
+static void mark_tg_arp_print(const void *ip,
+ const struct xt_entry_target *target, int numeric)
+{
+ mark_tg_arp_save(ip, target);
+}
+
+#define MARK_OPT 1
+#define AND_MARK_OPT 2
+#define OR_MARK_OPT 3
+
+static struct option mark_tg_arp_opts[] = {
+ { .name = "set-mark", .has_arg = required_argument, .flag = 0, .val =
MARK_OPT },
+ { .name = "and-mark", .has_arg = required_argument, .flag = 0, .val =
AND_MARK_OPT },
+ { .name = "or-mark", .has_arg = required_argument, .flag = 0, .val =
OR_MARK_OPT },
+ { .name = NULL}
+};
+
+static int
+mark_tg_arp_parse(int c, char **argv, int invert, unsigned int *flags,
+ const void *entry, struct xt_entry_target **target)
+{
+ struct xt_mark_tginfo2 *info =
+ (struct xt_mark_tginfo2 *)(*target)->data;
+ int i;
+
+ switch (c) {
+ case MARK_OPT:
+ if (sscanf(argv[optind-1], "%x", &i) != 1) {
+ xtables_error(PARAMETER_PROBLEM,
+ "Bad mark value `%s'", optarg);
+ return 0;
+ }
+ info->mark = i;
+ if (*flags)
+ xtables_error(PARAMETER_PROBLEM,
+ "MARK: Can't specify --set-mark twice");
+ *flags = 1;
+ break;
+ case AND_MARK_OPT:
+ if (sscanf(argv[optind-1], "%x", &i) != 1) {
+ xtables_error(PARAMETER_PROBLEM,
+ "Bad mark value `%s'", optarg);
+ return 0;
+ }
+ info->mark = 0;
+ info->mask = ~i;
+ if (*flags)
+ xtables_error(PARAMETER_PROBLEM,
+ "MARK: Can't specify --and-mark twice");
+ *flags = 1;
+ break;
+ case OR_MARK_OPT:
+ if (sscanf(argv[optind-1], "%x", &i) != 1) {
+ xtables_error(PARAMETER_PROBLEM,
+ "Bad mark value `%s'", optarg);
+ return 0;
+ }
+ info->mark = info->mask = i;
+ if (*flags)
+ xtables_error(PARAMETER_PROBLEM,
+ "MARK: Can't specify --or-mark twice");
+ *flags = 1;
+ break;
+ default:
+ return 0;
+ }
+ return 1;
+}
+
static int mark_tg_xlate(struct xt_xlate *xl,
const struct xt_xlate_tg_params *params)
{
@@ -335,6 +417,19 @@ static struct xtables_target mark_tg_reg[] = {
.x6_options= mark_tg_opts,
.xlate = mark_tg_xlate,
},
+ {
+ .version = XTABLES_VERSION,
+ .name = "MARK",
+ .revision = 2,
+ .family= NFPROTO_ARP,
+ .size = XT_ALIGN(sizeof(struct xt_mark_tginfo2)),
+ .userspacesize = XT_ALIGN(sizeof(struct xt_mark_tginfo2)),
+ .help = mark_tg_help,
+ .print = mark_tg_arp_print,
+ .save = mark_tg_arp_save,
+ .par
[iptables PATCH 4/7] arptables-nft: Remove space between *cnt= and value
When printing rule counters, call xtables_print_num() with FMT_NOTABLE
bit set to avoid spaces between equal sign and value.
Signed-off-by: Phil Sutter
---
iptables/nft-arp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 5567daa7ad530..b0405be8b2a22 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -605,9 +605,9 @@ nft_arp_print_rule(struct nftnl_rule *r, unsigned int num,
unsigned int format)
if (!(format & FMT_NOCOUNTS)) {
printf(", pcnt=");
- xtables_print_num(cs.counters.pcnt, format);
+ xtables_print_num(cs.counters.pcnt, format | FMT_NOTABLE);
printf("-- bcnt=");
- xtables_print_num(cs.counters.bcnt, format);
+ xtables_print_num(cs.counters.bcnt, format | FMT_NOTABLE);
}
if (!(format & FMT_NONEWLINE))
--
2.20.1
[iptables PATCH 3/7] arptables-nft: Fix CLASSIFY target printing
In legacy arptables, CLASSIFY target is not printed with fixed hex
number lengths. Counter this by introducing a dedicated target
definition for NFPROTO_ARP only having own print/save callbacks.
Signed-off-by: Phil Sutter
---
extensions/libxt_CLASSIFY.c | 59 +
1 file changed, 46 insertions(+), 13 deletions(-)
diff --git a/extensions/libxt_CLASSIFY.c b/extensions/libxt_CLASSIFY.c
index f90082dc7c50e..75aaf0c41b61a 100644
--- a/extensions/libxt_CLASSIFY.c
+++ b/extensions/libxt_CLASSIFY.c
@@ -73,6 +73,24 @@ CLASSIFY_save(const void *ip, const struct xt_entry_target
*target)
TC_H_MAJ(clinfo->priority)>>16, TC_H_MIN(clinfo->priority));
}
+static void
+CLASSIFY_arp_save(const void *ip, const struct xt_entry_target *target)
+{
+ const struct xt_classify_target_info *clinfo =
+ (const struct xt_classify_target_info *)target->data;
+
+ printf(" --set-class %x:%x",
+ TC_H_MAJ(clinfo->priority)>>16, TC_H_MIN(clinfo->priority));
+}
+
+static void
+CLASSIFY_arp_print(const void *ip,
+ const struct xt_entry_target *target,
+ int numeric)
+{
+ CLASSIFY_arp_save(ip, target);
+}
+
static int CLASSIFY_xlate(struct xt_xlate *xl,
const struct xt_xlate_tg_params *params)
{
@@ -98,21 +116,36 @@ static int CLASSIFY_xlate(struct xt_xlate *xl,
return 1;
}
-static struct xtables_target classify_target = {
- .family = NFPROTO_UNSPEC,
- .name = "CLASSIFY",
- .version= XTABLES_VERSION,
- .size = XT_ALIGN(sizeof(struct xt_classify_target_info)),
- .userspacesize = XT_ALIGN(sizeof(struct xt_classify_target_info)),
- .help = CLASSIFY_help,
- .print = CLASSIFY_print,
- .save = CLASSIFY_save,
- .x6_parse = CLASSIFY_parse,
- .x6_options = CLASSIFY_opts,
- .xlate = CLASSIFY_xlate,
+static struct xtables_target classify_tg_reg[] = {
+ {
+ .family = NFPROTO_UNSPEC,
+ .name = "CLASSIFY",
+ .version= XTABLES_VERSION,
+ .size = XT_ALIGN(sizeof(struct
xt_classify_target_info)),
+ .userspacesize = XT_ALIGN(sizeof(struct
xt_classify_target_info)),
+ .help = CLASSIFY_help,
+ .print = CLASSIFY_print,
+ .save = CLASSIFY_save,
+ .x6_parse = CLASSIFY_parse,
+ .x6_options = CLASSIFY_opts,
+ .xlate = CLASSIFY_xlate,
+ },
+ {
+ .family = NFPROTO_ARP,
+ .name = "CLASSIFY",
+ .version= XTABLES_VERSION,
+ .size = XT_ALIGN(sizeof(struct
xt_classify_target_info)),
+ .userspacesize = XT_ALIGN(sizeof(struct
xt_classify_target_info)),
+ .help = CLASSIFY_help,
+ .print = CLASSIFY_arp_print,
+ .save = CLASSIFY_arp_save,
+ .x6_parse = CLASSIFY_parse,
+ .x6_options = CLASSIFY_opts,
+ .xlate = CLASSIFY_xlate,
+ }
};
void _init(void)
{
- xtables_register_target(&classify_target);
+ xtables_register_targets(classify_tg_reg, ARRAY_SIZE(classify_tg_reg));
}
--
2.20.1
Re: [iptables PATCH 0/7] Align arptables-nft output with legacy
On Thu, Jan 31, 2019 at 05:23:05PM +0100, Florian Westphal wrote: > Phil Sutter wrote: > > Aligning arptables-nft output with legacy one is rather important IMO > > since there is no way to check existence of a rule (like with 'iptables > > -C'), so one should expect existence of scripts parsing list/save > > output. Therefore I think it is acceptable to carry quite a bit of extra > > code in MARK and CLASSIFY targets. > > Sucks but I agree. I will review (and likely apply) this series later today. Thanks! And yes, it is rather ugly. Luckily arptables doesn't have many extensions, that's also the reason why I went with the copy'n'paste programming approach in patches 2 and 3 instead of extending the guided option parser in libxtables. Cheers, Phil
[iptables PATCH 1/2] arptables-nft: Set h-type/h-length masks by default, too
These masks are not used in nftables backend, but mangle extension
checks arhln_mask value to make sure --h-length was given (which is
implicitly the case).
Fixes: 5aecb2d8bfdda ("arptables: pre-init hlen and ethertype")
Signed-off-by: Phil Sutter
---
iptables/xtables-arp.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 57e717fa901a1..4b663775c5bee 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -910,8 +910,12 @@ int do_commandarp(struct nft_handle *h, int argc, char
*argv[], char **table,
{
struct iptables_command_state cs = {
.jumpto = "",
- .arp.arp.arhln = 6,
- .arp.arp.arhrd = htons(ARPHRD_ETHER),
+ .arp.arp = {
+ .arhln = 6,
+ .arhln_mask = 255,
+ .arhrd = htons(ARPHRD_ETHER),
+ .arhrd_mask = 65535,
+ },
};
int invert = 0;
unsigned int nsaddrs = 0, ndaddrs = 0;
--
2.20.1
[iptables PATCH 0/2] Follow-up on arptables output changes
Changes to how arptables-nft prints rules accidentally broke extension testcases. While fixing them (patch 2), a problem with default setting of --h-length and --h-type was discovered regarding mangle extension. This one is fixed in patch 1. Phil Sutter (2): arptables-nft: Set h-type/h-length masks by default, too extensions: Fix arptables extension tests extensions/libarpt_CLASSIFY.t | 2 +- extensions/libarpt_MARK.t | 6 +++--- extensions/libarpt_mangle.t | 8 extensions/libarpt_standard.t | 4 ++-- iptables/xtables-arp.c| 8 ++-- 5 files changed, 16 insertions(+), 12 deletions(-) -- 2.20.1
[iptables PATCH 2/2] extensions: Fix arptables extension tests
With changes to arptables-nft output, many of these tests fail because rules are not printed as expected anymore. Since most of the tests with explicitly defined output did so just because of added --h-length and --h-type options, adjust input a little more (typically reordering of arguments) to make output match input. Signed-off-by: Phil Sutter --- extensions/libarpt_CLASSIFY.t | 2 +- extensions/libarpt_MARK.t | 6 +++--- extensions/libarpt_mangle.t | 8 extensions/libarpt_standard.t | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/extensions/libarpt_CLASSIFY.t b/extensions/libarpt_CLASSIFY.t index c30480d2b160c..0cf0f2ce8e736 100644 --- a/extensions/libarpt_CLASSIFY.t +++ b/extensions/libarpt_CLASSIFY.t @@ -1,4 +1,4 @@ :OUTPUT -o lo --destination-mac 11:22:33:44:55:66;-o lo --dst-mac 11:22:33:44:55:66;OK --dst-mac Broadcast ;--dst-mac ff:ff:ff:ff:ff:ff;OK -! -o eth+ -d 1.2.3.4/24 -j CLASSIFY --set-class :;! -o eth+ -d 1.2.3.0/24 --h-length 6 --h-type 1 -j CLASSIFY --set-class :;OK +! -o eth+ -d 1.2.3.4/24 -j CLASSIFY --set-class 0:0;-j CLASSIFY ! -o eth+ -d 1.2.3.0/24 --set-class 0:0;OK diff --git a/extensions/libarpt_MARK.t b/extensions/libarpt_MARK.t index cb4c2cb630357..3b13d44fd2c4b 100644 --- a/extensions/libarpt_MARK.t +++ b/extensions/libarpt_MARK.t @@ -1,4 +1,4 @@ :INPUT,OUTPUT --d 0.0.0.0/8 -j MARK --set-mark 0x1;-d 0.0.0.0/8 --h-length 6 --h-type 1 -j MARK --set-xmark 0x1/0x;OK --s ! 0.0.0.0 -j MARK --and-mark 0x17;! -s 0.0.0.0 --h-length 6 --h-type 1 -j MARK --set-xmark 0x0/0xffe8;OK --s 0.0.0.0 -j MARK --or-mark 0x17;-s 0.0.0.0 --h-length 6 --h-type 1 -j MARK --set-xmark 0x17/0x17;OK +-j MARK -d 0.0.0.0/8 --set-mark 1;=;OK +-s ! 0.0.0.0 -j MARK --and-mark 0x17;-j MARK ! -s 0.0.0.0 --and-mark 17;OK +-j MARK -s 0.0.0.0 --or-mark 17;=;OK diff --git a/extensions/libarpt_mangle.t b/extensions/libarpt_mangle.t index 1d4c3977d2a4c..da9669489d291 100644 --- a/extensions/libarpt_mangle.t +++ b/extensions/libarpt_mangle.t @@ -1,5 +1,5 @@ :OUTPUT --s 1.2.3.4 -j mangle --mangle-ip-s 1.2.3.5;-s 1.2.3.4 --h-length 6 --h-type 1 -j mangle --mangle-ip-s 1.2.3.5;OK --d 1.2.3.4 -j mangle --mangle-ip-d 1.2.3.5;-d 1.2.3.4 --h-length 6 --h-type 1 -j mangle --mangle-ip-d 1.2.3.5;OK --d 1.2.3.4 --h-length 6 --h-type 1 -j mangle --mangle-mac-d 00:01:02:03:04:05;=;OK --d 1.2.3.4 -j mangle --mangle-mac-s 00:01:02:03:04:05;=;FAIL +-j mangle -s 1.2.3.4 --mangle-ip-s 1.2.3.5;=;OK +-j mangle -d 1.2.3.4 --mangle-ip-d 1.2.3.5;=;OK +-j mangle -d 1.2.3.4 --mangle-mac-d 00:01:02:03:04:05;=;OK +-d 1.2.3.4 --h-length 5 -j mangle --mangle-mac-s 00:01:02:03:04:05;=;FAIL diff --git a/extensions/libarpt_standard.t b/extensions/libarpt_standard.t index 195865929c8d3..e84a00b780488 100644 --- a/extensions/libarpt_standard.t +++ b/extensions/libarpt_standard.t @@ -5,8 +5,8 @@ -d 192.168.0.1;=;OK ! -d 0.0.0.0;=;OK -d 0.0.0.0/24;=;OK --i lo -j DROP;-i lo --h-length 6 --h-type 1 -j DROP;OK -! -i lo -j ACCEPT;! -i lo --h-length 6 --h-type 1 -j ACCEPT;OK +-j DROP -i lo;=;OK +-j ACCEPT ! -i lo;=;OK -i ppp+;=;OK ! -i ppp+;=;OK -i lo --destination-mac 11:22:33:44:55:66;-i lo --dst-mac 11:22:33:44:55:66;OK -- 2.20.1
[iptables PATCH v2 0/2] xtables: Fix multiple issues in rule matching code
Patch 1 caused segfaults due to double free of cs->target->t. For unclear reasons, patch 2 fixed that situation again which is why I didn't notice it. This updated series has patch 1 set cs->target->t to NULL after freeing it to prevent the double free. Patch 2 didn't apply cleanly onto the changed patch 1, so sending an adjusted version here. Phil Sutter (2): xtables: Fix for crash when comparing rules with standard target xtables: Fix for false-positive rule matching iptables/nft-bridge.c | 9 ++ iptables/nft-shared.c | 28 --- .../testcases/iptables/0005-delete-rules_0| 14 ++ iptables/xtables.c| 4 ++- libxtables/xtables.c | 18 +++- 5 files changed, 67 insertions(+), 6 deletions(-) create mode 100755 iptables/tests/shell/testcases/iptables/0005-delete-rules_0 -- 2.20.1
[iptables PATCH v2 2/2] xtables: Fix for false-positive rule matching
When comparing two rules with non-standard targets, differences in
targets' payloads wasn't respected.
The cause is a rather hideous one: Unlike xtables_find_match(),
xtables_find_target() did not care whether the found target was already
in use or not, so the same target instance was assigned to both rules
and therefore payload comparison happened over the same memory location.
With legacy iptables it is not possible to reuse a target: The only case
where two rules (i.e., iptables_command_state instances) could exist at
the same time is when comparing rules, but that's handled using libiptc.
Signed-off-by: Phil Sutter
---
Changes since v1:
- Reapply onto changed patch 1.
---
iptables/nft-bridge.c | 9 +
iptables/nft-shared.c | 5 +
.../testcases/iptables/0005-delete-rules_0 | 7 +++
libxtables/xtables.c | 18 +-
4 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index b4265c8af4f70..5757bedcd8724 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -45,6 +45,15 @@ void ebt_cs_clean(struct iptables_command_state *cs)
free(m);
m = nm;
}
+
+ if (cs->target) {
+ free(cs->target->t);
+
+ if (cs->target == cs->target->next) {
+ free(cs->target);
+ cs->target = NULL;
+ }
+ }
}
static void ebt_print_mac(const unsigned char *mac)
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 2d4b8d557eb73..a72d414d78111 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -687,6 +687,11 @@ void nft_clear_iptables_command_state(struct
iptables_command_state *cs)
if (cs->target) {
free(cs->target->t);
cs->target->t = NULL;
+
+ if (cs->target == cs->target->next) {
+ free(cs->target);
+ cs->target = NULL;
+ }
}
}
diff --git a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
index 9312fd53c3437..5038cbce5a5cf 100755
--- a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
+++ b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
@@ -5,3 +5,10 @@
$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j DROP
$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j REJECT
[[ $? -eq 1 ]] || exit 1
+
+# test incorrect deletion of rules with deviating payload
+# in non-standard target
+
+$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j MARK --set-mark 23
+$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j MARK --set-mark 42
+[[ $? -eq 1 ]] || exit 1
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index ea9bb102c8eb4..895f6988eaf57 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -756,8 +756,24 @@ xtables_find_target(const char *name, enum xtables_tryload
tryload)
}
for (ptr = xtables_targets; ptr; ptr = ptr->next) {
- if (extension_cmp(name, ptr->name, ptr->family))
+ if (extension_cmp(name, ptr->name, ptr->family)) {
+ struct xtables_target *clone;
+
+ /* First target of this type: */
+ if (ptr->t == NULL)
+ break;
+
+ /* Second and subsequent clones */
+ clone = xtables_malloc(sizeof(struct xtables_target));
+ memcpy(clone, ptr, sizeof(struct xtables_target));
+ clone->udata = NULL;
+ clone->tflags = 0;
+ /* This is a clone: */
+ clone->next = clone;
+
+ ptr = clone;
break;
+ }
}
#ifndef NO_SHARED_LIBS
--
2.20.1
[iptables PATCH v2 1/2] xtables: Fix for crash when comparing rules with standard target
When parsing an nftnl_rule with a standard verdict,
nft_rule_to_iptables_command_state() initialized cs->target but didn't
care about cs->target->t. When later comparing that rule to another,
compare_targets() crashed due to unconditional access to t's fields.
Signed-off-by: Phil Sutter
---
Changes since v1:
- Fix for double free caused by not setting cs->target->t to NULL after
freeing it.
---
iptables/nft-shared.c | 23 +++
.../testcases/iptables/0005-delete-rules_0| 7 ++
iptables/xtables.c| 4 +++-
3 files changed, 29 insertions(+), 5 deletions(-)
create mode 100755 iptables/tests/shell/testcases/iptables/0005-delete-rules_0
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index ca4e593656562..2d4b8d557eb73 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -660,19 +660,34 @@ void nft_rule_to_iptables_command_state(const struct
nftnl_rule *r,
match->m = m;
}
- if (cs->target != NULL)
+ if (cs->target != NULL) {
cs->jumpto = cs->target->name;
- else if (cs->jumpto != NULL)
+ } else if (cs->jumpto != NULL) {
+ struct xt_entry_target *t;
+ uint32_t size;
+
cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
- else
+ if (!cs->target)
+ return;
+
+ size = XT_ALIGN(sizeof(struct xt_entry_target)) +
cs->target->size;
+ t = xtables_calloc(1, size);
+ t->u.target_size = size;
+ t->u.user.revision = cs->target->revision;
+ strcpy(t->u.user.name, cs->jumpto);
+ cs->target->t = t;
+ } else {
cs->jumpto = "";
+ }
}
void nft_clear_iptables_command_state(struct iptables_command_state *cs)
{
xtables_rule_matches_free(&cs->matches);
- if (cs->target)
+ if (cs->target) {
free(cs->target->t);
+ cs->target->t = NULL;
+ }
}
void print_header(unsigned int format, const char *chain, const char *pol,
diff --git a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
new file mode 100755
index 0..9312fd53c3437
--- /dev/null
+++ b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
@@ -0,0 +1,7 @@
+#!/bin/bash
+
+# test for crash when comparing rules with standard target
+
+$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j DROP
+$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j REJECT
+[[ $? -eq 1 ]] || exit 1
diff --git a/iptables/xtables.c b/iptables/xtables.c
index d0167e6396975..eaa9fedeb03bb 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1185,8 +1185,10 @@ int do_commandx(struct nft_handle *h, int argc, char
*argv[], char **table,
*table = p.table;
xtables_rule_matches_free(&cs.matches);
- if (cs.target)
+ if (cs.target) {
free(cs.target->t);
+ cs.target->t = NULL;
+ }
if (h->family == AF_INET) {
free(args.s.addr.v4);
--
2.20.1
Re: [iptables PATCH v2 2/2] xtables: Fix for false-positive rule matching
On Fri, Feb 01, 2019 at 07:37:57PM +0100, Florian Westphal wrote: > Phil Sutter wrote: > > When comparing two rules with non-standard targets, differences in > > targets' payloads wasn't respected. > > > > The cause is a rather hideous one: Unlike xtables_find_match(), > > xtables_find_target() did not care whether the found target was already > > in use or not, so the same target instance was assigned to both rules > > and therefore payload comparison happened over the same memory location. > > > > With legacy iptables it is not possible to reuse a target: The only case > > where two rules (i.e., iptables_command_state instances) could exist at > > the same time is when comparing rules, but that's handled using libiptc. > > This causes: > > extensions/libebt_ip.t: ERROR: line 2 (cannot delete: ebtables -I INPUT -p ip > --ip-src ! 192.168.0.0/24 -j ACCEPT) > > (and similar errors). Oh crap, sorry for all the mess. I'll get this fixed, and in future keep in mind to run the extension testsuite before submitting a series. Thanks, Phil
[iptables PATCH v3] xtables: Fix for false-positive rule matching
When comparing two rules with non-standard targets, differences in
targets' payloads wasn't respected.
The cause is a rather hideous one: Unlike xtables_find_match(),
xtables_find_target() did not care whether the found target was already
in use or not, so the same target instance was assigned to both rules
and therefore payload comparison happened over the same memory location.
With legacy iptables it is not possible to reuse a target: The only case
where two rules (i.e., iptables_command_state instances) could exist at
the same time is when comparing rules, but that's handled using libiptc.
The above change clashes with ebtables-nft's reuse of target objects:
While input parsing still just assigns the object from xtables_targets
list, rule conversion from nftnl to iptables_command_state allocates new
data. To fix this, make ebtables-nft input parsing use the common
command_jump() routine instead of its own simplified copy. In turn, this
also eliminates the ebtables-nft-specific variants of parse_target(),
though with a slight change of behaviour: Names of user-defined chains
are no longer allowed to contain up to 31 but merely 28 characters.
Signed-off-by: Phil Sutter
---
Changes since v2:
- Fix for failing ebtables-nft rule deletion with standard target.
- Make sure extension, translate and shell testsuites pass.
Changes since v1:
- Reapply onto changed patch 1.
---
iptables/nft-bridge.c | 10
iptables/nft-bridge.h | 2 -
iptables/nft-shared.c | 5 ++
.../testcases/iptables/0005-delete-rules_0| 7 +++
iptables/xtables-eb-translate.c | 24 +-
iptables/xtables-eb.c | 47 +--
libxtables/xtables.c | 18 ++-
7 files changed, 41 insertions(+), 72 deletions(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index b4265c8af4f70..7c390dfa2a898 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -45,6 +45,16 @@ void ebt_cs_clean(struct iptables_command_state *cs)
free(m);
m = nm;
}
+
+ if (cs->target) {
+ free(cs->target->t);
+ cs->target->t = NULL;
+
+ if (cs->target == cs->target->next) {
+ free(cs->target);
+ cs->target = NULL;
+ }
+ }
}
static void ebt_print_mac(const unsigned char *mac)
diff --git a/iptables/nft-bridge.h b/iptables/nft-bridge.h
index de52cd7195bbb..d90066f1030a2 100644
--- a/iptables/nft-bridge.h
+++ b/iptables/nft-bridge.h
@@ -32,7 +32,6 @@ int ebt_get_mac_and_mask(const char *from, unsigned char *to,
unsigned char *mas
*/
#define EBT_TABLE_MAXNAMELEN 32
-#define EBT_CHAIN_MAXNAMELEN EBT_TABLE_MAXNAMELEN
#define EBT_FUNCTION_MAXNAMELEN EBT_TABLE_MAXNAMELEN
/* verdicts >0 are "branches" */
@@ -122,6 +121,5 @@ void ebt_add_match(struct xtables_match *m,
void ebt_add_watcher(struct xtables_target *watcher,
struct iptables_command_state *cs);
int ebt_command_default(struct iptables_command_state *cs);
-struct xtables_target *ebt_command_jump(const char *jumpto);
#endif
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 2d4b8d557eb73..a72d414d78111 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -687,6 +687,11 @@ void nft_clear_iptables_command_state(struct
iptables_command_state *cs)
if (cs->target) {
free(cs->target->t);
cs->target->t = NULL;
+
+ if (cs->target == cs->target->next) {
+ free(cs->target);
+ cs->target = NULL;
+ }
}
}
diff --git a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
index 9312fd53c3437..5038cbce5a5cf 100755
--- a/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
+++ b/iptables/tests/shell/testcases/iptables/0005-delete-rules_0
@@ -5,3 +5,10 @@
$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j DROP
$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j REJECT
[[ $? -eq 1 ]] || exit 1
+
+# test incorrect deletion of rules with deviating payload
+# in non-standard target
+
+$XT_MULTI iptables -A FORWARD -i eth23 -o eth42 -j MARK --set-mark 23
+$XT_MULTI iptables -D FORWARD -i eth23 -o eth42 -j MARK --set-mark 42
+[[ $? -eq 1 ]] || exit 1
diff --git a/iptables/xtables-eb-translate.c b/iptables/xtables-eb-translate.c
index f98c38eb1..0fe14d2d0db32 100644
--- a/iptables/xtables-eb-translate.c
+++ b/iptables/xtables-eb-translate.c
@@ -64,27 +64,6 @@ static int parse_rule_number(const char *rule)
return rule_nr;
}
-static const char *
-parse_target(const char *targetname)
-{
- const char *ptr;
-
- if (strlen(targetname) < 1
[iptables PATCH 2/2] ebtables-nft: Support user-defined chain policies
Legacy ebtables supports policies for user-defined chains - and what's
worse, they default to ACCEPT unlike anywhere else. So lack of support
for this braindead feature in ebtables-nft is actually a change of
behaviour which very likely affects all ebtables users out there.
The solution implemented here uses an implicit (and transparent) last
rule in all user-defined ebtables-nft chains with policy other than
RETURN. This rule is identified by an nft comment
"XTABLES_EB_INTERNAL_POLICY_RULE" (since commit ccf154d7420c0 ("xtables:
Don't use native nftables comments") nft comments are not used
otherwise).
To minimize interference with existing code, this policy rule is removed
from chains during cache population and the policy is saved in
NFTNL_CHAIN_POLICY attribute. When committing changes to the kernel,
nft_commit() traverses through the list of chains and (re-)creates
policy rules if required.
In ebtables-nft-restore, table flushes are problematic. To avoid weird
kernel error responses, introduce a custom 'table_flush' callback which
removes any pending policy rule add/remove jobs prior to creating the
NFT_COMPAT_TABLE_FLUSH one.
I've hidden all this mess behind checks for h->family, so hopefully
impact on {ip,ip6,arp}tables-nft should be negligible.
Signed-off-by: Phil Sutter
---
iptables/nft-bridge.c | 2 +-
iptables/nft.c| 215 +-
iptables/nft.h| 4 +
.../ebtables/0002-ebtables-save-restore_0 | 7 +
iptables/xtables-eb.c | 20 +-
iptables/xtables-restore.c| 23 +-
6 files changed, 249 insertions(+), 22 deletions(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 7c390dfa2a898..72c89987a07f2 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -358,7 +358,7 @@ static void nft_bridge_print_header(unsigned int format,
const char *chain,
bool basechain, uint32_t refs, uint32_t
entries)
{
printf("Bridge chain: %s, entries: %u, policy: %s\n",
- chain, entries, basechain ? pol : "RETURN");
+ chain, entries, pol);
}
static void print_matches_and_watchers(const struct iptables_command_state *cs,
diff --git a/iptables/nft.c b/iptables/nft.c
index 8d0d10177f5ed..f1e30b5c09fa1 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1322,9 +1322,73 @@ retry:
return ret;
}
+#define EBT_POLICY_COMMENT "XTABLES_EB_INTERNAL_POLICY_RULE"
+
+static bool nft_rule_is_policy_rule(struct nftnl_rule *r)
+{
+ const void *data;
+ char *comment;
+ uint32_t len;
+
+ if (!nftnl_rule_is_set(r, NFTNL_RULE_USERDATA))
+ return false;
+
+ data = nftnl_rule_get_data(r, NFTNL_RULE_USERDATA, &len);
+ comment = get_comment(data, len);
+ if (!comment || strcmp(comment, EBT_POLICY_COMMENT))
+ return false;
+
+ return true;
+}
+
+struct nftnl_rule_list_cb_data {
+ struct nft_handle *handle;
+ struct nftnl_chain *chain;
+};
+
+static int nft_convert_policy_rule(struct nft_handle *h,
+ struct nftnl_chain *c, struct nftnl_rule *r)
+{
+ struct nftnl_expr_iter *iter;
+ int verdict = NFT_RETURN;
+ struct nftnl_expr *expr;
+
+ iter = nftnl_expr_iter_create(r);
+ if (iter == NULL)
+ return 0;
+
+ expr = nftnl_expr_iter_next(iter);
+ while (expr != NULL) {
+ if (strcmp("immediate",
+ nftnl_expr_get_str(expr, NFTNL_EXPR_NAME))) {
+ expr = nftnl_expr_iter_next(iter);
+ continue;
+ }
+ verdict = nftnl_expr_get_u32(expr, NFTNL_EXPR_IMM_VERDICT);
+ break;
+ }
+ nftnl_expr_iter_destroy(iter);
+
+ switch (verdict) {
+ case NF_ACCEPT:
+ case NF_DROP:
+ nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, verdict);
+ break;
+ }
+
+ if (batch_rule_add(h, NFT_COMPAT_RULE_DELETE, r) < 0) {
+ printf("%s: failed to delete old policy rule\n", __func__);
+ return -1;
+ }
+
+ return 0;
+}
+
static int nftnl_rule_list_cb(const struct nlmsghdr *nlh, void *data)
{
- struct nftnl_chain *c = data;
+ struct nftnl_rule_list_cb_data *d = data;
+ struct nftnl_chain *c = d->chain;
+ struct nft_handle *h = d->handle;
struct nftnl_rule *r;
r = nftnl_rule_alloc();
@@ -1336,12 +1400,21 @@ static int nftnl_rule_list_cb(const struct nlmsghdr
*nlh, void *data)
return MNL_CB_OK;
}
- nftnl_chain_rule_add_tail(r, c);
+ if (h->family == NFPROTO_BRIDGE &&
+ nft_rule_is_policy_rule(r))
+
[iptables PATCH 1/2] xshared: Explicitly pass target to command_jump()
The use of global 'optarg' variable inside that function is a mess, but
most importantly it limits its applicability to input parsers. Fix this
by having it take the option argument as a parameter.
Signed-off-by: Phil Sutter
---
iptables/ip6tables.c| 2 +-
iptables/iptables.c | 2 +-
iptables/xshared.c | 4 ++--
iptables/xshared.h | 2 +-
iptables/xtables-arp.c | 2 +-
iptables/xtables-eb-translate.c | 2 +-
iptables/xtables-eb.c | 2 +-
iptables/xtables.c | 2 +-
8 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index fe089de4c85d7..050afa9a36458 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1441,7 +1441,7 @@ int do_command6(int argc, char *argv[], char **table,
case 'j':
set_option(&cs.options, OPT_JUMP, &cs.fw6.ipv6.invflags,
cs.invert);
- command_jump(&cs);
+ command_jump(&cs, optarg);
break;
diff --git a/iptables/iptables.c b/iptables/iptables.c
index f8041f56ce70d..38c4bfe8ecf5c 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1421,7 +1421,7 @@ int do_command4(int argc, char *argv[], char **table,
case 'j':
set_option(&cs.options, OPT_JUMP, &cs.fw.ip.invflags,
cs.invert);
- command_jump(&cs);
+ command_jump(&cs, optarg);
break;
diff --git a/iptables/xshared.c b/iptables/xshared.c
index b16f5fa68e569..fb186fb1ac657 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -653,12 +653,12 @@ const char *xt_parse_target(const char *targetname)
return targetname;
}
-void command_jump(struct iptables_command_state *cs)
+void command_jump(struct iptables_command_state *cs, const char *jumpto)
{
struct option *opts = xt_params->opts;
size_t size;
- cs->jumpto = xt_parse_target(optarg);
+ cs->jumpto = xt_parse_target(jumpto);
/* TRY_LOAD (may be chain name) */
cs->target = xtables_find_target(cs->jumpto, XTF_TRY_LOAD);
diff --git a/iptables/xshared.h b/iptables/xshared.h
index db499f29236ed..fd1f96bad1b98 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -176,6 +176,6 @@ void print_ifaces(const char *iniface, const char
*outiface, uint8_t invflags,
void command_match(struct iptables_command_state *cs);
const char *xt_parse_target(const char *targetname);
-void command_jump(struct iptables_command_state *cs);
+void command_jump(struct iptables_command_state *cs, const char *jumpto);
#endif /* IPTABLES_XSHARED_H */
diff --git a/iptables/xtables-arp.c b/iptables/xtables-arp.c
index 4b663775c5bee..d3cb9df823feb 100644
--- a/iptables/xtables-arp.c
+++ b/iptables/xtables-arp.c
@@ -1161,7 +1161,7 @@ int do_commandarp(struct nft_handle *h, int argc, char
*argv[], char **table,
case 'j':
set_option(&options, OPT_JUMP, &cs.arp.arp.invflags,
invert);
- command_jump(&cs);
+ command_jump(&cs, optarg);
break;
case 'i':
diff --git a/iptables/xtables-eb-translate.c b/iptables/xtables-eb-translate.c
index 0fe14d2d0db32..96b2730fa97ed 100644
--- a/iptables/xtables-eb-translate.c
+++ b/iptables/xtables-eb-translate.c
@@ -390,7 +390,7 @@ print_zero:
break;
} else if (c == 'j') {
ebt_check_option2(&flags, OPT_JUMP);
- command_jump(&cs);
+ command_jump(&cs, optarg);
break;
} else if (c == 's') {
ebt_check_option2(&flags, OPT_SOURCE);
diff --git a/iptables/xtables-eb.c b/iptables/xtables-eb.c
index 75d43963d5ef8..4d2e6f683bebb 100644
--- a/iptables/xtables-eb.c
+++ b/iptables/xtables-eb.c
@@ -1011,7 +1011,7 @@ print_zero:
} else if (c == 'j') {
ebt_check_option2(&flags, OPT_JUMP);
if (strcmp(optarg, "CONTINUE") != 0) {
- command_jump(&cs);
+ command_jump(&cs, optarg);
}
break;
} else if (c == 's') {
diff --git a/iptables/xtables.c b/iptables/xtables.c
index eaa9fedeb03bb..1d777554076d7 100644
--- a/iptables/xtables.c
+++ b/iptables/xta
[iptables PATCH 0/2] ebtables-nft: Support user-defined chain policies
All the (literally) gory details are in patch 2, patch 1 is a dependency for the second one but sensible by itself. Phil Sutter (2): xshared: Explicitly pass target to command_jump() ebtables-nft: Support user-defined chain policies iptables/ip6tables.c | 2 +- iptables/iptables.c | 2 +- iptables/nft-bridge.c | 2 +- iptables/nft.c| 215 +- iptables/nft.h| 4 + .../ebtables/0002-ebtables-save-restore_0 | 7 + iptables/xshared.c| 4 +- iptables/xshared.h| 2 +- iptables/xtables-arp.c| 2 +- iptables/xtables-eb-translate.c | 2 +- iptables/xtables-eb.c | 22 +- iptables/xtables-restore.c| 23 +- iptables/xtables.c| 2 +- 13 files changed, 258 insertions(+), 31 deletions(-) -- 2.20.1
[iptables PATCH v2] Revert "ebtables: use extrapositioned negation consistently"
This reverts commit 5f508b76a0cebaf91965ffa678089222e2d47964.
While attempts at unifying syntax between arp-, eb- and iptables-nft
increase the opportunity for more code-sharing, they are problematic
when it comes to compatibility. Accepting the old syntax on input helps,
but due to the fact that neither arptables nor ebtables support --check
command we must expect for users to test existence of a rule by
comparing input with output. If that happens in a script, deviating from
the old syntax in output has a high chance of breaking it.
Therefore revert Florian's patch changing inversion character position
in output and review the old code for consistency - the only thing
changed on top of the actual revert is ebtables' own copy of
print_iface() to make it adhere to the intrapositioned negation scheme
used throughout ebtables.
Added extension tests by the reverted commit have been kept.
Signed-off-by: Phil Sutter
---
Changes since v1:
- Keep added extension tests, only adjust to intrapositioned negation if
required.
---
extensions/libebt_802_3.c| 4 ++--
extensions/libebt_802_3.t| 2 +-
extensions/libebt_arp.c | 14 +++---
extensions/libebt_arp.t | 8
extensions/libebt_ip.c | 16
extensions/libebt_ip.t | 6 +++---
extensions/libebt_ip6.c | 14 +++---
extensions/libebt_ip6.t | 6 +++---
extensions/libebt_mark_m.c | 2 +-
extensions/libebt_mark_m.t | 4 ++--
extensions/libebt_pkttype.c | 5 +
extensions/libebt_pkttype.t | 13 +++--
extensions/libebt_standard.t | 4 ++--
extensions/libebt_stp.c | 5 ++---
extensions/libebt_vlan.c | 13 -
extensions/libebt_vlan.t | 10 +-
iptables/nft-bridge.c| 6 +++---
17 files changed, 62 insertions(+), 70 deletions(-)
diff --git a/extensions/libebt_802_3.c b/extensions/libebt_802_3.c
index 9e91d05262591..f05d02ead5a4a 100644
--- a/extensions/libebt_802_3.c
+++ b/extensions/libebt_802_3.c
@@ -98,15 +98,15 @@ static void br802_3_print(const void *ip, const struct
xt_entry_match *match,
struct ebt_802_3_info *info = (struct ebt_802_3_info *)match->data;
if (info->bitmask & EBT_802_3_SAP) {
+ printf("--802_3-sap ");
if (info->invflags & EBT_802_3_SAP)
printf("! ");
- printf("--802_3-sap ");
printf("0x%.2x ", info->sap);
}
if (info->bitmask & EBT_802_3_TYPE) {
+ printf("--802_3-type ");
if (info->invflags & EBT_802_3_TYPE)
printf("! ");
- printf("--802_3-type ");
printf("0x%.4x ", ntohs(info->type));
}
}
diff --git a/extensions/libebt_802_3.t b/extensions/libebt_802_3.t
index 61081bd6983a8..ddfb2f0a72baf 100644
--- a/extensions/libebt_802_3.t
+++ b/extensions/libebt_802_3.t
@@ -1,3 +1,3 @@
:INPUT,FORWARD,OUTPUT
-! --802_3-sap 0x0a -j CONTINUE;=;OK
+--802_3-sap ! 0x0a -j CONTINUE;=;OK
--802_3-type 0x000a -j RETURN;=;OK
diff --git a/extensions/libebt_arp.c b/extensions/libebt_arp.c
index c1b0ab1db0cf1..a062b7e7e5864 100644
--- a/extensions/libebt_arp.c
+++ b/extensions/libebt_arp.c
@@ -338,51 +338,51 @@ static void brarp_print(const void *ip, const struct
xt_entry_match *match, int
if (arpinfo->bitmask & EBT_ARP_OPCODE) {
int opcode = ntohs(arpinfo->opcode);
+ printf("--arp-op ");
if (arpinfo->invflags & EBT_ARP_OPCODE)
printf("! ");
- printf("--arp-op ");
if (opcode > 0 && opcode <= ARRAY_SIZE(opcodes))
printf("%s ", opcodes[opcode - 1]);
else
printf("%d ", opcode);
}
if (arpinfo->bitmask & EBT_ARP_HTYPE) {
+ printf("--arp-htype ");
if (arpinfo->invflags & EBT_ARP_HTYPE)
printf("! ");
- printf("--arp-htype ");
printf("%d ", ntohs(arpinfo->htype));
}
if (arpinfo->bitmask & EBT_ARP_PTYPE) {
+ printf("--arp-ptype ");
if (arpinfo->invflags & EBT_ARP_PTYPE)
printf("! ");
- printf("--arp-ptype ");
printf("0x%x ", ntohs(arpinfo->ptype));
}
if (arpinfo->bitmask & EBT_ARP_SRC_IP) {
+ printf("--arp-ip-src ");
if (arpinfo->invflags & EBT_ARP_SRC_IP)
printf("! ");
- printf("--arp-ip-src ");
[iptables PATCH] nft: Eliminate dead code in __nft_rule_list
If passed a rulenum > 0, the function uses nftnl_rule_lookup_byindex()
and returns early. Negative rulenum values are not supposed to happen,
so the remaining code which iterates over the full list of rules does
not need to respect rulenum anymore.
Fixes: 039b048965210 ("nft: Make use of nftnl_rule_lookup_byindex()")
Signed-off-by: Phil Sutter
---
iptables/nft.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index 8d0d10177f5ed..cafa82a420856 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2231,18 +2231,7 @@ __nft_rule_list(struct nft_handle *h, struct nftnl_chain
*c,
r = nftnl_rule_iter_next(iter);
while (r != NULL) {
- rule_ctr++;
-
- if (rulenum > 0 && rule_ctr != rulenum) {
- /* List by rule number case */
- goto next;
- }
-
- cb(r, rule_ctr, format);
- if (rulenum > 0)
- break;
-
-next:
+ cb(r, ++rule_ctr, format);
r = nftnl_rule_iter_next(iter);
}
--
2.20.1
Re: [iptables PATCH 2/2] ebtables-nft: Support user-defined chain policies
On Thu, Feb 07, 2019 at 04:29:52PM +0100, Florian Westphal wrote:
> Phil Sutter wrote:
> > Legacy ebtables supports policies for user-defined chains - and what's
> > worse, they default to ACCEPT unlike anywhere else. So lack of support
> > for this braindead feature in ebtables-nft is actually a change of
> > behaviour which very likely affects all ebtables users out there.
> >
> > The solution implemented here uses an implicit (and transparent) last
> > rule in all user-defined ebtables-nft chains with policy other than
> > RETURN. This rule is identified by an nft comment
> > "XTABLES_EB_INTERNAL_POLICY_RULE" (since commit ccf154d7420c0 ("xtables:
> > Don't use native nftables comments") nft comments are not used
> > otherwise).
> >
> > To minimize interference with existing code, this policy rule is removed
> > from chains during cache population and the policy is saved in
> > NFTNL_CHAIN_POLICY attribute. When committing changes to the kernel,
> > nft_commit() traverses through the list of chains and (re-)creates
> > policy rules if required.
> >
> > In ebtables-nft-restore, table flushes are problematic. To avoid weird
> > kernel error responses, introduce a custom 'table_flush' callback which
> > removes any pending policy rule add/remove jobs prior to creating the
> > NFT_COMPAT_TABLE_FLUSH one.
> >
> > I've hidden all this mess behind checks for h->family, so hopefully
> > impact on {ip,ip6,arp}tables-nft should be negligible.
> >
> > Signed-off-by: Phil Sutter
> > ---
> > iptables/nft-bridge.c | 2 +-
> > iptables/nft.c| 215 +-
> > iptables/nft.h| 4 +
> > .../ebtables/0002-ebtables-save-restore_0 | 7 +
> > iptables/xtables-eb.c | 20 +-
> > iptables/xtables-restore.c| 23 +-
> > 6 files changed, 249 insertions(+), 22 deletions(-)
> >
> > diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
> > index 7c390dfa2a898..72c89987a07f2 100644
> > --- a/iptables/nft-bridge.c
> > +++ b/iptables/nft-bridge.c
> > @@ -358,7 +358,7 @@ static void nft_bridge_print_header(unsigned int
> > format, const char *chain,
> > bool basechain, uint32_t refs, uint32_t
> > entries)
> > {
> > printf("Bridge chain: %s, entries: %u, policy: %s\n",
> > - chain, entries, basechain ? pol : "RETURN");
> > + chain, entries, pol);
>
> This causes
>
> Bridge chain: FOOBAR, entries: 0, policy: (null)
>
> after "ebtables -P FOOBAR RETURN".
> Reverting this hunk shows "RETURN" as expected.
Oh. I missed that case. Of course that's not a real solution, otherwise
user-defined chain policies will always show up as RETURN. I'll find a
real fix.
> Doing "ebtables -P FOOBAR DROP" sets a hidden rule, but its still shown
> as ACCEPT. I'll debiug this further.
Not happening here, not sure why.
> plain nft shows:
> ether type 0x counter packets 0 bytes 0 drop comment
> "XTABLES_EB_INTERNAL_POLICY_RULE"
>
> This "ether type" is bonkers as well, it should not be there.
> Looks like ebt_add_policy_rule needs to set "cs.eb.bitmask = EBT_NOPROTO".
Yes, indeed. And since EBT_NOPROTO is defined in nft-bridge.h, I have to
include that in nft.c. Initially I wanted to avoid that. But if it's
needed anyway, I could move all introduced functions into nft-bridge.c.
The only problem with that is nft_rule_new() being static, but I could
either change that or duplicate its content since it's quite small.
> > + while (expr != NULL) {
> > + if (strcmp("immediate",
> > + nftnl_expr_get_str(expr, NFTNL_EXPR_NAME))) {
> > + expr = nftnl_expr_iter_next(iter);
> > + continue;
> > + }
>
> The imm should come first, in case there is a different expr this isn't
> an "implicit policy" (because its not unconditional verdict).
Good point! No need to traverse the whole expr list.
> Should perhaps also check nftnl_expr_is_set(expr, NFTNL_EXPR_IMM_VERDICT)
> so we really don't fall for random immediates.
Will do, thanks!
Cheers, Phil
Re: [iptables PATCH 2/2] ebtables-nft: Support user-defined chain policies
On Thu, Feb 07, 2019 at 05:00:36PM +0100, Florian Westphal wrote:
> Phil Sutter wrote:
> > > after "ebtables -P FOOBAR RETURN".
> > > Reverting this hunk shows "RETURN" as expected.
> >
> > Oh. I missed that case. Of course that's not a real solution, otherwise
> > user-defined chain policies will always show up as RETURN. I'll find a
> > real fix.
>
> Disregard that, I was low on caffeeine.
8)
> Attached patch makes things work much better for me (no need to take
> this, consider it a bit more verbose bug report).
Thanks!
> Not perfect, and one major issue remains.
>
> When someone munges the chain using native nft, ebtables-nft shows:
>
> Bridge chain: FOOBAR, entries: 1, policy: DROP
> -d 01:02:03:04:05:06 -j CONTINUE
>
> What it should show instead:
> Bridge chain: FOOBAR, entries: 1, policy: RETURN
> -j DROP
> -d 01:02:03:04:05:06 -j CONTINUE
>
> (because thats whats the actual state -- the last rule is unreachable).
Hmm. Yes, that's ugly. Also, if you perform a change to the ruleset in
that state (no matter what, e.g. just create another chain or add a rule
somewhere else), the policy rule will be moved to the end. Not sure how
we could handle this.
> Not a huge deal if we're only interested in "ebtables-nft only".
Maybe that's the only viable option? The only alternative I could think
of at this point would be to treat the whole chain as incompatible, but
that's probably not exactly a better way to handle it.
> diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
> --- a/iptables/nft-bridge.c
> +++ b/iptables/nft-bridge.c
> @@ -358,7 +358,7 @@ static void nft_bridge_print_header(unsigned int format,
> const char *chain,
> bool basechain, uint32_t refs, uint32_t
> entries)
> {
> printf("Bridge chain: %s, entries: %u, policy: %s\n",
> -chain, entries, pol);
> +chain, entries, pol ? pol : "RETURN");
> }
>
That's what I've come up with, too.
> static void print_matches_and_watchers(const struct iptables_command_state
> *cs,
> diff --git a/iptables/nft.c b/iptables/nft.c
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
[...]
> @@ -1400,9 +1414,8 @@ static int nftnl_rule_list_cb(const struct nlmsghdr
> *nlh, void *data)
> return MNL_CB_OK;
> }
>
> - if (h->family == NFPROTO_BRIDGE &&
> - nft_rule_is_policy_rule(r))
> - nft_convert_policy_rule(h, c, r);
> + if (h->family == NFPROTO_BRIDGE)
> + nft_bridge_chain_rule_add_tail(h, r, c);
> else
> nftnl_chain_rule_add_tail(r, c);
>
Ah, that's nice!
> @@ -2758,7 +2771,9 @@ static int nft_action(struct nft_handle *h, int action)
> static int ebt_add_policy_rule(struct nftnl_chain *c, void *data)
> {
> uint32_t policy = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
> - struct iptables_command_state cs = {};
> + struct iptables_command_state cs = {
> + .eb.bitmask = 0x2,
> + };
> struct nftnl_udata_buf *udata;
> struct nft_handle *h = data;
> struct nftnl_rule *r;
You're not really suggesting to hard-code EBT_NOPROTO value here, right?
Thanks, Phil
[iptables PATCH v2] ebtables-nft: Support user-defined chain policies
Legacy ebtables supports policies for user-defined chains - and what's
worse, they default to ACCEPT unlike anywhere else. So lack of support
for this braindead feature in ebtables-nft is actually a change of
behaviour which very likely affects all ebtables users out there.
The solution implemented here uses an implicit (and transparent) last
rule in all user-defined ebtables-nft chains with policy other than
RETURN. This rule is identified by an nft comment
"XTABLES_EB_INTERNAL_POLICY_RULE" (since commit ccf154d7420c0 ("xtables:
Don't use native nftables comments") nft comments are not used
otherwise).
To minimize interference with existing code, this policy rule is removed
from chains during cache population and the policy is saved in
NFTNL_CHAIN_POLICY attribute. When committing changes to the kernel,
nft_commit() traverses through the list of chains and (re-)creates
policy rules if required.
In ebtables-nft-restore, table flushes are problematic. To avoid weird
kernel error responses, introduce a custom 'table_flush' callback which
removes any pending policy rule add/remove jobs prior to creating the
NFT_COMPAT_TABLE_FLUSH one.
I've hidden all this mess behind checks for h->family, so hopefully
impact on {ip,ip6,arp}tables-nft should be negligible.
Signed-off-by: Phil Sutter
---
Changes since v1:
- Set EBT_NOPROTO in policy rules to avoid generating spurious ether
type match.
- Fix RETURN policy printing for user-defined ebtables chains.
- Switch from a per-rule approach to chain postprocessing after rules
have been fetched. This way only the last rule is considered as policy
rule, so if a user appended a rule to that chain using 'nft' command,
chain will fall back to RETURN policy and the former policy rule stays
in place.
- Introduce helper nft_chain_last_rule() to aid in the above.
- Improve policy rule parsing as suggested by Florian.
- Introduce nft_bridge_commit_prepare() to outsource bridge-specific
actions at nft_commit() time.
Note that I didn't drop the policy comment. It is required by
nft_abort_policy_rule() to identify suitable rule append/delete jobs.
This is strictly not necessary, the function runs only prior to a table
flush - so in theory one could just drop any rule append/delete jobs
from the batch for that particular table. After that, policy rules would
look just like regular ones also in 'nft' output. The downside is that a
user could accidentally set the chain's policy this way since
| ebtables-nft -A foo -j ACCEPT
would yield identical results as:
| ebtables-nft -P foo ACCEPT
This seems unproblematic, but consider a stupid example:
| ebtables-nft -N foo -P RETURN
| ebtables-nft -A foo -j ACCEPT
| ebtables-nft -A foo -j DROP
So after the second command, chain foo has one rule which accepts
everything. During the third command, that accept rule is turned into
the policy and after it we have a chain with accept policy but a single
rule which drops everything. That is at least a functional deviation
from legacy ebtables, and a confusing one as well. :)
---
iptables/nft-bridge.c | 2 +-
iptables/nft.c| 227 +-
iptables/nft.h| 4 +
.../ebtables/0002-ebtables-save-restore_0 | 7 +
iptables/xtables-eb.c | 20 +-
iptables/xtables-restore.c| 23 +-
6 files changed, 264 insertions(+), 19 deletions(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 848ca7934e354..ddfbee165da93 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -358,7 +358,7 @@ static void nft_bridge_print_header(unsigned int format,
const char *chain,
bool basechain, uint32_t refs, uint32_t
entries)
{
printf("Bridge chain: %s, entries: %u, policy: %s\n",
- chain, entries, basechain ? pol : "RETURN");
+ chain, entries, pol ?: "RETURN");
}
static void print_matches_and_watchers(const struct iptables_command_state *cs,
diff --git a/iptables/nft.c b/iptables/nft.c
index 8d0d10177f5ed..40d9f88e1c7d1 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -55,6 +55,7 @@
#include "nft.h"
#include "xshared.h" /* proto_to_name */
#include "nft-shared.h"
+#include "nft-bridge.h" /* EBT_NOPROTO */
#include "xtables-config-parser.h"
static void *nft_fn;
@@ -1322,6 +1323,86 @@ retry:
return ret;
}
+#define EBT_POLICY_COMMENT "XTABLES_EB_INTERNAL_POLICY_RULE"
+
+static bool nft_rule_is_policy_rule(struct nftnl_rule *r)
+{
+ const void *data;
+ char *comment;
+ uint32_t len;
+
+ if (!nftnl_rule_is_set(r, NFTNL_RULE_USERDATA))
+ return false;
+
+ data = nftnl_rule_get_data(r, NFTNL_RULE_USERDATA, &len);
+ commen
Re: [iptables PATCH 2/2] ebtables-nft: Support user-defined chain policies
On Thu, Feb 07, 2019 at 07:29:10PM +0100, Pablo Neira Ayuso wrote: > On Thu, Feb 07, 2019 at 05:48:53PM +0100, Florian Westphal wrote: > > Phil Sutter wrote: > > > > What it should show instead: > > > > Bridge chain: FOOBAR, entries: 1, policy: RETURN > > > > -j DROP > > > > -d 01:02:03:04:05:06 -j CONTINUE > > > > > > > > (because thats whats the actual state -- the last rule is unreachable). > > > > > > Hmm. Yes, that's ugly. Also, if you perform a change to the ruleset in > > > that state (no matter what, e.g. just create another chain or add a rule > > > somewhere else), the policy rule will be moved to the end. Not sure how > > > we could handle this. > > > > I think in that case ebtables-nft should make policy be 'RETURN', i.e., > > not re-add a new policy chain. > > > > I think we could even avoid the 'user comment' and just examine the last > > rule in the chain -- check if its unconditional DROP/ACCEPT and then > > handle that as the 'policy'. > > We can probably add UDATA_TYPE_EBTABLES_POLICY, so we don't need to > guess if this is an autogenerated policy rule at the end of non-base > chain. Just search for the rule with this flag. For nft this > autogenerated rule will be transparent. I just tried. There is a minor issue with nft_rule_to_iptables_command_state(): It assumes that if NFTNL_RULE_USERDATA is present, it is a comment. If I either drop that code (it is there only for backwards compatibility) or make it aware that NFTNL_RULE_USERDATA not necessarily contains a comment, things work fine. What do you think? Should I send a v3? Thanks, Phil
Re: [iptables PATCH 2/2] ebtables-nft: Support user-defined chain policies
On Thu, Feb 07, 2019 at 09:24:09PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Feb 07, 2019 at 09:00:50PM +0100, Phil Sutter wrote:
> > On Thu, Feb 07, 2019 at 07:29:10PM +0100, Pablo Neira Ayuso wrote:
> > > On Thu, Feb 07, 2019 at 05:48:53PM +0100, Florian Westphal wrote:
> > > > Phil Sutter wrote:
> > > > > > What it should show instead:
> > > > > > Bridge chain: FOOBAR, entries: 1, policy: RETURN
> > > > > > -j DROP
> > > > > > -d 01:02:03:04:05:06 -j CONTINUE
> > > > > >
> > > > > > (because thats whats the actual state -- the last rule is
> > > > > > unreachable).
> > > > >
> > > > > Hmm. Yes, that's ugly. Also, if you perform a change to the ruleset in
> > > > > that state (no matter what, e.g. just create another chain or add a
> > > > > rule
> > > > > somewhere else), the policy rule will be moved to the end. Not sure
> > > > > how
> > > > > we could handle this.
> > > >
> > > > I think in that case ebtables-nft should make policy be 'RETURN', i.e.,
> > > > not re-add a new policy chain.
> > > >
> > > > I think we could even avoid the 'user comment' and just examine the last
> > > > rule in the chain -- check if its unconditional DROP/ACCEPT and then
> > > > handle that as the 'policy'.
> > >
> > > We can probably add UDATA_TYPE_EBTABLES_POLICY, so we don't need to
> > > guess if this is an autogenerated policy rule at the end of non-base
> > > chain. Just search for the rule with this flag. For nft this
> > > autogenerated rule will be transparent.
> >
> > I just tried. There is a minor issue with
> > nft_rule_to_iptables_command_state(): It assumes that if
> > NFTNL_RULE_USERDATA is present, it is a comment. If I either drop that
> > code (it is there only for backwards compatibility) or make it aware
> > that NFTNL_RULE_USERDATA not necessarily contains a comment, things work
> > fine.
> >
> > What do you think? Should I send a v3?
>
> You could rename get_comment() to get_userdata() which returns an
> ipt_userdata structure that looks like:
>
> struct ipt_userdata {
> const char *comment;
> boolis_ebtables_policy;
> };
>
> that you can pass to get_userdata() to fetch things. You can probably
> store this userdata in the command_state structure, so we don't need
> to parse userdata over and over again from different spots.
>
> But this is just a suggestion.
It is a nice idea, but probably not worth it, userdata is parsed in just
two spots. When populating the iptables_command_state, a "comment" match
is allocated to hold the data. When used for ebtables policy rule, code
operates on struct nftnl_rule directly, so no point in keeping things in
iptables_command_state, either.
I'll send a v3 making use of a custom userdata attribute.
Thanks, Phil
[iptables PATCH v3 2/3] nft: Introduce UDATA_TYPE_EBTABLES_POLICY
This will be used later to identify ebtables user-defined chain policy
rules.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/iptables/nft.c b/iptables/nft.c
index 8d0d10177f5ed..4010ccd51d5aa 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1115,6 +1115,7 @@ int add_counters(struct nftnl_rule *r, uint64_t packets,
uint64_t bytes)
enum udata_type {
UDATA_TYPE_COMMENT,
+ UDATA_TYPE_EBTABLES_POLICY,
__UDATA_TYPE_MAX,
};
#define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
@@ -1131,6 +1132,8 @@ static int parse_udata_cb(const struct nftnl_udata *attr,
void *data)
if (value[len - 1] != '\0')
return -1;
break;
+ case UDATA_TYPE_EBTABLES_POLICY:
+ break;
default:
return 0;
}
--
2.20.1
[iptables PATCH v3 3/3] ebtables-nft: Support user-defined chain policies
Legacy ebtables supports policies for user-defined chains - and what's
worse, they default to ACCEPT unlike anywhere else. So lack of support
for this braindead feature in ebtables-nft is actually a change of
behaviour which very likely affects all ebtables users out there.
The solution implemented here uses an implicit (and transparent) last
rule in all user-defined ebtables-nft chains with policy other than
RETURN. This rule is identified by an nft comment
"XTABLES_EB_INTERNAL_POLICY_RULE" (since commit ccf154d7420c0 ("xtables:
Don't use native nftables comments") nft comments are not used
otherwise).
To minimize interference with existing code, this policy rule is removed
from chains during cache population and the policy is saved in
NFTNL_CHAIN_POLICY attribute. When committing changes to the kernel,
nft_commit() traverses through the list of chains and (re-)creates
policy rules if required.
In ebtables-nft-restore, table flushes are problematic. To avoid weird
kernel error responses, introduce a custom 'table_flush' callback which
removes any pending policy rule add/remove jobs prior to creating the
NFT_COMPAT_TABLE_FLUSH one.
I've hidden all this mess behind checks for h->family, so hopefully
impact on {ip,ip6,arp}tables-nft should be negligible.
Signed-off-by: Phil Sutter
---
Changes since v2:
- Use userdata type UDATA_TYPE_EBTABLES_POLICY to indicate the rule
being a policy rule.
Changes since v1:
- Set EBT_NOPROTO in policy rules to avoid generating spurious ether
type match.
- Fix RETURN policy printing for user-defined ebtables chains.
- Switch from a per-rule approach to chain postprocessing after rules
have been fetched. This way only the last rule is considered as policy
rule, so if a user appended a rule to that chain using 'nft' command,
chain will fall back to RETURN policy and the former policy rule stays
in place.
- Introduce helper nft_chain_last_rule() to aid in the above.
- Improve policy rule parsing as suggested by Florian.
- Introduce nft_bridge_commit_prepare() to outsource bridge-specific
actions at nft_commit() time.
---
iptables/nft-bridge.c | 2 +-
iptables/nft.c| 228 +-
iptables/nft.h| 4 +
.../ebtables/0002-ebtables-save-restore_0 | 7 +
iptables/xtables-eb.c | 20 +-
iptables/xtables-restore.c| 23 +-
6 files changed, 265 insertions(+), 19 deletions(-)
diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
index 848ca7934e354..ddfbee165da93 100644
--- a/iptables/nft-bridge.c
+++ b/iptables/nft-bridge.c
@@ -358,7 +358,7 @@ static void nft_bridge_print_header(unsigned int format,
const char *chain,
bool basechain, uint32_t refs, uint32_t
entries)
{
printf("Bridge chain: %s, entries: %u, policy: %s\n",
- chain, entries, basechain ? pol : "RETURN");
+ chain, entries, pol ?: "RETURN");
}
static void print_matches_and_watchers(const struct iptables_command_state *cs,
diff --git a/iptables/nft.c b/iptables/nft.c
index 4010ccd51d5aa..2d527358cc7f2 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -55,6 +55,7 @@
#include "nft.h"
#include "xshared.h" /* proto_to_name */
#include "nft-shared.h"
+#include "nft-bridge.h" /* EBT_NOPROTO */
#include "xtables-config-parser.h"
static void *nft_fn;
@@ -1325,6 +1326,87 @@ retry:
return ret;
}
+static bool nft_rule_is_policy_rule(struct nftnl_rule *r)
+{
+ const struct nftnl_udata *tb[UDATA_TYPE_MAX + 1] = {};
+ const void *data;
+ uint32_t len;
+
+ if (!nftnl_rule_is_set(r, NFTNL_RULE_USERDATA))
+ return false;
+
+ data = nftnl_rule_get_data(r, NFTNL_RULE_USERDATA, &len);
+ if (nftnl_udata_parse(data, len, parse_udata_cb, tb) < 0)
+ return NULL;
+
+ if (!tb[UDATA_TYPE_EBTABLES_POLICY] ||
+ nftnl_udata_get_u32(tb[UDATA_TYPE_EBTABLES_POLICY]) != 1)
+ return false;
+
+ return true;
+}
+
+static struct nftnl_rule *nft_chain_last_rule(struct nftnl_chain *c)
+{
+ struct nftnl_rule *r = NULL, *last;
+ struct nftnl_rule_iter *iter;
+
+ iter = nftnl_rule_iter_create(c);
+ if (!iter)
+ return NULL;
+
+ do {
+ last = r;
+ r = nftnl_rule_iter_next(iter);
+ } while (r);
+ nftnl_rule_iter_destroy(iter);
+
+ return last;
+}
+
+static void nft_bridge_chain_postprocess(struct nft_handle *h,
+struct nftnl_chain *c)
+{
+ struct nftnl_rule *last = nft_chain_last_rule(c);
+ struct nftnl_expr_iter *iter;
+ struct nftnl_expr *expr;
+ int verdict;
+
+ if (!last || !nft_rule_is_po
[iptables PATCH v3 1/3] nft: Don't assume NFTNL_RULE_USERDATA holds a comment
If this rule attribute is present but does not contain a comment,
get_comment() returns NULL which is then fed into strncpy() causing a
crash.
Signed-off-by: Phil Sutter
---
iptables/nft-shared.c | 39 ++-
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index a72d414d78111..1c09277d85fb5 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -639,25 +639,30 @@ void nft_rule_to_iptables_command_state(const struct
nftnl_rule *r,
if (nftnl_rule_is_set(r, NFTNL_RULE_USERDATA)) {
const void *data;
uint32_t len, size;
- struct xtables_match *match;
- struct xt_entry_match *m;
+ const char *comment;
data = nftnl_rule_get_data(r, NFTNL_RULE_USERDATA, &len);
- match = xtables_find_match("comment", XTF_TRY_LOAD,
- &cs->matches);
- if (match == NULL)
- return;
-
- size = XT_ALIGN(sizeof(struct xt_entry_match)) + match->size;
- m = xtables_calloc(1, size);
-
- strncpy((char *)m->data, get_comment(data, len),
- match->size - 1);
- m->u.match_size = size;
- m->u.user.revision = 0;
- strcpy(m->u.user.name, match->name);
-
- match->m = m;
+ comment = get_comment(data, len);
+ if (comment) {
+ struct xtables_match *match;
+ struct xt_entry_match *m;
+
+ match = xtables_find_match("comment", XTF_TRY_LOAD,
+ &cs->matches);
+ if (match == NULL)
+ return;
+
+ size = XT_ALIGN(sizeof(struct xt_entry_match))
+ + match->size;
+ m = xtables_calloc(1, size);
+
+ strncpy((char *)m->data, comment, match->size - 1);
+ m->u.match_size = size;
+ m->u.user.revision = 0;
+ strcpy(m->u.user.name, match->name);
+
+ match->m = m;
+ }
}
if (cs->target != NULL) {
--
2.20.1
[iptables PATCH] xtables-save: Fix table not found error message
First of all, this error message should not appear on stdout, otherwise
it may end in dump files. Next, with completely empty ruleset, even
valid table names cause errors. To avoid this, continue operation if the
not found table is a builtin one.
Signed-off-by: Phil Sutter
---
iptables/xtables-save.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/iptables/xtables-save.c b/iptables/xtables-save.c
index 414a864b6196b..87ebb913f33b7 100644
--- a/iptables/xtables-save.c
+++ b/iptables/xtables-save.c
@@ -105,8 +105,9 @@ do_output(struct nft_handle *h, const char *tablename, bool
counters)
return !!ret;
}
- if (!nft_table_find(h, tablename)) {
- printf("Table `%s' does not exist\n", tablename);
+ if (!nft_table_find(h, tablename) &&
+ !nft_table_builtin_find(h, tablename)) {
+ fprintf(stderr, "Table `%s' does not exist\n", tablename);
return 1;
}
--
2.20.1
Re: [iptables PATCH v3 2/3] nft: Introduce UDATA_TYPE_EBTABLES_POLICY
On Fri, Feb 08, 2019 at 03:24:24PM +0100, Florian Westphal wrote:
> Phil Sutter wrote:
> > This will be used later to identify ebtables user-defined chain policy
> > rules.
> >
> > Signed-off-by: Phil Sutter
> > ---
> > iptables/nft.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/iptables/nft.c b/iptables/nft.c
> > index 8d0d10177f5ed..4010ccd51d5aa 100644
> > --- a/iptables/nft.c
> > +++ b/iptables/nft.c
> > @@ -1115,6 +1115,7 @@ int add_counters(struct nftnl_rule *r, uint64_t
> > packets, uint64_t bytes)
> >
> > enum udata_type {
> > UDATA_TYPE_COMMENT,
> > + UDATA_TYPE_EBTABLES_POLICY,
>
> Pablo, do you see any problems with this?
> I'm a bit concerned native nft could clash with this.
Short-term best would be to extend enum udata_type in nftables'
include/rule.h by this type to avoid accidental reuse. Mid-term we
should move all the udata_type definitions into libnftnl along with
getters/setters to centralize management.
Cheers, Phil
Re: [PATCH libnftnl] udata: add NFTNL_UDATA_* definitions
On Fri, Feb 08, 2019 at 05:15:24PM +0100, Pablo Neira Ayuso wrote: > Place them in the library, so iptables and nftables do not need to > redefine them. > > Signed-off-by: Pablo Neira Ayuso > --- > No need to use these definitions right now. But let's place them here > already so we don't forget. What about udata_set_type and udata_set_elem_type? They should move to libnftnl as well, right? Thanks, Phil
Re: [PATCH libnftnl,v3] udata: add NFTNL_UDATA_* definitions
On Fri, Feb 08, 2019 at 05:52:17PM +0100, Pablo Neira Ayuso wrote: > Place them in the library, so iptables and nftables do not need to > redefine them. > > Signed-off-by: Pablo Neira Ayuso Acked-by: Phil Sutter
[conntrack-tools PATCH] conntrackd: helpers: dhcpv6: Fix potential array overrun
The value dhcpv6_msg_type points at is used as index to dhcpv6_timeouts
array, so upper boundary check has to treat a value of
ARRAY_SIZE(dhcpv6_timeouts) as invalid.
Fixes: 36118bfc4901b ("conntrackd: helpers: add DHCPv6 helper")
Signed-off-by: Phil Sutter
---
src/helpers/dhcpv6.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/helpers/dhcpv6.c b/src/helpers/dhcpv6.c
index 73632ec181a95..f87b6cebfe157 100644
--- a/src/helpers/dhcpv6.c
+++ b/src/helpers/dhcpv6.c
@@ -72,7 +72,7 @@ dhcpv6_helper_cb(struct pkt_buff *pkt, uint32_t protoff,
return NF_ACCEPT;
dhcpv6_msg_type = pktb_network_header(pkt) + protoff + sizeof(struct
udphdr);
- if (*dhcpv6_msg_type > ARRAY_SIZE(dhcpv6_timeouts)) {
+ if (*dhcpv6_msg_type >= ARRAY_SIZE(dhcpv6_timeouts)) {
printf("Dropping DHCPv6 message with bad type %u\n",
*dhcpv6_msg_type);
return NF_DROP;
--
2.20.1
[ebtables PATCH] Print IPv6 prefixes in CIDR notation
According to RFC4291, IPv6 prefixes are represented in CIDR notation.
While the use of a "netmask" notation is not explicitly denied, its
existence merely stems from applying IPv4 standards to IPv6. This is not
necessarily correct.
Therefore change printing of IPv6 prefixes to use CIDR notation as long
as the address mask's bits are left contiguous.
Signed-off-by: Phil Sutter
---
useful_functions.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/useful_functions.c b/useful_functions.c
index 8a34f820f230b..bf4393712fa44 100644
--- a/useful_functions.c
+++ b/useful_functions.c
@@ -416,16 +416,41 @@ char *ebt_ip6_to_numeric(const struct in6_addr *addrp)
return (char *)inet_ntop(AF_INET6, addrp, buf, sizeof(buf));
}
+int ebt_ip6mask_to_cidr(const struct in6_addr *k)
+{
+ unsigned int bits = 0;
+ uint32_t a, b, c, d;
+
+ a = ntohl(k->s6_addr32[0]);
+ b = ntohl(k->s6_addr32[1]);
+ c = ntohl(k->s6_addr32[2]);
+ d = ntohl(k->s6_addr32[3]);
+ while (a & 0x8000U) {
+ ++bits;
+ a <<= 1;
+ a |= (b >> 31) & 1;
+ b <<= 1;
+ b |= (c >> 31) & 1;
+ c <<= 1;
+ c |= (d >> 31) & 1;
+ d <<= 1;
+ }
+ if (a != 0 || b != 0 || c != 0 || d != 0)
+ return -1;
+ return bits;
+}
+
char *ebt_ip6_mask_to_string(const struct in6_addr *msk)
{
- /* /:::::000.000.000.000
-* /::::::: */
+ int l = ebt_ip6mask_to_cidr(msk);
static char buf[51+1];
- if (msk->s6_addr32[0] == 0xL && msk->s6_addr32[1] ==
0xL &&
- msk->s6_addr32[2] == 0xL && msk->s6_addr32[3] ==
0xL)
+
+ if (l == 127)
*buf = '\0';
- else
+ else if (l == -1)
sprintf(buf, "/%s", ebt_ip6_to_numeric(msk));
+ else
+ sprintf(buf, "/%d", l);
return buf;
}
--
2.20.1
[conntrack-tools PATCH] nfct: Drop dead code in nfct_timeout_parse_params()
Due to the first switch() in that function, default case in second one
is unreachable. Given that both of them contain the same cases but the
first one merely acts as an invalid command barrier (adding no value to
the second one), drop the first one to make invalid commands actually
hit default case in the second switch().
Fixes: dd73ceecdbe87 ("nfct: Update syntax to specify command before subsystem")
Signed-off-by: Phil Sutter
---
src/nfct-extensions/timeout.c | 14 --
1 file changed, 14 deletions(-)
diff --git a/src/nfct-extensions/timeout.c b/src/nfct-extensions/timeout.c
index 30f94642bd3bd..31e91a63de722 100644
--- a/src/nfct-extensions/timeout.c
+++ b/src/nfct-extensions/timeout.c
@@ -54,20 +54,6 @@ nfct_timeout_parse_params(struct mnl_socket *nl, int argc,
char *argv[], int cmd
return -1;
}
- switch (cmd) {
- case NFCT_CMD_LIST:
- case NFCT_CMD_ADD:
- case NFCT_CMD_DELETE:
- case NFCT_CMD_GET:
- case NFCT_CMD_FLUSH:
- case NFCT_CMD_DEFAULT_SET:
- case NFCT_CMD_DEFAULT_GET:
- break;
- default:
- nfct_cmd_timeout_usage(argv);
- return -1;
- }
-
switch (cmd) {
case NFCT_CMD_LIST:
ret = nfct_cmd_timeout_list(nl, argc, argv);
--
2.20.1
[conntrack-tools PATCH] Fix for implicit-fallthrough warnings
Mark fall through cases as such. Note that correctness of those fall
throughs have not been verified.
Signed-off-by: Phil Sutter
---
include/jhash.h | 2 +-
src/cache-ct.c | 2 ++
src/cache-exp.c | 1 +
src/tcp.c | 1 +
4 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/jhash.h b/include/jhash.h
index d164e38bde7f6..9793452ebc23d 100644
--- a/include/jhash.h
+++ b/include/jhash.h
@@ -106,7 +106,7 @@ static inline u32 jhash2(const u32 *k, u32 length, u32
initval)
c += length * 4;
switch (len) {
- case 2 : b += k[1];
+ case 2 : b += k[1]; /* fall through */
case 1 : a += k[0];
};
diff --git a/src/cache-ct.c b/src/cache-ct.c
index f86d143c8137d..abcfde4c8a260 100644
--- a/src/cache-ct.c
+++ b/src/cache-ct.c
@@ -266,6 +266,7 @@ static int cache_ct_commit(struct cache *c, struct
nfct_handle *h, int clientfd)
STATE_SYNC(commit).stats.ok = c->stats.commit_ok;
STATE_SYNC(commit).stats.fail = c->stats.commit_fail;
STATE_SYNC(commit).clientfd = clientfd;
+ /* fall through */
case COMMIT_STATE_MASTER:
STATE_SYNC(commit).current =
hashtable_iterate_limit(c->h, &tmp,
@@ -280,6 +281,7 @@ static int cache_ct_commit(struct cache *c, struct
nfct_handle *h, int clientfd)
}
STATE_SYNC(commit).current = 0;
STATE_SYNC(commit).state = COMMIT_STATE_RELATED;
+ /* fall through */
case COMMIT_STATE_RELATED:
STATE_SYNC(commit).current =
hashtable_iterate_limit(c->h, &tmp,
diff --git a/src/cache-exp.c b/src/cache-exp.c
index 9183b2c42d93f..63e344078a7c6 100644
--- a/src/cache-exp.c
+++ b/src/cache-exp.c
@@ -236,6 +236,7 @@ cache_exp_commit(struct cache *c, struct nfct_handle *h,
int clientfd)
STATE_SYNC(commit).stats.ok = c->stats.commit_ok;
STATE_SYNC(commit).stats.fail = c->stats.commit_fail;
STATE_SYNC(commit).clientfd = clientfd;
+ /* fall through */
case COMMIT_STATE_MASTER:
STATE_SYNC(commit).current =
hashtable_iterate_limit(c->h, &tmp,
diff --git a/src/tcp.c b/src/tcp.c
index c8f254483aa35..91fe524542013 100644
--- a/src/tcp.c
+++ b/src/tcp.c
@@ -300,6 +300,7 @@ ssize_t tcp_send(struct tcp_sock *m, const void *data, int
size)
/* we got connected :) */
m->state = TCP_CLIENT_CONNECTED;
}
+ /* fall through */
case TCP_CLIENT_CONNECTED:
ret = sendto(m->fd, data, size, 0,
(struct sockaddr *) &m->addr, m->sockaddr_len);
--
2.20.1
[conntrack-tools PATCH] Support compiling against libtirpc
Try compiling against libtirpc on systems where RPC headers are not
provided by Glibc.
Due to naming conflicts, rpc_call() has had to be renamed.
Signed-off-by: Phil Sutter
---
Note that I didn't do real functional testing apart from running
conntrack and nfct testsuites. OTOH, in Fedora Rawhide the package is
linked against libtirpc as well and seems to work.
---
Make_global.am| 3 ++-
Makefile.am | 2 +-
configure.ac | 6 ++
src/helpers/rpc.c | 7 ---
4 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/Make_global.am b/Make_global.am
index 80842493a4ad0..554bb3ccc6951 100644
--- a/Make_global.am
+++ b/Make_global.am
@@ -6,4 +6,5 @@ AM_CFLAGS = -std=gnu99 -W -Wall \
${LIBNETFILTER_CONNTRACK_CFLAGS} \
${LIBNETFILTER_CTTIMEOUT_CFLAGS} \
${LIBNETFILTER_QUEUE_CFLAGS} \
- ${LIBNETFILTER_CTHELPER_CFLAGS}
+ ${LIBNETFILTER_CTHELPER_CFLAGS} \
+ ${LIBTIRPC_CFLAGS}
diff --git a/Makefile.am b/Makefile.am
index f64d60438d411..05072e1f696d6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -7,7 +7,7 @@ EXTRA_DIST = $(man_MANS) Make_global.am doc m4 tests
SUBDIRS = extensions src
DIST_SUBDIRS = include src extensions
-LIBS = @LIBNETFILTER_CONNTRACK_LIBS@
+LIBS = @LIBNETFILTER_CONNTRACK_LIBS@ @LIBTIRPC_LIBS@
dist-hook:
rm -rf `find $(distdir)/doc -name *.orig`
diff --git a/configure.ac b/configure.ac
index 048d261ac1088..5384d3c80962c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -73,6 +73,12 @@ AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$enable_systemd" =
"xyes"])
AC_CHECK_HEADERS([linux/capability.h],, [AC_MSG_ERROR([Cannot find
linux/capabibility.h])])
+# check for rpc_msg.h existence
+AC_CHECK_HEADER([rpc/rpc_msg.h], [have_rpc_msg_h=yes], [have_rpc_msg_h=no])
+AS_IF([test "x$have_rpc_msg_h" = "xno"],
+ [PKG_CHECK_MODULES([LIBTIRPC], [libtirpc], [],
+ [AC_MSG_ERROR([No provider for rpc/rpc_msg.h
found])])])
+
# Checks for libraries.
# FIXME: Replace `main' with a function in `-lc':
dnl AC_CHECK_LIB([c], [main])
diff --git a/src/helpers/rpc.c b/src/helpers/rpc.c
index 3a7b337135f04..bd24dd3269c8e 100644
--- a/src/helpers/rpc.c
+++ b/src/helpers/rpc.c
@@ -26,6 +26,7 @@
#include
+#include
#include
#include
#define _GNU_SOURCE
@@ -114,8 +115,8 @@ nf_nat_rpc(struct pkt_buff *pkt, int dir, struct nf_expect
*exp,
#define ROUNDUP(n) n) + 3)/4)*4)
static int
-rpc_call(const uint32_t *data, uint32_t offset, uint32_t datalen,
-struct rpc_info *rpc_info)
+rpc_parse_call(const uint32_t *data, uint32_t offset, uint32_t datalen,
+ struct rpc_info *rpc_info)
{
uint32_t p, r;
@@ -393,7 +394,7 @@ rpc_helper_cb(struct pkt_buff *pkt, uint32_t protoff,
}
if (rm_dir == CALL) {
- if (rpc_call(data, offset, datalen, rpc_info) < 0)
+ if (rpc_parse_call(data, offset, datalen, rpc_info) < 0)
goto out;
rpc_info->xid = xid;
--
2.20.1
Re: [conntrack-tools PATCH] Support compiling against libtirpc
Hi Jan,
On Wed, Feb 13, 2019 at 12:30:17AM +0100, Jan Engelhardt wrote:
> On Wednesday 2019-02-13 00:22, Phil Sutter wrote:
>
> > SUBDIRS = extensions src
> > DIST_SUBDIRS = include src extensions
> >-LIBS = @LIBNETFILTER_CONNTRACK_LIBS@
> >+LIBS = @LIBNETFILTER_CONNTRACK_LIBS@ @LIBTIRPC_LIBS@
>
> This should all use ${LIBNETFILTER_CONNTRACK_LIBS} ${LIBTIRPC_LIBS}.
> (You're writing Makefile.am, not Makefile.in, so using ${} is possible and
> much desired.)
Ah, thanks! Replicating broken code is obviously still easier than
thinking for oneself. I'll send a v2.
Thanks, Phil
[conntrack-tools PATCH v2] Support compiling against libtirpc
Try compiling against libtirpc on systems where RPC headers are not
provided by Glibc.
Due to naming conflicts, rpc_call() has had to be renamed.
Cc: Jan Engelhardt
Signed-off-by: Phil Sutter
---
Note that I didn't do real functional testing apart from running
conntrack and nfct testsuites. OTOH, in Fedora Rawhide the package is
linked against libtirpc as well and seems to work.
Changes since v1:
- Fix @VAR@ into ${VAR} in Makefile.am as suggested by Jan Engelhardt.
---
Make_global.am| 3 ++-
Makefile.am | 2 +-
configure.ac | 6 ++
src/helpers/rpc.c | 7 ---
4 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/Make_global.am b/Make_global.am
index 80842493a4ad0..554bb3ccc6951 100644
--- a/Make_global.am
+++ b/Make_global.am
@@ -6,4 +6,5 @@ AM_CFLAGS = -std=gnu99 -W -Wall \
${LIBNETFILTER_CONNTRACK_CFLAGS} \
${LIBNETFILTER_CTTIMEOUT_CFLAGS} \
${LIBNETFILTER_QUEUE_CFLAGS} \
- ${LIBNETFILTER_CTHELPER_CFLAGS}
+ ${LIBNETFILTER_CTHELPER_CFLAGS} \
+ ${LIBTIRPC_CFLAGS}
diff --git a/Makefile.am b/Makefile.am
index f64d60438d411..d73d7f4c54ff2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -7,7 +7,7 @@ EXTRA_DIST = $(man_MANS) Make_global.am doc m4 tests
SUBDIRS = extensions src
DIST_SUBDIRS = include src extensions
-LIBS = @LIBNETFILTER_CONNTRACK_LIBS@
+LIBS = ${LIBNETFILTER_CONNTRACK_LIBS} ${LIBTIRPC_LIBS}
dist-hook:
rm -rf `find $(distdir)/doc -name *.orig`
diff --git a/configure.ac b/configure.ac
index 048d261ac1088..5384d3c80962c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -73,6 +73,12 @@ AM_CONDITIONAL([HAVE_SYSTEMD], [test "x$enable_systemd" =
"xyes"])
AC_CHECK_HEADERS([linux/capability.h],, [AC_MSG_ERROR([Cannot find
linux/capabibility.h])])
+# check for rpc_msg.h existence
+AC_CHECK_HEADER([rpc/rpc_msg.h], [have_rpc_msg_h=yes], [have_rpc_msg_h=no])
+AS_IF([test "x$have_rpc_msg_h" = "xno"],
+ [PKG_CHECK_MODULES([LIBTIRPC], [libtirpc], [],
+ [AC_MSG_ERROR([No provider for rpc/rpc_msg.h
found])])])
+
# Checks for libraries.
# FIXME: Replace `main' with a function in `-lc':
dnl AC_CHECK_LIB([c], [main])
diff --git a/src/helpers/rpc.c b/src/helpers/rpc.c
index 3a7b337135f04..bd24dd3269c8e 100644
--- a/src/helpers/rpc.c
+++ b/src/helpers/rpc.c
@@ -26,6 +26,7 @@
#include
+#include
#include
#include
#define _GNU_SOURCE
@@ -114,8 +115,8 @@ nf_nat_rpc(struct pkt_buff *pkt, int dir, struct nf_expect
*exp,
#define ROUNDUP(n) n) + 3)/4)*4)
static int
-rpc_call(const uint32_t *data, uint32_t offset, uint32_t datalen,
-struct rpc_info *rpc_info)
+rpc_parse_call(const uint32_t *data, uint32_t offset, uint32_t datalen,
+ struct rpc_info *rpc_info)
{
uint32_t p, r;
@@ -393,7 +394,7 @@ rpc_helper_cb(struct pkt_buff *pkt, uint32_t protoff,
}
if (rm_dir == CALL) {
- if (rpc_call(data, offset, datalen, rpc_info) < 0)
+ if (rpc_parse_call(data, offset, datalen, rpc_info) < 0)
goto out;
rpc_info->xid = xid;
--
2.20.1
[iptables PATCH 5/5] tests: Extend return codes check by error messages
Check that error messages match between legacy and nft code.
Signed-off-by: Phil Sutter
---
.../testcases/iptables/0004-return-codes_0| 59 +++
1 file changed, 46 insertions(+), 13 deletions(-)
diff --git a/iptables/tests/shell/testcases/iptables/0004-return-codes_0
b/iptables/tests/shell/testcases/iptables/0004-return-codes_0
index 9d2493992bd69..15f3a3e9efb68 100755
--- a/iptables/tests/shell/testcases/iptables/0004-return-codes_0
+++ b/iptables/tests/shell/testcases/iptables/0004-return-codes_0
@@ -5,44 +5,77 @@
global_rc=0
-cmd() { # (rc, cmd, [args ...])
+cmd() { # (rc, msg, cmd, [args ...])
rc_exp=$1; shift
- $XT_MULTI "$@"
+ msg_exp=""
+ [ $rc_exp != 0 ] && {
+ msg_exp="$1"; shift
+ }
+
+ msg="$($XT_MULTI "$@" 2>&1 >/dev/null)"
rc=$?
[ $rc -eq $rc_exp ] || {
- echo "---> expected $rc_exp, got $rc for command '$@'"
+ echo "---> expected return code $rc_exp, got $rc for command
'$@'"
+ global_rc=1
+ }
+
+ [ -n "$msg_exp" ] || return
+ grep -q "$msg_exp" <<< $msg || {
+ echo "---> expected error message '$msg_exp', got '$msg' for
command '$@'"
global_rc=1
}
}
+EEXIST_F="File exists."
+EEXIST="Chain already exists."
+ENOENT="No chain/target/match by that name."
+E2BIG_I="Index of insertion too big."
+E2BIG_D="Index of deletion too big."
+E2BIG_R="Index of replacement too big."
+EBADRULE="Bad rule (does a matching rule exist in that chain?)."
+ENOTGT="Couldn't load target \`foobar':No such file or directory"
+ENOMTH="Couldn't load match \`foobar':No such file or directory"
+ENOTBL="can't initialize iptables table \`foobar': Table does not exist"
+
# test chain creation
cmd 0 iptables -N foo
-cmd 1 iptables -N foo
+cmd 1 "$EEXIST" iptables -N foo
# iptables-nft allows this - bug or feature?
#cmd 2 iptables -N "invalid name"
# test chain flushing/zeroing
cmd 0 iptables -F foo
cmd 0 iptables -Z foo
-cmd 1 iptables -F bar
-cmd 1 iptables -Z bar
+cmd 1 "$ENOENT" iptables -F bar
+cmd 1 "$ENOENT" iptables -Z bar
# test chain rename
cmd 0 iptables -E foo bar
-cmd 1 iptables -E foo bar
+cmd 1 "$EEXIST_F" iptables -E foo bar
# test rule adding
cmd 0 iptables -A INPUT -j ACCEPT
-cmd 1 iptables -A noexist -j ACCEPT
+cmd 1 "$ENOENT" iptables -A noexist -j ACCEPT
+
+# test rulenum commands
+cmd 1 "$E2BIG_I" iptables -I INPUT 23 -j ACCEPT
+cmd 1 "$E2BIG_D" iptables -D INPUT 23
+cmd 1 "$E2BIG_R" iptables -R INPUT 23 -j ACCEPT
+cmd 1 "$ENOENT" iptables -I nonexist 23 -j ACCEPT
+cmd 1 "$ENOENT" iptables -D nonexist 23
+cmd 1 "$ENOENT" iptables -R nonexist 23 -j ACCEPT
# test rule checking
cmd 0 iptables -C INPUT -j ACCEPT
-cmd 1 iptables -C FORWARD -j ACCEPT
-cmd 1 iptables -C nonexist -j ACCEPT
-cmd 2 iptables -C INPUT -j foobar
-cmd 2 iptables -C INPUT -m foobar -j ACCEPT
-cmd 3 iptables -t foobar -C INPUT -j ACCEPT
+cmd 1 "$EBADRULE" iptables -C FORWARD -j ACCEPT
+cmd 1 "$BADRULE" iptables -C nonexist -j ACCEPT
+cmd 2 "$ENOMTH" iptables -C INPUT -m foobar -j ACCEPT
+# messages of those don't match, but iptables-nft ones are actually nicer.
+#cmd 2 "$ENOTGT" iptables -C INPUT -j foobar
+#cmd 3 "$ENOTBL" iptables -t foobar -C INPUT -j ACCEPT
+cmd 2 "" iptables -C INPUT -j foobar
+cmd 3 "" iptables -t foobar -C INPUT -j ACCEPT
exit $global_rc
--
2.20.1
[iptables PATCH 2/5] xtables: Move new chain check to where it belongs
Instead of checking chain existence in xtables.c, do it in
nft_chain_user_add() and reuse predefined error message.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 5 +
iptables/xtables.c | 3 ---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index 60b0531f4c8c8..c1b8ba3aa4bcf 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1726,6 +1726,11 @@ int nft_chain_user_add(struct nft_handle *h, const char
*chain, const char *tabl
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
nft_xt_builtin_init(h, table);
+ if (nft_chain_exists(h, table, chain)) {
+ errno = EEXIST;
+ return 0;
+ }
+
c = nftnl_chain_alloc();
if (c == NULL)
return 0;
diff --git a/iptables/xtables.c b/iptables/xtables.c
index 1d777554076d7..44986a37aaf50 100644
--- a/iptables/xtables.c
+++ b/iptables/xtables.c
@@ -1069,9 +1069,6 @@ void do_parse(struct nft_handle *h, int argc, char
*argv[],
xtables_error(PARAMETER_PROBLEM,
"Chain '%s' does not exist", cs->jumpto);
}
- if (!p->xlate && p->command == CMD_NEW_CHAIN &&
- nft_chain_exists(h, p->table, p->chain))
- xtables_error(OTHER_PROBLEM, "Chain already exists");
}
int do_commandx(struct nft_handle *h, int argc, char *argv[], char **table,
--
2.20.1
[iptables PATCH 1/5] xtables: Fix error message when zeroing a non-existent chain
Previously, error message was a bit misleading:
| # iptables-nft -Z noexist
| iptables: Incompatible with this kernel.
Set errno value so that the typical "No chain/target/match by that
name." is printed instead.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index d708fb6176b88..60b0531f4c8c8 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3235,8 +3235,10 @@ int nft_chain_zero_counters(struct nft_handle *h, const
char *chain,
if (chain) {
c = nftnl_chain_list_lookup_byname(list, chain);
- if (!c)
+ if (!c) {
+ errno = ENOENT;
return 0;
+ }
ret = __nft_chain_zero_counters(c, &d);
goto err;
--
2.20.1
[iptables PATCH 3/5] xtables: Fix error messages in commands with rule number
Use E2BIG if rule identified by given number is not found. ENOENT is
used if referenced chain is not found. Without this, a command
specifying a non-existing chain in combination with a rule number like
e.g.: 'iptables-nft -I nonexist 23 -j ACCEPT' returns "Index of
insertion too big." instead of "No chain/target/match by that name."
like legacy iptables does.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index c1b8ba3aa4bcf..f42a1be734ba8 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -2235,7 +2235,7 @@ int nft_rule_insert(struct nft_handle *h, const char
*chain,
return nft_rule_append(h, chain, table, data,
NULL, verbose);
- errno = ENOENT;
+ errno = E2BIG;
goto err;
}
}
@@ -2276,7 +2276,7 @@ int nft_rule_delete_num(struct nft_handle *h, const char
*chain,
if (ret < 0)
errno = ENOMEM;
} else
- errno = ENOENT;
+ errno = E2BIG;
return ret;
}
@@ -2304,7 +2304,7 @@ int nft_rule_replace(struct nft_handle *h, const char
*chain,
ret = nft_rule_append(h, chain, table, data, r, verbose);
} else
- errno = ENOENT;
+ errno = E2BIG;
return ret;
}
@@ -2985,10 +2985,10 @@ const char *nft_strerror(int err)
{ nft_chain_user_del, EMLINK,
"Can't delete chain with references left" },
{ nft_chain_user_add, EEXIST, "Chain already exists" },
- { nft_rule_insert, ENOENT, "Index of insertion too big" },
+ { nft_rule_insert, E2BIG, "Index of insertion too big" },
{ nft_rule_check, ENOENT, "Bad rule (does a matching rule exist in
that chain?)" },
- { nft_rule_replace, ENOENT, "Index of replacement too big" },
- { nft_rule_delete_num, ENOENT, "Index of deletion too big" },
+ { nft_rule_replace, E2BIG, "Index of replacement too big" },
+ { nft_rule_delete_num, E2BIG, "Index of deletion too big" },
/* { TC_READ_COUNTER, E2BIG, "Index of counter too big" },
{ TC_ZERO_COUNTER, E2BIG, "Index of counter too big" }, */
/* ENOENT for DELETE probably means no matching rule */
--
2.20.1
[iptables PATCH 0/5] Align iptables-nft error messages with legacy
After extending return codes testcase by error messages (see last patch) so that it passes legacy iptables, I went on to fix iptables-nft output accordingly. This is all cosmetics but patch 2 which actually simplifies the necessary check for chain existence when trying to create a new user-defined chain. Phil Sutter (5): xtables: Fix error message when zeroing a non-existent chain xtables: Move new chain check to where it belongs xtables: Fix error messages in commands with rule number xtables: Fix error message for chain renaming tests: Extend return codes check by error messages iptables/nft.c| 29 ++--- .../testcases/iptables/0004-return-codes_0| 59 +++ iptables/xtables.c| 3 - 3 files changed, 67 insertions(+), 24 deletions(-) -- 2.20.1
[iptables PATCH 4/5] xtables: Fix error message for chain renaming
If the new name already exists, legacy iptables prints "File exists.".
This is a bit exotic, but more appropriate than "No chain/target/match
by that name." printed by iptables-nft without this patch.
Signed-off-by: Phil Sutter
---
iptables/nft.c | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/iptables/nft.c b/iptables/nft.c
index f42a1be734ba8..a297d9856001a 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1855,7 +1855,12 @@ int nft_chain_user_rename(struct nft_handle *h,const
char *chain,
uint64_t handle;
int ret;
- nft_fn = nft_chain_user_add;
+ nft_fn = nft_chain_user_rename;
+
+ if (nft_chain_exists(h, table, newname)) {
+ errno = EEXIST;
+ return 0;
+ }
/* If built-in chains don't exist for this table, create them */
if (nft_xtables_config_load(h, XTABLES_CONFIG_DEFAULT, 0) < 0)
@@ -2985,6 +2990,7 @@ const char *nft_strerror(int err)
{ nft_chain_user_del, EMLINK,
"Can't delete chain with references left" },
{ nft_chain_user_add, EEXIST, "Chain already exists" },
+ { nft_chain_user_rename, EEXIST, "File exists" },
{ nft_rule_insert, E2BIG, "Index of insertion too big" },
{ nft_rule_check, ENOENT, "Bad rule (does a matching rule exist in
that chain?)" },
{ nft_rule_replace, E2BIG, "Index of replacement too big" },
--
2.20.1
Re: [conntrack-tools PATCH] Fix for implicit-fallthrough warnings
On Wed, Feb 13, 2019 at 06:21:00PM +0100, Pablo Neira Ayuso wrote: > On Wed, Feb 13, 2019 at 12:13:44AM +0100, Phil Sutter wrote: > > Mark fall through cases as such. Note that correctness of those fall > > throughs have not been verified. > > Applied, thanks. > > BTW, do we need to update any Makefile to include a new -W option for > gcc to check for missing "fall through"? TBH, I have no idea. :) I use gcc-8.2.0 here and it emitted the warnings by default. Maybe with older versions you need -Wimplicit-fallthrough to see them? Cheers, Phil
RFC: nftables does not allow to delete a rule twice
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 <
Re: [ebtables PATCH] Print IPv6 prefixes in CIDR notation
Hi Pablo, On Thu, Feb 14, 2019 at 04:57:40PM +0100, Pablo Neira Ayuso wrote: > On Tue, Feb 12, 2019 at 10:51:34PM +0100, Phil Sutter wrote: > > According to RFC4291, IPv6 prefixes are represented in CIDR notation. > > While the use of a "netmask" notation is not explicitly denied, its > > existence merely stems from applying IPv4 standards to IPv6. This is not > > necessarily correct. > > > > Therefore change printing of IPv6 prefixes to use CIDR notation as long > > as the address mask's bits are left contiguous. > > Applied, thanks. > > Only affects -L listing, right? I think people should not be scraping > that output. No, this is effective for ebtables-save, too. Feel free to revert if you consider that unacceptable, I'll adjust ebtables-nft output into the opposite direction then. :( Cheers, Phil
Re: [nft PATCH] src: Quote user-defined names
Hi Pablo, On Thu, Feb 14, 2019 at 12:10:54PM +0100, Pablo Neira Ayuso wrote: > Another spin on this, let's try to make a final decision on this asap. > > In this case, this patch passes the quoted string to the kernel, so > the listing shows it again. > > Still, problem here is that the shell is stripping off the quotes > unless I escape them, ie. > > nft add chain "x" "y" > > enters the unquoted path from the scannner. So I have to use: > > nft add chain \"x\" \"y\" > > or: > > nft add chain x '"y"' > > I think your patch fixes the problem with using keywords as object > names, which we could also fix via a rule that deals with this. > > The problem with using any arbitrary name would be still there, unless > the user escapes the quotes. > > On the other hand, if we quote the string in the listing by default, > we clearly show that these are user-defined, which is not a bad idea. > However, we don't get much from showing quotes by default on listings, > I mean, this is not solving the arbitrary name problem from the input > path, which I think it the real problem. > > Then, enforcing quotes since the beginning would not have helped > either, because of the shell behaviour. > > Exploring another patch here to allow to use keywords without quotes > as object names, it won't look nice in bison, since we will need > something similar to what we do in primary_rhs_expr for TCP, UDP... > but it will work. Are you sure about that? Flex would still recognize the keyword as such and you won't get STRING type in Bison. Or am I missing the point? Personally, I'm totally fine with people having to escape the quotes. This is how shells work, and we have the same problem in other situation requiring the quotes, too. My shell for instance catches the curly braces and semi-colons, as well if not escaped. Quoting all user-defined names on output merely helps with the case where a user *really* wanted to produce a confusing ruleset and to avoid ruleset restore after dump from failing miserably because the names are not quoted in output. HTH, Phil
Re: RFC: nftables does not allow to delete a rule twice
Hi,
On Fri, Feb 15, 2019 at 10:50:42AM +0100, Pablo Neira Ayuso wrote:
[...]
> > 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.
Thanks for the quick fix!
> nft_delrule_by_chain() is always called from flush path. We can apply
> this smaller fix I think.
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 5a92f23f179f..4893f248dfdc 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -313,6 +313,9 @@ static int nft_delrule_by_chain(struct nft_ctx *ctx)
> int err;
>
> list_for_each_entry(rule, &ctx->chain->rules, list) {
> + if (!nft_is_active_next(ctx->net, rule))
> + continue;
> +
> err = nft_delrule(ctx, rule);
> if (err < 0)
> return err;
Successfully tested with my ebtables-nft bug:
| # ebtables-nft -N foo
| # ebtables-nft -F
(Batch then contains policy rule delete and chain flush.)
Acked-by: Phil Sutter
Thanks, Phil
[iptables PATCH] arptables: Print space before comma and counters
Legacy arptables separates counters from rest of rule by ' , '. Assuming
that scripts scraping 'arptables -vL' output match on this, make
arptables-nft output conformant.
Signed-off-by: Phil Sutter
---
iptables/nft-arp.c | 2 +-
.../arptables/0003-arptables-verbose-output_0| 12 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/iptables/nft-arp.c b/iptables/nft-arp.c
index 637da4274ded3..a37155798e85e 100644
--- a/iptables/nft-arp.c
+++ b/iptables/nft-arp.c
@@ -617,7 +617,7 @@ nft_arp_print_rule(struct nftnl_rule *r, unsigned int num,
unsigned int format)
print_matches_and_target(&cs, format);
if (!(format & FMT_NOCOUNTS)) {
- printf(", pcnt=");
+ printf(" , pcnt=");
xtables_print_num(cs.counters.pcnt, format | FMT_NOTABLE);
printf("-- bcnt=");
xtables_print_num(cs.counters.bcnt, format | FMT_NOTABLE);
diff --git
a/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
b/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
index 35126fa7d717c..3a9807a1cfe0b 100755
--- a/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
+++ b/iptables/tests/shell/testcases/arptables/0003-arptables-verbose-output_0
@@ -36,16 +36,16 @@ diff -u -Z <(echo -e "$VOUT5") <($XT_MULTI arptables -v -A
OUTPUT $RULE5)
diff -u -Z <(echo -e "$VOUT6") <($XT_MULTI arptables -v -A foo $RULE6)
EXPECT='Chain INPUT (policy ACCEPT 0 packets, 0 bytes)
--j ACCEPT -i eth23 -o *, pcnt=0 -- bcnt=0
--i eth23 -o *, pcnt=0 -- bcnt=0
--j MARK -i eth23 -o * --set-mark 42, pcnt=0 -- bcnt=0
+-j ACCEPT -i eth23 -o * , pcnt=0 -- bcnt=0
+-i eth23 -o * , pcnt=0 -- bcnt=0
+-j MARK -i eth23 -o * --set-mark 42 , pcnt=0 -- bcnt=0
Chain OUTPUT (policy ACCEPT 0 packets, 0 bytes)
--j CLASSIFY -i * -o eth23 --set-class 23:42, pcnt=0 -- bcnt=0
--j foo -i * -o eth23, pcnt=0 -- bcnt=0
+-j CLASSIFY -i * -o eth23 --set-class 23:42 , pcnt=0 -- bcnt=0
+-j foo -i * -o eth23 , pcnt=0 -- bcnt=0
Chain foo (1 references)
--j mangle -i * -o eth23 --mangle-ip-s 10.0.0.1, pcnt=0 -- bcnt=0'
+-j mangle -i * -o eth23 --mangle-ip-s 10.0.0.1 , pcnt=0 -- bcnt=0'
diff -u -Z <(echo -e "$EXPECT") <($XT_MULTI arptables -v -n -L)
--
2.20.1
Re: RFC: nftables does not allow to delete a rule twice
On Fri, Feb 15, 2019 at 03:33:02PM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 15, 2019 at 02:15:37PM +0100, Phil Sutter wrote:
> > Hi,
> >
> > On Fri, Feb 15, 2019 at 10:50:42AM +0100, Pablo Neira Ayuso wrote:
> > [...]
> > > > 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.
> >
> > Thanks for the quick fix!
> >
> > > nft_delrule_by_chain() is always called from flush path. We can apply
> > > this smaller fix I think.
> >
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index 5a92f23f179f..4893f248dfdc 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -313,6 +313,9 @@ static int nft_delrule_by_chain(struct nft_ctx *ctx)
> > > int err;
> > >
> > > list_for_each_entry(rule, &ctx->chain->rules, list) {
> > > + if (!nft_is_active_next(ctx->net, rule))
> > > + continue;
> > > +
> > > err = nft_delrule(ctx, rule);
> > > if (err < 0)
> > > return err;
> >
> > Successfully tested with my ebtables-nft bug:
> >
> > | # ebtables-nft -N foo
> > | # ebtables-nft -F
> >
> > (Batch then contains policy rule delete and chain flush.)
> >
> > Acked-by: Phil Sutter
>
> OK, I have pushed this out to nf.git.
Nice, thanks! If this has soaked into stable for a while, we may drop
nft_abort_policy_rule() again, I guess. :)
Cheers, Phil
[iptables PATCH 0/5] Make testsuites a bit more versatile
The first two patches in this series enable xlate and shell testsuites to be run against installed binaries on host system. This is enabled via an extra commandline parameter, the default is unchanged. Patch 3 adds support for customizing connlabel.conf path to connlabel extension. This is a prerequisite for the last two patches, which change xlate and iptables testsuites to define a custom configuration for testing purposes instead of having to rely on host's filesystem to contain the file (or allow changing it in case of iptables testsuite). Phil Sutter (5): xlate-test: Support testing host binaries tests/shell: Support testing host binaries extensions: connlabel: Allow connlabel.conf override xlate-test: Add and use a connlabel.conf for testing iptables-test: Make use of sample connlabel.conf extensions/.gitignore| 1 + extensions/libxt_connlabel.c | 6 ++-- extensions/libxt_connlabel.conf.test | 3 ++ extensions/libxt_connlabel.man | 2 ++ extensions/libxt_connlabel.t | 13 ++-- iptables-test.py | 2 ++ iptables/tests/shell/run-tests.sh| 48 ++-- xlate-test.py| 17 -- 8 files changed, 61 insertions(+), 31 deletions(-) create mode 100644 extensions/libxt_connlabel.conf.test -- 2.20.1
