* [nftables 2/9] netfilter: nf_tables: fix chain after rule deletion
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
@ 2013-01-31 0:03 ` pablo
2013-01-31 0:03 ` [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps pablo
` (6 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:03 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
Rules are released via RCU and they may take longer to be released
than the chain. Since the chain refcounter is decremented by the
rcu callback, we may hit -EBUSY while deleting a chain since the
chain is released faster than the rule.
Make sure the chain is released after the rule by forcing that
the chain is released after all rules that reference it.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 2 ++
net/netfilter/nf_tables_api.c | 14 ++++++++++++--
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 5c75072..3ba63b6 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -367,6 +367,7 @@ enum nft_chain_flags {
*
* @rules: list of rules in the chain
* @list: used internally
+ * @rcu_head: used internally
* @handle: chain handle
* @flags: bitmask of enum nft_chain_flags
* @use: number of jump references to this chain
@@ -376,6 +377,7 @@ enum nft_chain_flags {
struct nft_chain {
struct list_head rules;
struct list_head list;
+ struct rcu_head rcu_head;
u64 handle;
u8 flags;
u16 use;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a8ae0b4..db6150b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -882,6 +882,15 @@ notify:
return 0;
}
+static void nf_tables_rcu_chain_destroy(struct rcu_head *head)
+{
+ struct nft_chain *chain = container_of(head, struct nft_chain, rcu_head);
+
+ BUG_ON(chain->use > 0);
+
+ kfree(chain);
+}
+
static int nf_tables_delchain(struct sock *nlsk, struct sk_buff *skb,
const struct nlmsghdr *nlh,
const struct nlattr * const nla[])
@@ -905,7 +914,7 @@ static int nf_tables_delchain(struct sock *nlsk, struct sk_buff *skb,
if (IS_ERR(chain))
return PTR_ERR(chain);
- if (!list_empty(&chain->rules) || chain->use > 0)
+ if (!list_empty(&chain->rules))
return -EBUSY;
list_del(&chain->list);
@@ -917,7 +926,8 @@ static int nf_tables_delchain(struct sock *nlsk, struct sk_buff *skb,
nf_tables_chain_notify(skb, nlh, table, chain, NFT_MSG_DELCHAIN,
family);
- kfree(chain);
+ /* Make sure all rule references are gone before this is released */
+ call_rcu(&chain->rcu_head, nf_tables_rcu_chain_destroy);
return 0;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
2013-01-31 0:03 ` [nftables 2/9] netfilter: nf_tables: fix chain after rule deletion pablo
@ 2013-01-31 0:03 ` pablo
2013-02-18 17:17 ` Tomasz Bursztyka
2013-02-19 22:02 ` Patrick McHardy
2013-01-31 0:04 ` [nftables 4/9] netfilter: nf_tables: fix error path in newchain pablo
` (5 subsequent siblings)
7 siblings, 2 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:03 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
This patch adds global atomic rule updates and dumps based on
bitmask generations. This allows to atomically commit a set of
rule-set updates incrementally without altering the internal
state of existing nf_tables expressions/matches/targets.
The idea consists of using a generation cursor of 1 bit and
a bitmask of 2 bits per rule. Assuming the gencursor is 0,
then the genmask (expressed as a bitmask) can be interpreted
as:
00 active in the present, will be active in the next generation.
01 inactive in the present, will be active in the next generation.
10 active in the present, will be deleted in the next generation.
^
gencursor
Once you invoke the transition to the next generation, the global
gencursor is updated:
00 active in the present, will be active in the next generation.
01 active in the present, needs to zero its future, it becomes 00.
10 inactive in the present, delete now.
^
gencursor
If a dump is in progress and nf_tables enters a new generation,
the dump will stop and return -EBUSY to let userspace know that
it has to retry again. In order to invalidate dumps, a global
genctr counter is increased everytime nf_tables enters a new
generation.
This new operation can be used from the user-space utility
that controls the firewall, eg.
nft restore < file
The rule updates contained in `file' will be applied atomically.
cat file
-----
add filter INPUT ip saddr 1.1.1.1 counter accept #1
del filter INPUT ip daddr 2.2.2.2 counter drop #2
commit #3
-EOF-
Note that the rule 1 will be inactive until the transition to the
next generation, the rule 2 will be evicted in the next generation.
There is a penalty during the rule update due to the branch
misprediction in the packet matching framework. But that should be
quickly resolved once the iteration over the dirty list that
contain rules that require updates is finished.
Event notification happens once the rule-set update has been
committed. So we skip notifications is case the rule-set update
is aborted, which can happen in case that the rule-set is tested
to apply correctly.
This patch is based on ideas extracted from discussions with
Patrick McHardy.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/linux/netfilter/nf_tables.h | 8 ++
include/net/netfilter/nf_tables.h | 10 +-
include/net/netns/nftables.h | 2 +
net/netfilter/nf_tables_api.c | 227 ++++++++++++++++++++++++++++++++---
net/netfilter/nf_tables_core.c | 10 ++
5 files changed, 240 insertions(+), 17 deletions(-)
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index 7640290..3749069 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -37,6 +37,8 @@ enum nf_tables_msg_types {
NFT_MSG_NEWSETELEM,
NFT_MSG_GETSETELEM,
NFT_MSG_DELSETELEM,
+ NFT_MSG_COMMIT,
+ NFT_MSG_ABORT,
NFT_MSG_MAX,
};
@@ -85,12 +87,18 @@ enum nft_chain_attributes {
};
#define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
+enum {
+ NFT_RULE_F_COMMIT = (1 << 0),
+ NFT_RULE_F_MASK = NFT_RULE_F_COMMIT,
+};
+
enum nft_rule_attributes {
NFTA_RULE_UNSPEC,
NFTA_RULE_TABLE,
NFTA_RULE_CHAIN,
NFTA_RULE_HANDLE,
NFTA_RULE_EXPRESSIONS,
+ NFTA_RULE_FLAGS,
__NFTA_RULE_MAX
};
#define NFTA_RULE_MAX (__NFTA_RULE_MAX - 1)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3ba63b6..1131e49 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -319,15 +319,19 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
* struct nft_rule - nf_tables rule
*
* @list: used internally
+ * @dirty_list: this rule needs an update after new generation
* @rcu_head: used internally for rcu
* @handle: rule handle
+ * @genmask: generation mask
* @dlen: length of expression data
* @data: expression data
*/
struct nft_rule {
struct list_head list;
+ struct list_head dirty_list;
struct rcu_head rcu_head;
- u64 handle:48,
+ u64 handle:46,
+ genmask:2,
dlen:16;
unsigned char data[]
__attribute__((aligned(__alignof__(struct nft_expr))));
@@ -366,8 +370,10 @@ enum nft_chain_flags {
* struct nft_chain - nf_tables chain
*
* @rules: list of rules in the chain
+ * @dirty_rules: rules that need an update after next generation
* @list: used internally
* @rcu_head: used internally
+ * @net: net namespace that this chain belongs to
* @handle: chain handle
* @flags: bitmask of enum nft_chain_flags
* @use: number of jump references to this chain
@@ -376,8 +382,10 @@ enum nft_chain_flags {
*/
struct nft_chain {
struct list_head rules;
+ struct list_head dirty_rules;
struct list_head list;
struct rcu_head rcu_head;
+ struct net *net;
u64 handle;
u8 flags;
u16 use;
diff --git a/include/net/netns/nftables.h b/include/net/netns/nftables.h
index a98b1c5..fe919c7 100644
--- a/include/net/netns/nftables.h
+++ b/include/net/netns/nftables.h
@@ -10,6 +10,8 @@ struct netns_nftables {
struct nft_af_info *ipv4;
struct nft_af_info *ipv6;
struct nft_af_info *bridge;
+ u8 gencursor:1,
+ genctr:7;
};
#endif
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index db6150b..7f08381 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -863,7 +863,9 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
}
INIT_LIST_HEAD(&chain->rules);
+ INIT_LIST_HEAD(&chain->dirty_rules);
chain->handle = nf_tables_alloc_handle(table);
+ chain->net = net;
nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);
list_add_tail(&chain->list, &table->chains);
@@ -1150,6 +1152,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
.len = NFT_CHAIN_MAXNAMELEN - 1 },
[NFTA_RULE_HANDLE] = { .type = NLA_U64 },
[NFTA_RULE_EXPRESSIONS] = { .type = NLA_NESTED },
+ [NFTA_RULE_FLAGS] = { .type = NLA_U32 },
};
static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
@@ -1239,6 +1242,41 @@ err:
return err;
}
+static inline bool
+nft_rule_is_active(struct net *net, const struct nft_rule *rule)
+{
+ return (rule->genmask & (1 << net->nft.gencursor)) == 0;
+}
+
+static inline int gencursor_next(struct net *net)
+{
+ return net->nft.gencursor+1 == 1 ? 1 : 0;
+}
+
+static inline int
+nft_rule_is_active_next(struct net *net, const struct nft_rule *rule)
+{
+ return (rule->genmask & (1 << gencursor_next(net))) == 0;
+}
+
+static inline void
+nft_rule_activate_next(struct net *net, struct nft_rule *rule)
+{
+ /* Now inactive, will be active in the future */
+ rule->genmask = (1 << net->nft.gencursor);
+}
+
+static inline void
+nft_rule_disactivate_next(struct net *net, struct nft_rule *rule)
+{
+ rule->genmask = (1 << gencursor_next(net));
+}
+
+static inline void nft_rule_clear(struct net *net, struct nft_rule *rule)
+{
+ rule->genmask = 0;
+}
+
static int nf_tables_dump_rules(struct sk_buff *skb,
struct netlink_callback *cb)
{
@@ -1250,6 +1288,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
unsigned int idx = 0, s_idx = cb->args[0];
struct net *net = sock_net(skb->sk);
int family = nfmsg->nfgen_family;
+ unsigned int genctr = net->nft.genctr, gencursor = net->nft.gencursor;
list_for_each_entry(afi, &net->nft.af_info, list) {
if (family != NFPROTO_UNSPEC && family != afi->family)
@@ -1258,6 +1297,8 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
list_for_each_entry(table, &afi->tables, list) {
list_for_each_entry(chain, &table->chains, list) {
list_for_each_entry(rule, &chain->rules, list) {
+ if (!nft_rule_is_active(net, rule))
+ goto cont;
if (idx < s_idx)
goto cont;
if (idx > s_idx)
@@ -1276,6 +1317,10 @@ cont:
}
}
done:
+ /* Invalidate this dump, a transition to the new generation happened */
+ if (gencursor != net->nft.gencursor || genctr != net->nft.genctr)
+ return -EBUSY;
+
cb->args[0] = idx;
return skb->len;
}
@@ -1376,6 +1421,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
unsigned int size, i, n;
int err, rem;
bool create;
+ u32 flags = 0;
u64 handle;
create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
@@ -1434,6 +1480,15 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
if (rule == NULL)
goto err1;
+ if (nla[NFTA_RULE_FLAGS]) {
+ flags = ntohl(nla_get_be32(nla[NFTA_RULE_FLAGS]));
+ if (flags & ~NFT_RULE_F_MASK)
+ return -EINVAL;
+
+ if (flags & NFT_RULE_F_COMMIT)
+ nft_rule_activate_next(net, rule);
+ }
+
rule->handle = handle;
rule->dlen = size;
@@ -1447,16 +1502,26 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
}
if (nlh->nlmsg_flags & NLM_F_REPLACE) {
- list_replace_rcu(&old_rule->list, &rule->list);
- nf_tables_rule_destroy(old_rule);
+ if (flags & NFT_RULE_F_COMMIT) {
+ nft_rule_disactivate_next(net, old_rule);
+ list_add_tail_rcu(&rule->list, &chain->rules);
+ } else {
+ list_replace_rcu(&old_rule->list, &rule->list);
+ nf_tables_rule_destroy(old_rule);
+ }
} else if (nlh->nlmsg_flags & NLM_F_APPEND)
list_add_tail_rcu(&rule->list, &chain->rules);
else
list_add_rcu(&rule->list, &chain->rules);
- nf_tables_rule_notify(skb, nlh, table, chain, rule, NFT_MSG_NEWRULE,
- nlh->nlmsg_flags & (NLM_F_APPEND | NLM_F_REPLACE),
- nfmsg->nfgen_family);
+ if (flags & NFT_RULE_F_COMMIT)
+ list_add(&rule->dirty_list, &chain->dirty_rules);
+ else {
+ nf_tables_rule_notify(skb, nlh, table, chain, rule,
+ NFT_MSG_NEWRULE,
+ nlh->nlmsg_flags & (NLM_F_APPEND | NLM_F_REPLACE),
+ nfmsg->nfgen_family);
+ }
return 0;
err2:
@@ -1469,6 +1534,23 @@ err1:
return err;
}
+static void
+nf_tables_delrule_one(struct nft_ctx *ctx, struct nft_rule *rule, u32 flags)
+{
+ if (flags & NFT_RULE_F_COMMIT) {
+ struct nft_chain *chain = (struct nft_chain *)ctx->chain;
+
+ nft_rule_disactivate_next(ctx->net, rule);
+ list_add(&rule->dirty_list, &chain->dirty_rules);
+ } else {
+ list_del_rcu(&rule->list);
+ nf_tables_rule_notify(ctx->skb, ctx->nlh, ctx->table,
+ ctx->chain, rule, NFT_MSG_DELRULE,
+ 0, ctx->afi->family);
+ nf_tables_rule_destroy(rule);
+ }
+}
+
static int nf_tables_delrule(struct sock *nlsk, struct sk_buff *skb,
const struct nlmsghdr *nlh,
const struct nlattr * const nla[])
@@ -1480,6 +1562,8 @@ static int nf_tables_delrule(struct sock *nlsk, struct sk_buff *skb,
struct nft_chain *chain;
struct nft_rule *rule, *tmp;
int family = nfmsg->nfgen_family;
+ struct nft_ctx ctx;
+ u32 flags = 0;
afi = nf_tables_afinfo_lookup(net, family, false);
if (IS_ERR(afi))
@@ -1493,31 +1577,132 @@ static int nf_tables_delrule(struct sock *nlsk, struct sk_buff *skb,
if (IS_ERR(chain))
return PTR_ERR(chain);
+ nft_ctx_init(&ctx, skb, nlh, afi, table, chain);
+
+ if (nla[NFTA_RULE_FLAGS]) {
+ flags = ntohl(nla_get_be32(nla[NFTA_RULE_FLAGS]));
+
+ if (flags & ~NFT_RULE_F_MASK)
+ return -EINVAL;
+ }
+
if (nla[NFTA_RULE_HANDLE]) {
rule = nf_tables_rule_lookup(chain, nla[NFTA_RULE_HANDLE]);
if (IS_ERR(rule))
return PTR_ERR(rule);
- /* List removal must be visible before destroying expressions */
- list_del_rcu(&rule->list);
-
- nf_tables_rule_notify(skb, nlh, table, chain, rule,
- NFT_MSG_DELRULE, 0, family);
- nf_tables_rule_destroy(rule);
+ nf_tables_delrule_one(&ctx, rule, flags);
} else {
/* Remove all rules in this chain */
- list_for_each_entry_safe(rule, tmp, &chain->rules, list) {
- list_del_rcu(&rule->list);
+ list_for_each_entry_safe(rule, tmp, &chain->rules, list)
+ nf_tables_delrule_one(&ctx, rule, flags);
+ }
- nf_tables_rule_notify(skb, nlh, table, chain, rule,
- NFT_MSG_DELRULE, 0, family);
- nf_tables_rule_destroy(rule);
+ return 0;
+}
+
+static int nf_tables_commit(struct sock *nlsk, struct sk_buff *skb,
+ const struct nlmsghdr *nlh,
+ const struct nlattr * const nla[])
+{
+ const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
+ const struct nft_af_info *afi;
+ struct net *net = sock_net(skb->sk);
+ struct nft_table *table;
+ struct nft_chain *chain;
+ struct nft_rule *rule, *tmp;
+ int family = nfmsg->nfgen_family;
+ bool create;
+
+ create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
+
+ afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
+ if (IS_ERR(afi))
+ return PTR_ERR(afi);
+
+ /* Bump generation counter, invalidate any dump in progress */
+ net->nft.genctr++;
+
+ /* A new generation has just started */
+ net->nft.gencursor++;
+
+ /* Make sure all packets have left the previous generation before
+ * purging old rules.
+ */
+ synchronize_rcu();
+
+ list_for_each_entry(table, &afi->tables, list) {
+ list_for_each_entry(chain, &table->chains, list) {
+ list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
+ /* Delete this rule from the dirty list */
+ list_del(&rule->dirty_list);
+
+ /* This rule was inactive in the past and just
+ * became active. Clear the next bit of the
+ * genmask since its meaning has changed, now
+ * it is the future.
+ */
+ if (nft_rule_is_active(net, rule)) {
+ nft_rule_clear(net, rule);
+ nf_tables_rule_notify(skb, nlh, table,
+ chain, rule,
+ NFT_MSG_NEWRULE,
+ 0,
+ nfmsg->nfgen_family);
+ continue;
+ }
+
+ /* This rule is in the past, get rid of it */
+ list_del_rcu(&rule->list);
+ nf_tables_rule_notify(skb, nlh, table, chain,
+ rule, NFT_MSG_DELRULE, 0,
+ family);
+ nf_tables_rule_destroy(rule);
+ }
}
}
return 0;
}
+static int nf_tables_abort(struct sock *nlsk, struct sk_buff *skb,
+ const struct nlmsghdr *nlh,
+ const struct nlattr * const nla[])
+{
+ const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
+ const struct nft_af_info *afi;
+ struct net *net = sock_net(skb->sk);
+ struct nft_table *table;
+ struct nft_chain *chain;
+ struct nft_rule *rule, *tmp;
+ bool create;
+
+ create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
+
+ afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
+ if (IS_ERR(afi))
+ return PTR_ERR(afi);
+
+ list_for_each_entry(table, &afi->tables, list) {
+ list_for_each_entry(chain, &table->chains, list) {
+ list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
+ /* Delete all rules from the dirty list */
+ list_del(&rule->dirty_list);
+
+ if (!nft_rule_is_active_next(net, rule)) {
+ nft_rule_clear(net, rule);
+ continue;
+ }
+
+ /* This rule is inactive, get rid of it */
+ list_del_rcu(&rule->list);
+ nf_tables_rule_destroy(rule);
+ }
+ }
+ }
+ return 0;
+}
+
/*
* Sets
*/
@@ -2477,6 +2662,16 @@ static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
.attr_count = NFTA_SET_ELEM_LIST_MAX,
.policy = nft_set_elem_list_policy,
},
+ [NFT_MSG_COMMIT] = {
+ .call = nf_tables_commit,
+ .attr_count = NFTA_TABLE_MAX,
+ .policy = nft_rule_policy,
+ },
+ [NFT_MSG_ABORT] = {
+ .call = nf_tables_abort,
+ .attr_count = NFTA_TABLE_MAX,
+ .policy = nft_rule_policy,
+ },
};
static const struct nfnetlink_subsystem nf_tables_subsys = {
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index b9917b7..a3f848f 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -72,12 +72,22 @@ nft_do_chain_pktinfo(struct nft_pktinfo *pkt, const struct nf_hook_ops *ops)
const struct nft_chain *chain;
const struct nft_rule *rule;
} jumpstack[NFT_JUMP_STACK_SIZE];
+ /*
+ * Cache cursor to avoid problems in case that the cursor is updated
+ * while traversing the ruleset.
+ */
+ unsigned int gencursor = chain->net->nft.gencursor;
do_chain:
rule = list_entry(&chain->rules, struct nft_rule, list);
next_rule:
data[NFT_REG_VERDICT].verdict = NFT_CONTINUE;
list_for_each_entry_continue_rcu(rule, &chain->rules, list) {
+
+ /* This rule is not active, skip. */
+ if (unlikely(rule->genmask & (1 << gencursor)))
+ continue;
+
nft_rule_for_each_expr(expr, last, rule) {
if (expr->ops == &nft_cmp_fast_ops)
nft_cmp_fast_eval(expr, data);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-01-31 0:03 ` [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps pablo
@ 2013-02-18 17:17 ` Tomasz Bursztyka
2013-02-20 1:12 ` Pablo Neira Ayuso
2013-02-19 22:02 ` Patrick McHardy
1 sibling, 1 reply; 16+ messages in thread
From: Tomasz Bursztyka @ 2013-02-18 17:17 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, kaber
Hi Pablo,
Hopefully you mentioned such patchset, went far away in my mail box.
I am fine with most of them but this particular one.
Some small parts should be reworked a bit.
The overall idea is nice, especially the bit field so it does not make
the nft_rule bigger.
> diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
> index 7640290..3749069 100644
> --- a/include/linux/netfilter/nf_tables.h
> +++ b/include/linux/netfilter/nf_tables.h
> @@ -37,6 +37,8 @@ enum nf_tables_msg_types {
> NFT_MSG_NEWSETELEM,
> NFT_MSG_GETSETELEM,
> NFT_MSG_DELSETELEM,
> + NFT_MSG_COMMIT,
> + NFT_MSG_ABORT,
> NFT_MSG_MAX,
> };
>
> @@ -85,12 +87,18 @@ enum nft_chain_attributes {
> };
> #define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
>
> +enum {
> + NFT_RULE_F_COMMIT = (1 << 0),
> + NFT_RULE_F_MASK = NFT_RULE_F_COMMIT,
> +};
> +
I guess you added this flags to add rules on non-transaction based
use-case, so it commits right away.
Wouldn't it be better to have no such flags in the API, but instead keep
track of an on going transaction in the struct nft_ctx (let's have an
internal flag here).
I.E: no on-going transaction -> we directly commit. If not, it's handled
as being part of the transaction.
I just took a look at nfnetlink however, it does not seem possible to
keep a data pointer per connection on the subsystem.
Wait, there is a field in struct sock that is meant for such usage. It
would require to change nf_tables_api.c only then.
Would it make sense or am I wrong with nft_ctx usage? (there would be
an issue: to free such context when the connection goes down, we could
abort an on-going transaction this way then)
> enum nft_rule_attributes {
> NFTA_RULE_UNSPEC,
> NFTA_RULE_TABLE,
> NFTA_RULE_CHAIN,
> NFTA_RULE_HANDLE,
> NFTA_RULE_EXPRESSIONS,
> + NFTA_RULE_FLAGS,
> __NFTA_RULE_MAX
> };
... then it would not require such extra arguments, we would keep a
simple api.
> * @rcu_head: used internally for rcu
> * @handle: rule handle
> + * @genmask: generation mask
> * @dlen: length of expression data
> * @data: expression data
> */
> struct nft_rule {
> struct list_head list;
> + struct list_head dirty_list;
> struct rcu_head rcu_head;
> - u64 handle:48,
> + u64 handle:46,
> + genmask:2,
> dlen:16;
> unsigned char data[]
> __attribute__((aligned(__alignof__(struct nft_expr))));
> @@ -366,8 +370,10 @@ enum nft_chain_flags {
> * struct nft_chain - nf_tables chain
> *
> * @rules: list of rules in the chain
> + * @dirty_rules: rules that need an update after next generation
See the end of the patch, on abort or commit function. I think we should
try to do something better (performance wise)
> * @list: used internally
> * @rcu_head: used internally
> + * @net: net namespace that this chain belongs to
I would see that in another patch, even if it's a really tiny one.
(preceding this current one)
Moreover that we will have to full support the namespaces at some point,
right?
> +static int nf_tables_commit(struct sock *nlsk, struct sk_buff *skb,
> + const struct nlmsghdr *nlh,
> + const struct nlattr * const nla[])
> +{
> + const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> + const struct nft_af_info *afi;
> + struct net *net = sock_net(skb->sk);
> + struct nft_table *table;
> + struct nft_chain *chain;
> + struct nft_rule *rule, *tmp;
> + int family = nfmsg->nfgen_family;
> + bool create;
> +
> + create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
> +
> + afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
> + if (IS_ERR(afi))
> + return PTR_ERR(afi);
> +
> + /* Bump generation counter, invalidate any dump in progress */
> + net->nft.genctr++;
> +
> + /* A new generation has just started */
> + net->nft.gencursor++;
> +
> + /* Make sure all packets have left the previous generation before
> + * purging old rules.
> + */
> + synchronize_rcu();
> +
> + list_for_each_entry(table, &afi->tables, list) {
> + list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
> + /* Delete this rule from the dirty list */
> + list_del(&rule->dirty_list);
Ok here it sounds we could do better. Instead of storing the dirty list
inside each chain, we may try an external one so we won't have to go
through the whole table/chain/rule base like we do in a dump. If such
dirty list is external (keeping a pointer on targeted table and chain
too for each rule), it will be possible to loop only on it.
And having this list_for_each_entry() make the line out of 80 chars
anyway. (there is the same issue for nft_dump_rules and some other)
> +static int nf_tables_abort(struct sock *nlsk, struct sk_buff *skb,
> + const struct nlmsghdr *nlh,
> + const struct nlattr * const nla[])
> +{
> + const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> + const struct nft_af_info *afi;
> + struct net *net = sock_net(skb->sk);
> + struct nft_table *table;
> + struct nft_chain *chain;
> + struct nft_rule *rule, *tmp;
> + bool create;
> +
> + create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
> +
> + afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
> + if (IS_ERR(afi))
> + return PTR_ERR(afi);
> +
> + list_for_each_entry(table, &afi->tables, list) {
> + list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
Same here.
Br,
Tomasz
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-02-18 17:17 ` Tomasz Bursztyka
@ 2013-02-20 1:12 ` Pablo Neira Ayuso
2013-02-20 8:16 ` Tomasz Bursztyka
0 siblings, 1 reply; 16+ messages in thread
From: Pablo Neira Ayuso @ 2013-02-20 1:12 UTC (permalink / raw)
To: Tomasz Bursztyka; +Cc: netfilter-devel, kaber
Hi Tomasz,
On Mon, Feb 18, 2013 at 07:17:56PM +0200, Tomasz Bursztyka wrote:
> Hi Pablo,
>
> Hopefully you mentioned such patchset, went far away in my mail box.
> I am fine with most of them but this particular one.
> Some small parts should be reworked a bit.
>
> The overall idea is nice, especially the bit field so it does not
> make the nft_rule bigger.
>
> >diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
> >index 7640290..3749069 100644
> >--- a/include/linux/netfilter/nf_tables.h
> >+++ b/include/linux/netfilter/nf_tables.h
> >@@ -37,6 +37,8 @@ enum nf_tables_msg_types {
> > NFT_MSG_NEWSETELEM,
> > NFT_MSG_GETSETELEM,
> > NFT_MSG_DELSETELEM,
> >+ NFT_MSG_COMMIT,
> >+ NFT_MSG_ABORT,
> > NFT_MSG_MAX,
> > };
> >@@ -85,12 +87,18 @@ enum nft_chain_attributes {
> > };
> > #define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
> >+enum {
> >+ NFT_RULE_F_COMMIT = (1 << 0),
> >+ NFT_RULE_F_MASK = NFT_RULE_F_COMMIT,
> >+};
> >+
>
> I guess you added this flags to add rules on non-transaction based
> use-case, so it commits right away.
> Wouldn't it be better to have no such flags in the API, but instead
> keep track of an on going transaction in the struct nft_ctx (let's
> have an internal flag here).
> I.E: no on-going transaction -> we directly commit. If not, it's
> handled as being part of the transaction.
>
> I just took a look at nfnetlink however, it does not seem possible
> to keep a data pointer per connection on the subsystem.
> Wait, there is a field in struct sock that is meant for such usage.
> It would require to change nf_tables_api.c only then.
> Would it make sense or am I wrong with nft_ctx usage? (there would
> be an issue: to free such context when the connection goes down, we
> could abort an on-going transaction this way then)
We can define some container structure to store rules in the dirty
list:
struct nft_rule_update {
struct list_head head;
uint32_t nl_portid;
struct nft_rule *rule;
struct nft_table *table;
struct nft_chain *chain;
}
That should allows us to remove the struct list_head dirty_list in
struct nft_rule that I needed for this.
The nl_portid would be the netlink portid so we know what updates
belong to what netlink connection. Still I don't see how to get rid of
the commit flag.
Could you develop your idea?
> > enum nft_rule_attributes {
> > NFTA_RULE_UNSPEC,
> > NFTA_RULE_TABLE,
> > NFTA_RULE_CHAIN,
> > NFTA_RULE_HANDLE,
> > NFTA_RULE_EXPRESSIONS,
> >+ NFTA_RULE_FLAGS,
> > __NFTA_RULE_MAX
> > };
>
> ... then it would not require such extra arguments, we would keep a
> simple api.
>
> > * @rcu_head: used internally for rcu
> > * @handle: rule handle
> >+ * @genmask: generation mask
> > * @dlen: length of expression data
> > * @data: expression data
> > */
> > struct nft_rule {
> > struct list_head list;
> >+ struct list_head dirty_list;
> > struct rcu_head rcu_head;
> >- u64 handle:48,
> >+ u64 handle:46,
> >+ genmask:2,
> > dlen:16;
> > unsigned char data[]
> > __attribute__((aligned(__alignof__(struct nft_expr))));
> >@@ -366,8 +370,10 @@ enum nft_chain_flags {
> > * struct nft_chain - nf_tables chain
> > *
> > * @rules: list of rules in the chain
> >+ * @dirty_rules: rules that need an update after next generation
>
> See the end of the patch, on abort or commit function. I think we
> should try to do something better (performance wise)
>
> > * @list: used internally
> > * @rcu_head: used internally
> >+ * @net: net namespace that this chain belongs to
>
> I would see that in another patch, even if it's a really tiny one.
> (preceding this current one)
> Moreover that we will have to full support the namespaces at some
> point, right?
I need that net for the code in nft_do_chain_pktinfo added in this
patch. Probably it can be added to base chains only, would need to
check that.
> >+static int nf_tables_commit(struct sock *nlsk, struct sk_buff *skb,
> >+ const struct nlmsghdr *nlh,
> >+ const struct nlattr * const nla[])
> >+{
> >+ const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> >+ const struct nft_af_info *afi;
> >+ struct net *net = sock_net(skb->sk);
> >+ struct nft_table *table;
> >+ struct nft_chain *chain;
> >+ struct nft_rule *rule, *tmp;
> >+ int family = nfmsg->nfgen_family;
> >+ bool create;
> >+
> >+ create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
> >+
> >+ afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
> >+ if (IS_ERR(afi))
> >+ return PTR_ERR(afi);
> >+
> >+ /* Bump generation counter, invalidate any dump in progress */
> >+ net->nft.genctr++;
> >+
> >+ /* A new generation has just started */
> >+ net->nft.gencursor++;
> >+
> >+ /* Make sure all packets have left the previous generation before
> >+ * purging old rules.
> >+ */
> >+ synchronize_rcu();
> >+
> >+ list_for_each_entry(table, &afi->tables, list) {
> >+ list_for_each_entry(chain, &table->chains, list) {
> >+ list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
> >+ /* Delete this rule from the dirty list */
> >+ list_del(&rule->dirty_list);
>
> Ok here it sounds we could do better. Instead of storing the dirty
> list inside each chain, we may try an external one so we won't have
> to go through the whole table/chain/rule base like we do in a dump.
> If such dirty list is external (keeping a pointer on targeted table
> and chain too for each rule), it will be possible to loop only on
> it.
Yes, that's doable. We can store a pointer to the table and the chain
in the nft_rule_update container structure that I proposed above.
> And having this list_for_each_entry() make the line out of 80 chars
> anyway. (there is the same issue for nft_dump_rules and some other)
>
> >+static int nf_tables_abort(struct sock *nlsk, struct sk_buff *skb,
> >+ const struct nlmsghdr *nlh,
> >+ const struct nlattr * const nla[])
> >+{
> >+ const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> >+ const struct nft_af_info *afi;
> >+ struct net *net = sock_net(skb->sk);
> >+ struct nft_table *table;
> >+ struct nft_chain *chain;
> >+ struct nft_rule *rule, *tmp;
> >+ bool create;
> >+
> >+ create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
> >+
> >+ afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
> >+ if (IS_ERR(afi))
> >+ return PTR_ERR(afi);
> >+
> >+ list_for_each_entry(table, &afi->tables, list) {
> >+ list_for_each_entry(chain, &table->chains, list) {
> >+ list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
>
> Same here.
>
>
> Br,
>
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-02-20 1:12 ` Pablo Neira Ayuso
@ 2013-02-20 8:16 ` Tomasz Bursztyka
2013-02-20 23:10 ` Pablo Neira Ayuso
0 siblings, 1 reply; 16+ messages in thread
From: Tomasz Bursztyka @ 2013-02-20 8:16 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel, kaber
Hi Pablo,
>>
>>> diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
>>> index 7640290..3749069 100644
>>> --- a/include/linux/netfilter/nf_tables.h
>>> +++ b/include/linux/netfilter/nf_tables.h
>>> @@ -37,6 +37,8 @@ enum nf_tables_msg_types {
>>> NFT_MSG_NEWSETELEM,
>>> NFT_MSG_GETSETELEM,
>>> NFT_MSG_DELSETELEM,
>>> + NFT_MSG_COMMIT,
>>> + NFT_MSG_ABORT,
>>> NFT_MSG_MAX,
>>> };
>>> @@ -85,12 +87,18 @@ enum nft_chain_attributes {
>>> };
>>> #define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
>>> +enum {
>>> + NFT_RULE_F_COMMIT = (1 << 0),
>>> + NFT_RULE_F_MASK = NFT_RULE_F_COMMIT,
>>> +};
>>> +
>> I guess you added this flags to add rules on non-transaction based
>> use-case, so it commits right away.
>> Wouldn't it be better to have no such flags in the API, but instead
>> keep track of an on going transaction in the struct nft_ctx (let's
>> have an internal flag here).
>> I.E: no on-going transaction -> we directly commit. If not, it's
>> handled as being part of the transaction.
>>
>> I just took a look at nfnetlink however, it does not seem possible
>> to keep a data pointer per connection on the subsystem.
>> Wait, there is a field in struct sock that is meant for such usage.
>> It would require to change nf_tables_api.c only then.
>> Would it make sense or am I wrong with nft_ctx usage? (there would
>> be an issue: to free such context when the connection goes down, we
>> could abort an on-going transaction this way then)
> We can define some container structure to store rules in the dirty
> list:
>
> struct nft_rule_update {
> struct list_head head;
> uint32_t nl_portid;
> struct nft_rule *rule;
> struct nft_table *table;
> struct nft_chain *chain;
> }
>
> That should allows us to remove the struct list_head dirty_list in
> struct nft_rule that I needed for this.
>
> The nl_portid would be the netlink portid so we know what updates
> belong to what netlink connection. Still I don't see how to get rid of
> the commit flag.
>
> Could you develop your idea?
I was exactly thinking about such external list. But it would be tighten
to the netlink connection more deeply: as a user data to the socket,
instead of storing the nl_portid.
I will explain below why.
To me iptables-nftables is a non-transaction based tool. There is no
point to start a transaction for one rule.
Btw it would then require a NFT_MSG_START or some sort, to start the
transaction based manipulation.
Let's say now you have a software, which require to do rules
manipulation, very often, which will be always connected through netlink
to nftables.
starts a transaction:
- manip 1
- manip 2
- ...
- manip n
Commit or Abort.
done.
Now, for whatever reason: the software crashes in the middle of a
transaction. When it restarts it has no idea what it was doing and why.
Here is why we should tight the transaction per netlink connection: if
the connections breaks, we abort right away. It's transparent.
It's consistent also with the fact that you raise a notification only
when the rule has been activated. Until it comes: no one knows about those
rules in the transaction but the one who owns the transaction.
We could do that via the struct sock { ... user_data ... }; related to
the netlink connection, storing the transaction list.
Now, no need of a flag: if the transaction list for the current netlink
connection is not NULL: you know you are on a transaction-based work, so
whatever manipulation comes: it will be part of the transaction. If it's
NULL, you do as usual: activating the rule right away.
>
>>> * @list: used internally
>>> * @rcu_head: used internally
>>> + * @net: net namespace that this chain belongs to
>> I would see that in another patch, even if it's a really tiny one.
>> (preceding this current one)
>> Moreover that we will have to full support the namespaces at some
>> point, right?
> I need that net for the code in nft_do_chain_pktinfo added in this
> patch. Probably it can be added to base chains only, would need to
> check that.
Indeed. I missed that.
Tomasz
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-02-20 8:16 ` Tomasz Bursztyka
@ 2013-02-20 23:10 ` Pablo Neira Ayuso
0 siblings, 0 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2013-02-20 23:10 UTC (permalink / raw)
To: Tomasz Bursztyka; +Cc: netfilter-devel, kaber
Hi Tomasz,
On Wed, Feb 20, 2013 at 10:16:29AM +0200, Tomasz Bursztyka wrote:
[...]
> >We can define some container structure to store rules in the dirty
> >list:
> >
> >struct nft_rule_update {
> > struct list_head head;
> > uint32_t nl_portid;
> > struct nft_rule *rule;
> > struct nft_table *table;
> > struct nft_chain *chain;
> >}
> >
> >That should allows us to remove the struct list_head dirty_list in
> >struct nft_rule that I needed for this.
> >
> >The nl_portid would be the netlink portid so we know what updates
> >belong to what netlink connection. Still I don't see how to get rid of
> >the commit flag.
> >
> >Could you develop your idea?
>
> I was exactly thinking about such external list. But it would be
> tighten to the netlink connection more deeply: as a user data to the
> socket, instead of storing the nl_portid.
> I will explain below why.
>
> To me iptables-nftables is a non-transaction based tool. There is no
> point to start a transaction for one rule.
Agreed, ie. Not for iptables, only for iptables-restore.
> Btw it would then require a NFT_MSG_START or some sort, to start
> the transaction based manipulation.
What I had in mind is that the transaction is considered to implicitly
start once you open a netlink socket from user-space, you add rule
updates that you want to happen atomically and then you call commit.
> Let's say now you have a software, which require to do rules
> manipulation, very often, which will be always connected through
> netlink to nftables.
> starts a transaction:
> - manip 1
> - manip 2
> - ...
> - manip n
> Commit or Abort.
>
> done.
>
> Now, for whatever reason: the software crashes in the middle of a
> transaction. When it restarts it has no idea what it was doing and
> why. Here is why we should tight the transaction per netlink
> connection: if the connections breaks, we abort right away. It's
> transparent.
Good point, I have a patch for that as well. We can catch the
NETLINK_URELEASE event to remove all entries in the dirty list for the
program that crashed / exited without committing.
See net/netfilter/nfnetlink_queue_core.c for instance.
> It's consistent also with the fact that you raise a notification
> only when the rule has been activated. Until it comes: no one knows
> about those rules in the transaction but the one who owns the
> transaction.
>
> We could do that via the struct sock { ... user_data ... }; related
> to the netlink connection, storing the transaction list.
>
> Now, no need of a flag: if the transaction list for the current
> netlink connection is not NULL: you know you are on a
> transaction-based work.
> whatever manipulation comes: it will be part of the transaction. If
> it's NULL, you do as usual: activating the rule right away.
This reminds me that we can use netlink's NLM_F_ATOMIC and remove
that flags.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-01-31 0:03 ` [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps pablo
2013-02-18 17:17 ` Tomasz Bursztyka
@ 2013-02-19 22:02 ` Patrick McHardy
2013-02-20 0:44 ` Pablo Neira Ayuso
2013-02-20 10:32 ` Tomasz Bursztyka
1 sibling, 2 replies; 16+ messages in thread
From: Patrick McHardy @ 2013-02-19 22:02 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, tomasz.bursztyka
On Thu, Jan 31, 2013 at 01:03:59AM +0100, pablo@netfilter.org wrote:
> From: Pablo Neira Ayuso <pablo@netfilter.org>
>
> This patch adds global atomic rule updates and dumps based on
> bitmask generations. This allows to atomically commit a set of
> rule-set updates incrementally without altering the internal
> state of existing nf_tables expressions/matches/targets.
>
> The idea consists of using a generation cursor of 1 bit and
> a bitmask of 2 bits per rule. Assuming the gencursor is 0,
> then the genmask (expressed as a bitmask) can be interpreted
> as:
>
> 00 active in the present, will be active in the next generation.
> 01 inactive in the present, will be active in the next generation.
> 10 active in the present, will be deleted in the next generation.
> ^
> gencursor
>
> Once you invoke the transition to the next generation, the global
> gencursor is updated:
>
> 00 active in the present, will be active in the next generation.
> 01 active in the present, needs to zero its future, it becomes 00.
> 10 inactive in the present, delete now.
> ^
> gencursor
>
> If a dump is in progress and nf_tables enters a new generation,
> the dump will stop and return -EBUSY to let userspace know that
> it has to retry again. In order to invalidate dumps, a global
> genctr counter is increased everytime nf_tables enters a new
> generation.
>
> This new operation can be used from the user-space utility
> that controls the firewall, eg.
>
> nft restore < file
>
> The rule updates contained in `file' will be applied atomically.
>
> cat file
> -----
> add filter INPUT ip saddr 1.1.1.1 counter accept #1
> del filter INPUT ip daddr 2.2.2.2 counter drop #2
> commit #3
> -EOF-
>
> Note that the rule 1 will be inactive until the transition to the
> next generation, the rule 2 will be evicted in the next generation.
>
> There is a penalty during the rule update due to the branch
> misprediction in the packet matching framework. But that should be
> quickly resolved once the iteration over the dirty list that
> contain rules that require updates is finished.
>
> Event notification happens once the rule-set update has been
> committed. So we skip notifications is case the rule-set update
> is aborted, which can happen in case that the rule-set is tested
> to apply correctly.
>
> This patch is based on ideas extracted from discussions with
> Patrick McHardy.
Generally, I think this has a pretty high penalty both memory-wise
and performance-wise, so we should reconsider the idea of moving
most of the costs to userspace by temporarily adding "skip" rules
that skip over uncommitted rules.
A couple of comments on the patch below though.
> @@ -37,6 +37,8 @@ enum nf_tables_msg_types {
> NFT_MSG_NEWSETELEM,
> NFT_MSG_GETSETELEM,
> NFT_MSG_DELSETELEM,
> + NFT_MSG_COMMIT,
> + NFT_MSG_ABORT,
I see why you're doing this, but this is similar to the NEWCHAINNAME
attribute, it doesn't really fit into the scheme how the netlink API
is designed.
> index 3ba63b6..1131e49 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -319,15 +319,19 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
> * struct nft_rule - nf_tables rule
> *
> * @list: used internally
> + * @dirty_list: this rule needs an update after new generation
> * @rcu_head: used internally for rcu
> * @handle: rule handle
> + * @genmask: generation mask
> * @dlen: length of expression data
> * @data: expression data
> */
> struct nft_rule {
> struct list_head list;
> + struct list_head dirty_list;
This is a quite heavy price. To reduce memory overhead, we could just have
dirty chains and iterate over all rules within them.
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index db6150b..7f08381 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> static int nf_tables_dump_rules(struct sk_buff *skb,
> struct netlink_callback *cb)
> {
> @@ -1250,6 +1288,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
> unsigned int idx = 0, s_idx = cb->args[0];
> struct net *net = sock_net(skb->sk);
> int family = nfmsg->nfgen_family;
> + unsigned int genctr = net->nft.genctr, gencursor = net->nft.gencursor;
>
> list_for_each_entry(afi, &net->nft.af_info, list) {
> if (family != NFPROTO_UNSPEC && family != afi->family)
> @@ -1258,6 +1297,8 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
> list_for_each_entry(table, &afi->tables, list) {
> list_for_each_entry(chain, &table->chains, list) {
> list_for_each_entry(rule, &chain->rules, list) {
> + if (!nft_rule_is_active(net, rule))
> + goto cont;
Not sure whether we should really skip them during the dump. That hides
information from userspace, we could just as well include an INACTIVE flag
and have userspace decide whether to process them or not.
> +static void
> +nf_tables_delrule_one(struct nft_ctx *ctx, struct nft_rule *rule, u32 flags)
> +{
> + if (flags & NFT_RULE_F_COMMIT) {
> + struct nft_chain *chain = (struct nft_chain *)ctx->chain;
> +
> + nft_rule_disactivate_next(ctx->net, rule);
deactivate :)
> + list_add(&rule->dirty_list, &chain->dirty_rules);
> + } else {
> + list_del_rcu(&rule->list);
> + nf_tables_rule_notify(ctx->skb, ctx->nlh, ctx->table,
> + ctx->chain, rule, NFT_MSG_DELRULE,
> + 0, ctx->afi->family);
> + nf_tables_rule_destroy(rule);
> + }
> +}
> +
> static int nf_tables_delrule(struct sock *nlsk, struct sk_buff *skb,
> const struct nlmsghdr *nlh,
> const struct nlattr * const nla[])
> +static int nf_tables_commit(struct sock *nlsk, struct sk_buff *skb,
> + const struct nlmsghdr *nlh,
> + const struct nlattr * const nla[])
> +{
> + const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> + const struct nft_af_info *afi;
> + struct net *net = sock_net(skb->sk);
> + struct nft_table *table;
> + struct nft_chain *chain;
> + struct nft_rule *rule, *tmp;
> + int family = nfmsg->nfgen_family;
> + bool create;
> +
> + create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
> +
> + afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
> + if (IS_ERR(afi))
> + return PTR_ERR(afi);
> +
> + /* Bump generation counter, invalidate any dump in progress */
> + net->nft.genctr++;
> +
> + /* A new generation has just started */
> + net->nft.gencursor++;
> +
> + /* Make sure all packets have left the previous generation before
> + * purging old rules.
> + */
> + synchronize_rcu();
This doesn't work. As soon as synchronize_rcu() is finished, new packets
might enter nft_do_chain(). And this also seems to be the main problem
with this approach, you can't iteratively flip the active bits without
locking out packet processing, otherwise it will be just as non-atomic as
it is now. For this to work you'd need a bigger generation counter in the
rules that doesn't overflow and doesn't require changes to its values.
> +
> + list_for_each_entry(table, &afi->tables, list) {
> + list_for_each_entry(chain, &table->chains, list) {
> + list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
> + /* Delete this rule from the dirty list */
> + list_del(&rule->dirty_list);
> +
> + /* This rule was inactive in the past and just
> + * became active. Clear the next bit of the
> + * genmask since its meaning has changed, now
> + * it is the future.
> + */
> + if (nft_rule_is_active(net, rule)) {
> + nft_rule_clear(net, rule);
> + nf_tables_rule_notify(skb, nlh, table,
> + chain, rule,
> + NFT_MSG_NEWRULE,
> + 0,
> + nfmsg->nfgen_family);
> + continue;
> + }
> +
> + /* This rule is in the past, get rid of it */
> + list_del_rcu(&rule->list);
> + nf_tables_rule_notify(skb, nlh, table, chain,
> + rule, NFT_MSG_DELRULE, 0,
> + family);
> + nf_tables_rule_destroy(rule);
> + }
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-02-19 22:02 ` Patrick McHardy
@ 2013-02-20 0:44 ` Pablo Neira Ayuso
2013-02-20 10:32 ` Tomasz Bursztyka
1 sibling, 0 replies; 16+ messages in thread
From: Pablo Neira Ayuso @ 2013-02-20 0:44 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netfilter-devel, tomasz.bursztyka
Hi Patrick,
On Tue, Feb 19, 2013 at 11:02:58PM +0100, Patrick McHardy wrote:
> On Thu, Jan 31, 2013 at 01:03:59AM +0100, pablo@netfilter.org wrote:
> > From: Pablo Neira Ayuso <pablo@netfilter.org>
> >
> > This patch adds global atomic rule updates and dumps based on
> > bitmask generations. This allows to atomically commit a set of
> > rule-set updates incrementally without altering the internal
> > state of existing nf_tables expressions/matches/targets.
> >
> > The idea consists of using a generation cursor of 1 bit and
> > a bitmask of 2 bits per rule. Assuming the gencursor is 0,
> > then the genmask (expressed as a bitmask) can be interpreted
> > as:
> >
> > 00 active in the present, will be active in the next generation.
> > 01 inactive in the present, will be active in the next generation.
> > 10 active in the present, will be deleted in the next generation.
> > ^
> > gencursor
> >
> > Once you invoke the transition to the next generation, the global
> > gencursor is updated:
> >
> > 00 active in the present, will be active in the next generation.
> > 01 active in the present, needs to zero its future, it becomes 00.
> > 10 inactive in the present, delete now.
> > ^
> > gencursor
> >
> > If a dump is in progress and nf_tables enters a new generation,
> > the dump will stop and return -EBUSY to let userspace know that
> > it has to retry again. In order to invalidate dumps, a global
> > genctr counter is increased everytime nf_tables enters a new
> > generation.
> >
> > This new operation can be used from the user-space utility
> > that controls the firewall, eg.
> >
> > nft restore < file
> >
> > The rule updates contained in `file' will be applied atomically.
> >
> > cat file
> > -----
> > add filter INPUT ip saddr 1.1.1.1 counter accept #1
> > del filter INPUT ip daddr 2.2.2.2 counter drop #2
> > commit #3
> > -EOF-
> >
> > Note that the rule 1 will be inactive until the transition to the
> > next generation, the rule 2 will be evicted in the next generation.
> >
> > There is a penalty during the rule update due to the branch
> > misprediction in the packet matching framework. But that should be
> > quickly resolved once the iteration over the dirty list that
> > contain rules that require updates is finished.
> >
> > Event notification happens once the rule-set update has been
> > committed. So we skip notifications is case the rule-set update
> > is aborted, which can happen in case that the rule-set is tested
> > to apply correctly.
> >
> > This patch is based on ideas extracted from discussions with
> > Patrick McHardy.
>
> Generally, I think this has a pretty high penalty both memory-wise
> and performance-wise, so we should reconsider the idea of moving
> most of the costs to userspace by temporarily adding "skip" rules
> that skip over uncommitted rules.
>
> A couple of comments on the patch below though.
>
> > @@ -37,6 +37,8 @@ enum nf_tables_msg_types {
> > NFT_MSG_NEWSETELEM,
> > NFT_MSG_GETSETELEM,
> > NFT_MSG_DELSETELEM,
> > + NFT_MSG_COMMIT,
> > + NFT_MSG_ABORT,
>
> I see why you're doing this, but this is similar to the NEWCHAINNAME
> attribute, it doesn't really fit into the scheme how the netlink API
> is designed.
What would you suggest as an alternative?
> > index 3ba63b6..1131e49 100644
> > --- a/include/net/netfilter/nf_tables.h
> > +++ b/include/net/netfilter/nf_tables.h
> > @@ -319,15 +319,19 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
> > * struct nft_rule - nf_tables rule
> > *
> > * @list: used internally
> > + * @dirty_list: this rule needs an update after new generation
> > * @rcu_head: used internally for rcu
> > * @handle: rule handle
> > + * @genmask: generation mask
> > * @dlen: length of expression data
> > * @data: expression data
> > */
> > struct nft_rule {
> > struct list_head list;
> > + struct list_head dirty_list;
>
> This is a quite heavy price. To reduce memory overhead, we could just have
> dirty chains and iterate over all rules within them.
We can save that the extra struct list_head for the dirty_list in the
rule by allocating temporary container structure, I have a follow up
patch for that.
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index db6150b..7f08381 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > static int nf_tables_dump_rules(struct sk_buff *skb,
> > struct netlink_callback *cb)
> > {
> > @@ -1250,6 +1288,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
> > unsigned int idx = 0, s_idx = cb->args[0];
> > struct net *net = sock_net(skb->sk);
> > int family = nfmsg->nfgen_family;
> > + unsigned int genctr = net->nft.genctr, gencursor = net->nft.gencursor;
> >
> > list_for_each_entry(afi, &net->nft.af_info, list) {
> > if (family != NFPROTO_UNSPEC && family != afi->family)
> > @@ -1258,6 +1297,8 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
> > list_for_each_entry(table, &afi->tables, list) {
> > list_for_each_entry(chain, &table->chains, list) {
> > list_for_each_entry(rule, &chain->rules, list) {
> > + if (!nft_rule_is_active(net, rule))
> > + goto cont;
>
> Not sure whether we should really skip them during the dump. That hides
> information from userspace, we could just as well include an INACTIVE flag
> and have userspace decide whether to process them or not.
Agreed, they should be dumped.
> > +static void
> > +nf_tables_delrule_one(struct nft_ctx *ctx, struct nft_rule *rule, u32 flags)
> > +{
> > + if (flags & NFT_RULE_F_COMMIT) {
> > + struct nft_chain *chain = (struct nft_chain *)ctx->chain;
> > +
> > + nft_rule_disactivate_next(ctx->net, rule);
>
> deactivate :)
Hm, I swear I checked this in the dictionary before writing it, you
know how broken my English can be ;-)
> > + list_add(&rule->dirty_list, &chain->dirty_rules);
> > + } else {
> > + list_del_rcu(&rule->list);
> > + nf_tables_rule_notify(ctx->skb, ctx->nlh, ctx->table,
> > + ctx->chain, rule, NFT_MSG_DELRULE,
> > + 0, ctx->afi->family);
> > + nf_tables_rule_destroy(rule);
> > + }
> > +}
> > +
> > static int nf_tables_delrule(struct sock *nlsk, struct sk_buff *skb,
> > const struct nlmsghdr *nlh,
> > const struct nlattr * const nla[])
> > +static int nf_tables_commit(struct sock *nlsk, struct sk_buff *skb,
> > + const struct nlmsghdr *nlh,
> > + const struct nlattr * const nla[])
> > +{
> > + const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> > + const struct nft_af_info *afi;
> > + struct net *net = sock_net(skb->sk);
> > + struct nft_table *table;
> > + struct nft_chain *chain;
> > + struct nft_rule *rule, *tmp;
> > + int family = nfmsg->nfgen_family;
> > + bool create;
> > +
> > + create = nlh->nlmsg_flags & NLM_F_CREATE ? true : false;
> > +
> > + afi = nf_tables_afinfo_lookup(net, nfmsg->nfgen_family, create);
> > + if (IS_ERR(afi))
> > + return PTR_ERR(afi);
> > +
> > + /* Bump generation counter, invalidate any dump in progress */
> > + net->nft.genctr++;
> > +
> > + /* A new generation has just started */
> > + net->nft.gencursor++;
> > +
> > + /* Make sure all packets have left the previous generation before
> > + * purging old rules.
> > + */
> > + synchronize_rcu();
>
> This doesn't work. As soon as synchronize_rcu() is finished, new packets
> might enter nft_do_chain(). And this also seems to be the main problem
> with this approach, you can't iteratively flip the active bits without
> locking out packet processing, otherwise it will be just as non-atomic as
> it is now. For this to work you'd need a bigger generation counter in the
> rules that doesn't overflow and doesn't require changes to its values.
The gencursor is being cached at the entrace of nft_do_chain_pktinfo to
avoid that intermediate state. So AFAICS we wait until old packets
have finished iterating over the rule-set with the old gencursor and,
then, new packets will see the new rule-set using the new gencursor.
> > + list_for_each_entry(table, &afi->tables, list) {
> > + list_for_each_entry(chain, &table->chains, list) {
> > + list_for_each_entry_safe(rule, tmp, &chain->dirty_rules, dirty_list) {
> > + /* Delete this rule from the dirty list */
> > + list_del(&rule->dirty_list);
> > +
> > + /* This rule was inactive in the past and just
> > + * became active. Clear the next bit of the
> > + * genmask since its meaning has changed, now
> > + * it is the future.
> > + */
> > + if (nft_rule_is_active(net, rule)) {
> > + nft_rule_clear(net, rule);
> > + nf_tables_rule_notify(skb, nlh, table,
> > + chain, rule,
> > + NFT_MSG_NEWRULE,
> > + 0,
> > + nfmsg->nfgen_family);
> > + continue;
> > + }
> > +
> > + /* This rule is in the past, get rid of it */
> > + list_del_rcu(&rule->list);
> > + nf_tables_rule_notify(skb, nlh, table, chain,
> > + rule, NFT_MSG_DELRULE, 0,
> > + family);
> > + nf_tables_rule_destroy(rule);
> > + }
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps
2013-02-19 22:02 ` Patrick McHardy
2013-02-20 0:44 ` Pablo Neira Ayuso
@ 2013-02-20 10:32 ` Tomasz Bursztyka
1 sibling, 0 replies; 16+ messages in thread
From: Tomasz Bursztyka @ 2013-02-20 10:32 UTC (permalink / raw)
To: Patrick McHardy; +Cc: pablo, netfilter-devel
Hi Patrick,
>
>> @@ -37,6 +37,8 @@ enum nf_tables_msg_types {
>> NFT_MSG_NEWSETELEM,
>> NFT_MSG_GETSETELEM,
>> NFT_MSG_DELSETELEM,
>> + NFT_MSG_COMMIT,
>> + NFT_MSG_ABORT,
> I see why you're doing this, but this is similar to the NEWCHAINNAME
> attribute, it doesn't really fit into the scheme how the netlink API
> is designed.
Why doesn't it fit? Do you mean you would prefer one command leads to
one result, so no transaction based manipulation?
I would rather see a NFT_MSG_START added actually.
>
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index db6150b..7f08381 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> static int nf_tables_dump_rules(struct sk_buff *skb,
>> struct netlink_callback *cb)
>> {
>> @@ -1250,6 +1288,7 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
>> unsigned int idx = 0, s_idx = cb->args[0];
>> struct net *net = sock_net(skb->sk);
>> int family = nfmsg->nfgen_family;
>> + unsigned int genctr = net->nft.genctr, gencursor = net->nft.gencursor;
>>
>> list_for_each_entry(afi, &net->nft.af_info, list) {
>> if (family != NFPROTO_UNSPEC && family != afi->family)
>> @@ -1258,6 +1297,8 @@ static int nf_tables_dump_rules(struct sk_buff *skb,
>> list_for_each_entry(table, &afi->tables, list) {
>> list_for_each_entry(chain, &table->chains, list) {
>> list_for_each_entry(rule, &chain->rules, list) {
>> + if (!nft_rule_is_active(net, rule))
>> + goto cont;
> Not sure whether we should really skip them during the dump. That hides
> information from userspace, we could just as well include an INACTIVE flag
> and have userspace decide whether to process them or not.
Personally I don't see much point dumping non-active (yet to be
committed) rules to anybody.
The current way of notifying the rule change when committed, as
described at the end of this patch message, is I think just right.
That would be better though within the proposal I made earlier to Pablo.
Unless you don't want transaction based manipulation. What would be the
idea then?
Tomasz
^ permalink raw reply [flat|nested] 16+ messages in thread
* [nftables 4/9] netfilter: nf_tables: fix error path in newchain
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
2013-01-31 0:03 ` [nftables 2/9] netfilter: nf_tables: fix chain after rule deletion pablo
2013-01-31 0:03 ` [nftables 3/9] netfilter: nf_tables: atomic rule updates and dumps pablo
@ 2013-01-31 0:04 ` pablo
2013-01-31 0:04 ` [nftables 5/9] netfilter: nf_tables: add packet and byte counters per chain pablo
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
Make sure the chain is not in the table list until the hook
registration returns success.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_tables_api.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 7f08381..031aad9 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -868,8 +868,6 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
chain->net = net;
nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);
- list_add_tail(&chain->list, &table->chains);
-
if (!(table->flags & NFT_TABLE_F_DORMANT) &&
chain->flags & NFT_BASE_CHAIN) {
err = nf_register_hook(&nft_base_chain(chain)->ops);
@@ -878,6 +876,7 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
return err;
}
}
+ list_add_tail(&chain->list, &table->chains);
notify:
nf_tables_chain_notify(skb, nlh, table, chain, NFT_MSG_NEWCHAIN,
family);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [nftables 5/9] netfilter: nf_tables: add packet and byte counters per chain
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
` (2 preceding siblings ...)
2013-01-31 0:04 ` [nftables 4/9] netfilter: nf_tables: fix error path in newchain pablo
@ 2013-01-31 0:04 ` pablo
2013-01-31 0:04 ` [nftables 6/9] netfilter: nf_tables: add protocol and flags for xtables over nf_tables pablo
` (3 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
This patch adds per-cpu chain counters. Only base chains are allowed
to have them to emulate x_tables behaviour.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/linux/netfilter/nf_tables.h | 1 +
include/net/netfilter/nf_tables.h | 7 +++
net/netfilter/nf_tables_api.c | 100 +++++++++++++++++++++++++++++++++++
net/netfilter/nf_tables_core.c | 9 ++++
4 files changed, 117 insertions(+)
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index 3749069..ccd2c30 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -83,6 +83,7 @@ enum nft_chain_attributes {
NFTA_CHAIN_POLICY,
NFTA_CHAIN_USE,
NFTA_CHAIN_TYPE,
+ NFTA_CHAIN_COUNTERS,
__NFTA_CHAIN_MAX
};
#define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 1131e49..a9df310 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -400,18 +400,25 @@ enum nft_chain_type {
NFT_CHAIN_T_MAX
};
+struct nft_stats {
+ u64 bytes;
+ u64 pkts;
+};
+
/**
* struct nft_base_chain - nf_tables base chain
*
* @ops: netfilter hook ops
* @type: chain type
* @policy: default policy
+ * @stats: per-cpu chain stats
* @chain: the chain
*/
struct nft_base_chain {
struct nf_hook_ops ops;
enum nft_chain_type type;
u8 policy;
+ struct nft_stats __percpu *stats;
struct nft_chain chain;
};
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 031aad9..141e33e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -529,6 +529,33 @@ static const struct nla_policy nft_hook_policy[NFTA_HOOK_MAX + 1] = {
[NFTA_HOOK_PRIORITY] = { .type = NLA_U32 },
};
+static int nft_dump_stats(struct sk_buff *skb, struct nft_stats __percpu *stats)
+{
+ struct nft_stats *cpu_stats, total;
+ struct nlattr *nest;
+ int cpu;
+
+ memset(&total, 0, sizeof(total));
+ for_each_possible_cpu(cpu) {
+ cpu_stats = per_cpu_ptr(stats, cpu);
+ total.pkts += cpu_stats->pkts;
+ total.bytes += cpu_stats->bytes;
+ }
+ nest = nla_nest_start(skb, NFTA_CHAIN_COUNTERS);
+ if (nest == NULL)
+ goto nla_put_failure;
+
+ if (nla_put_be64(skb, NFTA_COUNTER_PACKETS, cpu_to_be64(total.pkts)) ||
+ nla_put_be64(skb, NFTA_COUNTER_BYTES, cpu_to_be64(total.bytes)))
+ goto nla_put_failure;
+
+ nla_nest_end(skb, nest);
+ return 0;
+
+nla_put_failure:
+ return -ENOSPC;
+}
+
static int nf_tables_fill_chain_info(struct sk_buff *skb, u32 portid, u32 seq,
int event, u32 flags, int family,
const struct nft_table *table,
@@ -575,6 +602,9 @@ static int nf_tables_fill_chain_info(struct sk_buff *skb, u32 portid, u32 seq,
if (nla_put_string(skb, NFTA_CHAIN_TYPE,
chain_type[ops->pf][nft_base_chain(chain)->type]->name))
goto nla_put_failure;
+
+ if (nft_dump_stats(skb, nft_base_chain(chain)->stats))
+ goto nla_put_failure;
}
if (nla_put_be32(skb, NFTA_CHAIN_USE, htonl(chain->use)))
@@ -728,6 +758,51 @@ nf_tables_chain_policy(struct nft_base_chain *chain, const struct nlattr *attr)
return 0;
}
+static const struct nla_policy nft_counter_policy[NFTA_COUNTER_MAX + 1] = {
+ [NFTA_COUNTER_PACKETS] = { .type = NLA_U64 },
+ [NFTA_COUNTER_BYTES] = { .type = NLA_U64 },
+};
+
+static int
+nf_tables_counters(struct nft_base_chain *chain, const struct nlattr *attr)
+{
+ struct nlattr *tb[NFTA_COUNTER_MAX+1];
+ struct nft_stats __percpu *newstats;
+ struct nft_stats *stats;
+ int err;
+
+ err = nla_parse_nested(tb, NFTA_COUNTER_MAX, attr, nft_counter_policy);
+ if (err < 0)
+ return err;
+
+ if (!tb[NFTA_COUNTER_BYTES] || !tb[NFTA_COUNTER_PACKETS])
+ return -EINVAL;
+
+ newstats = alloc_percpu(struct nft_stats);
+ if (newstats == NULL)
+ return -ENOMEM;
+
+ /* Restore old counters on this cpu, no problem. Per-cpu statistics
+ * are not exposed to userspace.
+ */
+ stats = this_cpu_ptr(newstats);
+ stats->bytes = be64_to_cpu(nla_get_be64(tb[NFTA_COUNTER_BYTES]));
+ stats->pkts = be64_to_cpu(nla_get_be64(tb[NFTA_COUNTER_PACKETS]));
+
+ if (chain->stats) {
+ /* nfnl_lock is held, add some nfnl function for this, later */
+ struct nft_stats __percpu *oldstats =
+ rcu_dereference_protected(chain->stats, 1);
+
+ rcu_assign_pointer(chain->stats, newstats);
+ synchronize_rcu();
+ free_percpu(oldstats);
+ } else
+ rcu_assign_pointer(chain->stats, newstats);
+
+ return 0;
+}
+
static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
const struct nlmsghdr *nlh,
const struct nlattr * const nla[])
@@ -792,6 +867,16 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
return err;
}
+ if (nla[NFTA_CHAIN_COUNTERS]) {
+ if (!(chain->flags & NFT_BASE_CHAIN))
+ return -EOPNOTSUPP;
+
+ err = nf_tables_counters(nft_base_chain(chain),
+ nla[NFTA_CHAIN_COUNTERS]);
+ if (err < 0)
+ return err;
+ }
+
if (nla[NFTA_CHAIN_HANDLE] && name)
nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);
@@ -851,11 +936,22 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
err = nf_tables_chain_policy(basechain,
nla[NFTA_CHAIN_POLICY]);
if (err < 0) {
+ free_percpu(basechain->stats);
kfree(basechain);
return err;
}
} else
basechain->policy = NF_ACCEPT;
+
+ if (nla[NFTA_CHAIN_COUNTERS]) {
+ err = nf_tables_counters(basechain,
+ nla[NFTA_CHAIN_COUNTERS]);
+ if (err < 0) {
+ free_percpu(basechain->stats);
+ kfree(basechain);
+ return err;
+ }
+ }
} else {
chain = kzalloc(sizeof(*chain), GFP_KERNEL);
if (chain == NULL)
@@ -872,6 +968,7 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
chain->flags & NFT_BASE_CHAIN) {
err = nf_register_hook(&nft_base_chain(chain)->ops);
if (err < 0) {
+ free_percpu(basechain->stats);
kfree(basechain);
return err;
}
@@ -889,6 +986,9 @@ static void nf_tables_rcu_chain_destroy(struct rcu_head *head)
BUG_ON(chain->use > 0);
+ if (chain->flags & NFT_BASE_CHAIN)
+ free_percpu(nft_base_chain(chain)->stats);
+
kfree(chain);
}
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index a3f848f..5bb11f5 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -137,6 +137,15 @@ next_rule:
goto next_rule;
}
+ if (rcu_access_pointer(nft_base_chain(chain)->stats)) {
+ struct nft_stats __percpu *stats;
+
+ rcu_read_lock_bh();
+ stats = rcu_dereference(nft_base_chain(chain)->stats);
+ __this_cpu_inc(stats->pkts);
+ __this_cpu_add(stats->bytes, pkt->skb->len);
+ rcu_read_unlock_bh();
+ }
return nft_base_chain(chain)->policy;
}
EXPORT_SYMBOL_GPL(nft_do_chain_pktinfo);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [nftables 6/9] netfilter: nf_tables: add protocol and flags for xtables over nf_tables
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
` (3 preceding siblings ...)
2013-01-31 0:04 ` [nftables 5/9] netfilter: nf_tables: add packet and byte counters per chain pablo
@ 2013-01-31 0:04 ` pablo
2013-01-31 0:04 ` [nftables 7/9] netfilter: nf_tables: add trace support pablo
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
This patch adds the missing protocol and flags information in order
to pass it to xt_check_target / xt_check_match.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/linux/netfilter/nf_tables.h | 14 +++++++++++
include/net/netfilter/nf_tables.h | 2 ++
net/netfilter/nf_tables_api.c | 15 +++++++-----
net/netfilter/nft_compat.c | 44 +++++++++++++++++++++++++++++++----
4 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index ccd2c30..76b215f 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -100,10 +100,24 @@ enum nft_rule_attributes {
NFTA_RULE_HANDLE,
NFTA_RULE_EXPRESSIONS,
NFTA_RULE_FLAGS,
+ NFTA_RULE_COMPAT,
__NFTA_RULE_MAX
};
#define NFTA_RULE_MAX (__NFTA_RULE_MAX - 1)
+enum nft_rule_compat_flags {
+ NFT_RULE_COMPAT_F_INV = (1 << 1),
+ NFT_RULE_COMPAT_F_MASK = NFT_RULE_COMPAT_F_INV,
+};
+
+enum nft_rule_compat_attributes {
+ NFTA_RULE_COMPAT_UNSPEC,
+ NFTA_RULE_COMPAT_PROTO,
+ NFTA_RULE_COMPAT_FLAGS,
+ __NFTA_RULE_COMPAT_MAX
+};
+#define NFTA_RULE_COMPAT_MAX (__NFTA_RULE_COMPAT_MAX - 1)
+
/**
* enum nft_set_flags - nf_tables set flags
*
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index a9df310..d172b66 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -74,6 +74,7 @@ static inline void nft_data_debug(const struct nft_data *data)
* @afi: address family info
* @table: the table the chain is contained in
* @chain: the chain the rule is contained in
+ * @nla: netlink attributes
*/
struct nft_ctx {
struct net *net;
@@ -82,6 +83,7 @@ struct nft_ctx {
const struct nft_af_info *afi;
const struct nft_table *table;
const struct nft_chain *chain;
+ const struct nlattr * const *nla;
};
struct nft_data_desc {
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 141e33e..6eda6cc 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1037,7 +1037,8 @@ static void nft_ctx_init(struct nft_ctx *ctx,
const struct nlmsghdr *nlh,
const struct nft_af_info *afi,
const struct nft_table *table,
- const struct nft_chain *chain)
+ const struct nft_chain *chain,
+ const struct nlattr * const *nla)
{
ctx->net = sock_net(skb->sk);
ctx->skb = skb;
@@ -1045,6 +1046,7 @@ static void nft_ctx_init(struct nft_ctx *ctx,
ctx->afi = afi;
ctx->table = table;
ctx->chain = chain;
+ ctx->nla = nla;
}
/*
@@ -1252,6 +1254,7 @@ static const struct nla_policy nft_rule_policy[NFTA_RULE_MAX + 1] = {
[NFTA_RULE_HANDLE] = { .type = NLA_U64 },
[NFTA_RULE_EXPRESSIONS] = { .type = NLA_NESTED },
[NFTA_RULE_FLAGS] = { .type = NLA_U32 },
+ [NFTA_RULE_COMPAT] = { .type = NLA_NESTED },
};
static int nf_tables_fill_rule_info(struct sk_buff *skb, u32 portid, u32 seq,
@@ -1555,7 +1558,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
handle = nf_tables_alloc_handle(table);
}
- nft_ctx_init(&ctx, skb, nlh, afi, table, chain);
+ nft_ctx_init(&ctx, skb, nlh, afi, table, chain, nla);
n = 0;
size = 0;
@@ -1676,7 +1679,7 @@ static int nf_tables_delrule(struct sock *nlsk, struct sk_buff *skb,
if (IS_ERR(chain))
return PTR_ERR(chain);
- nft_ctx_init(&ctx, skb, nlh, afi, table, chain);
+ nft_ctx_init(&ctx, skb, nlh, afi, table, chain, nla);
if (nla[NFTA_RULE_FLAGS]) {
flags = ntohl(nla_get_be32(nla[NFTA_RULE_FLAGS]));
@@ -1885,7 +1888,7 @@ static int nft_ctx_init_from_setattr(struct nft_ctx *ctx,
if (IS_ERR(table))
return PTR_ERR(table);
- nft_ctx_init(ctx, skb, nlh, afi, table, NULL);
+ nft_ctx_init(ctx, skb, nlh, afi, table, NULL, nla);
return 0;
}
@@ -2168,7 +2171,7 @@ static int nf_tables_newset(struct sock *nlsk, struct sk_buff *skb,
if (IS_ERR(table))
return PTR_ERR(table);
- nft_ctx_init(&ctx, skb, nlh, afi, table, NULL);
+ nft_ctx_init(&ctx, skb, nlh, afi, table, NULL, nla);
set = nf_tables_set_lookup(table, nla[NFTA_SET_NAME]);
if (IS_ERR(set)) {
@@ -2353,7 +2356,7 @@ static int nft_ctx_init_from_elemattr(struct nft_ctx *ctx,
if (IS_ERR(table))
return PTR_ERR(table);
- nft_ctx_init(ctx, skb, nlh, afi, table, NULL);
+ nft_ctx_init(ctx, skb, nlh, afi, table, NULL, nla);
return 0;
}
diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index dc156f6..f7dd3db 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -107,6 +107,34 @@ static inline int nft_compat_target_offset(struct xt_target *target)
#endif
}
+static const struct nla_policy nft_rule_compat_policy[NFTA_RULE_COMPAT_MAX + 1] = {
+ [NFTA_RULE_COMPAT_PROTO] = { .type = NLA_U32 },
+ [NFTA_RULE_COMPAT_FLAGS] = { .type = NLA_U32 },
+};
+
+static u8 nft_parse_compat(const struct nlattr *attr, bool *inv)
+{
+ struct nlattr *tb[NFTA_RULE_COMPAT_MAX+1];
+ u32 flags;
+ int err;
+
+ err = nla_parse_nested(tb, NFTA_RULE_COMPAT_MAX, attr,
+ nft_rule_compat_policy);
+ if (err < 0)
+ return err;
+
+ if (!tb[NFTA_RULE_COMPAT_PROTO] || !tb[NFTA_RULE_COMPAT_FLAGS])
+ return -EINVAL;
+
+ flags = ntohl(nla_get_be32(tb[NFTA_RULE_COMPAT_FLAGS]));
+ if (flags & ~NFT_RULE_COMPAT_F_MASK)
+ return -EINVAL;
+ if (flags & NFT_RULE_COMPAT_F_INV)
+ *inv = true;
+
+ return ntohl(nla_get_be32(tb[NFTA_RULE_COMPAT_PROTO]));
+}
+
static int
nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
const struct nlattr * const tb[])
@@ -115,14 +143,18 @@ nft_target_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
struct xt_target *target = expr->ops->data;
struct xt_tgchk_param par;
size_t size = XT_ALIGN(nla_len(tb[NFTA_TARGET_INFO]));
+ u8 proto = 0;
+ bool inv = false;
int ret;
target_compat_from_user(target, nla_data(tb[NFTA_TARGET_INFO]), info);
nft_target_set_tgchk_param(&par, ctx, target, info);
- /* FIXME: not checking protocol and inversion */
- ret = xt_check_target(&par, size, 0, false);
+ if (ctx->nla[NFTA_RULE_COMPAT])
+ proto = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &inv);
+
+ ret = xt_check_target(&par, size, proto, inv);
if (ret < 0)
goto err;
@@ -289,14 +321,18 @@ nft_match_init(const struct nft_ctx *ctx, const struct nft_expr *expr,
struct xt_match *match = expr->ops->data;
struct xt_mtchk_param par;
size_t size = XT_ALIGN(nla_len(tb[NFTA_MATCH_INFO]));
+ u8 proto = 0;
+ bool inv = false;
int ret;
match_compat_from_user(match, nla_data(tb[NFTA_MATCH_INFO]), info);
nft_match_set_mtchk_param(&par, ctx, match, info);
- /* FIXME: not checking protocol and inversion, do this in userspace */
- ret = xt_check_match(&par, size, 0, false);
+ if (ctx->nla[NFTA_RULE_COMPAT])
+ proto = nft_parse_compat(ctx->nla[NFTA_RULE_COMPAT], &inv);
+
+ ret = xt_check_match(&par, size, proto, inv);
if (ret < 0)
goto err;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [nftables 7/9] netfilter: nf_tables: add trace support
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
` (4 preceding siblings ...)
2013-01-31 0:04 ` [nftables 6/9] netfilter: nf_tables: add protocol and flags for xtables over nf_tables pablo
@ 2013-01-31 0:04 ` pablo
2013-01-31 0:04 ` [nftables 8/9] netfilter: nf_tables: add missing code in route chain type pablo
2013-01-31 0:04 ` [nftables 9/9] netfilter: nf_tables: statify chain definition to fix sparse warning pablo
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/net/netfilter/nf_tables.h | 2 ++
net/netfilter/nf_tables_api.c | 1 +
net/netfilter/nf_tables_core.c | 55 +++++++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+)
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index d172b66..3782777 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -376,6 +376,7 @@ enum nft_chain_flags {
* @list: used internally
* @rcu_head: used internally
* @net: net namespace that this chain belongs to
+ * @table: table that this chain belongs to
* @handle: chain handle
* @flags: bitmask of enum nft_chain_flags
* @use: number of jump references to this chain
@@ -388,6 +389,7 @@ struct nft_chain {
struct list_head list;
struct rcu_head rcu_head;
struct net *net;
+ struct nft_table *table;
u64 handle;
u8 flags;
u16 use;
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6eda6cc..7fc8d51 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -962,6 +962,7 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
INIT_LIST_HEAD(&chain->dirty_rules);
chain->handle = nf_tables_alloc_handle(table);
chain->net = net;
+ chain->table = table;
nla_strlcpy(chain->name, name, NFT_CHAIN_MAXNAMELEN);
if (!(table->flags & NFT_TABLE_F_DORMANT) &&
diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
index 5bb11f5..1b81546 100644
--- a/net/netfilter/nf_tables_core.c
+++ b/net/netfilter/nf_tables_core.c
@@ -19,6 +19,7 @@
#include <linux/netfilter/nf_tables.h>
#include <net/netfilter/nf_tables_core.h>
#include <net/netfilter/nf_tables.h>
+#include <net/netfilter/nf_log.h>
static void nft_cmp_fast_eval(const struct nft_expr *expr,
struct nft_data data[NFT_REG_MAX + 1])
@@ -60,6 +61,39 @@ static bool nft_payload_fast_eval(const struct nft_expr *expr,
return true;
}
+enum nft_trace {
+ NFT_TRACE_RULE,
+ NFT_TRACE_RETURN,
+ NFT_TRACE_POLICY,
+};
+
+static const char *const comments[] = {
+ [NFT_TRACE_RULE] = "rule",
+ [NFT_TRACE_RETURN] = "return",
+ [NFT_TRACE_POLICY] = "policy",
+};
+
+static struct nf_loginfo trace_loginfo = {
+ .type = NF_LOG_TYPE_LOG,
+ .u = {
+ .log = {
+ .level = 4,
+ .logflags = NF_LOG_MASK,
+ },
+ },
+};
+
+static void nft_trace_packet(const struct nft_pktinfo *pkt,
+ const struct nft_chain *chain,
+ int rulenum, enum nft_trace type)
+{
+ const char *comment = comments[type];
+
+ nf_log_packet(pkt->xt.family, pkt->hooknum, pkt->skb, pkt->in, pkt->out,
+ &trace_loginfo, "TRACE: %s:%s:%s:%u ",
+ chain->table->name, chain->name, comment, rulenum);
+}
+
unsigned int
nft_do_chain_pktinfo(struct nft_pktinfo *pkt, const struct nf_hook_ops *ops)
{
@@ -71,7 +105,10 @@ nft_do_chain_pktinfo(struct nft_pktinfo *pkt, const struct nf_hook_ops *ops)
struct {
const struct nft_chain *chain;
const struct nft_rule *rule;
+ int rulenum;
} jumpstack[NFT_JUMP_STACK_SIZE];
+ int rulenum = 0;
+
/*
* Cache cursor to avoid problems in case that the cursor is updated
* while traversing the ruleset.
@@ -83,6 +120,7 @@ do_chain:
next_rule:
data[NFT_REG_VERDICT].verdict = NFT_CONTINUE;
list_for_each_entry_continue_rcu(rule, &chain->rules, list) {
+ rulenum++;
/* This rule is not active, skip. */
if (unlikely(rule->genmask & (1 << gencursor)))
@@ -113,17 +151,28 @@ next_rule:
case NF_ACCEPT:
case NF_DROP:
case NF_QUEUE:
+ if (unlikely(pkt->skb->nf_trace))
+ nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
+
return data[NFT_REG_VERDICT].verdict;
case NFT_JUMP:
+ if (unlikely(pkt->skb->nf_trace))
+ nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RULE);
+
BUG_ON(stackptr >= NFT_JUMP_STACK_SIZE);
jumpstack[stackptr].chain = chain;
jumpstack[stackptr].rule = rule;
+ jumpstack[stackptr].rulenum = rulenum;
stackptr++;
/* fall through */
case NFT_GOTO:
chain = data[NFT_REG_VERDICT].chain;
goto do_chain;
case NFT_RETURN:
+ if (unlikely(pkt->skb->nf_trace))
+ nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_RETURN);
+
+ /* fall through */
case NFT_CONTINUE:
break;
default:
@@ -134,6 +183,7 @@ next_rule:
stackptr--;
chain = jumpstack[stackptr].chain;
rule = jumpstack[stackptr].rule;
+ rulenum = jumpstack[stackptr].rulenum;
goto next_rule;
}
@@ -146,6 +196,11 @@ next_rule:
__this_cpu_add(stats->bytes, pkt->skb->len);
rcu_read_unlock_bh();
}
+ if (unlikely(pkt->skb->nf_trace)) {
+ rulenum++; /* Skip last rule */
+ nft_trace_packet(pkt, chain, rulenum, NFT_TRACE_POLICY);
+ }
+
return nft_base_chain(chain)->policy;
}
EXPORT_SYMBOL_GPL(nft_do_chain_pktinfo);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [nftables 8/9] netfilter: nf_tables: add missing code in route chain type
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
` (5 preceding siblings ...)
2013-01-31 0:04 ` [nftables 7/9] netfilter: nf_tables: add trace support pablo
@ 2013-01-31 0:04 ` pablo
2013-01-31 0:04 ` [nftables 9/9] netfilter: nf_tables: statify chain definition to fix sparse warning pablo
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
Add missing code for the route chain type based on the mangle
table code from x_tables.
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/ipv4/netfilter/nft_chain_route_ipv4.c | 24 +++++++++++++++++++++---
net/ipv6/netfilter/nft_chain_route_ipv6.c | 24 +++++++++++++++++++-----
2 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/net/ipv4/netfilter/nft_chain_route_ipv4.c b/net/ipv4/netfilter/nft_chain_route_ipv4.c
index f991eb0..4b4fd08 100644
--- a/net/ipv4/netfilter/nft_chain_route_ipv4.c
+++ b/net/ipv4/netfilter/nft_chain_route_ipv4.c
@@ -29,15 +29,33 @@ static unsigned int nf_route_table_hook(const struct nf_hook_ops *ops,
unsigned int ret;
struct nft_pktinfo pkt;
u32 mark;
+ __be32 saddr, daddr;
+ u_int8_t tos;
+ const struct iphdr *iph;
nft_set_pktinfo_ipv4(&pkt, ops, skb, in, out);
- // FIXME: length validation
+ /* root is playing with raw sockets. */
+ if (skb->len < sizeof(struct iphdr) ||
+ ip_hdrlen(skb) < sizeof(struct iphdr))
+ return NF_ACCEPT;
+
mark = skb->mark;
+ iph = ip_hdr(skb);
+ saddr = iph->saddr;
+ daddr = iph->daddr;
+ tos = iph->tos;
+
ret = nft_do_chain_pktinfo(&pkt, ops);
if (ret != NF_DROP && ret != NF_QUEUE) {
- if (skb->mark != mark && ip_route_me_harder(skb, RTN_UNSPEC))
- ret = NF_DROP;
+ iph = ip_hdr(skb);
+
+ if (iph->saddr != saddr ||
+ iph->daddr != daddr ||
+ skb->mark != mark ||
+ iph->tos != tos)
+ if (ip_route_me_harder(skb, RTN_UNSPEC))
+ ret = NF_DROP;
}
return ret;
}
diff --git a/net/ipv6/netfilter/nft_chain_route_ipv6.c b/net/ipv6/netfilter/nft_chain_route_ipv6.c
index 341b3a8..c70d2de 100644
--- a/net/ipv6/netfilter/nft_chain_route_ipv6.c
+++ b/net/ipv6/netfilter/nft_chain_route_ipv6.c
@@ -30,17 +30,31 @@ static unsigned int nf_route_table_hook(const struct nf_hook_ops *ops,
{
unsigned int ret;
struct nft_pktinfo pkt;
- u32 mark;
+ struct in6_addr saddr, daddr;
+ u_int8_t hop_limit;
+ u32 mark, flowlabel;
if (nft_set_pktinfo_ipv6(&pkt, ops, skb, in, out) < 0)
return NF_DROP;
+ /* save source/dest address, mark, hoplimit, flowlabel, priority */
+ memcpy(&saddr, &ipv6_hdr(skb)->saddr, sizeof(saddr));
+ memcpy(&daddr, &ipv6_hdr(skb)->daddr, sizeof(daddr));
mark = skb->mark;
+ hop_limit = ipv6_hdr(skb)->hop_limit;
+
+ /* flowlabel and prio (includes version, which shouldn't change either */
+ flowlabel = *((u32 *)ipv6_hdr(skb));
+
ret = nft_do_chain_pktinfo(&pkt, ops);
- if (ret != NF_DROP && ret != NF_QUEUE) {
- if (skb->mark != mark && ip6_route_me_harder(skb))
- ret = NF_DROP;
- }
+ if (ret != NF_DROP && ret != NF_QUEUE &&
+ (memcmp(&ipv6_hdr(skb)->saddr, &saddr, sizeof(saddr)) ||
+ memcmp(&ipv6_hdr(skb)->daddr, &daddr, sizeof(daddr)) ||
+ skb->mark != mark ||
+ ipv6_hdr(skb)->hop_limit != hop_limit ||
+ flowlabel != *((u_int32_t *)ipv6_hdr(skb))))
+ return ip6_route_me_harder(skb) == 0 ? ret : NF_DROP;
+
return ret;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread* [nftables 9/9] netfilter: nf_tables: statify chain definition to fix sparse warning
2013-01-31 0:03 [nftables 1/9] netfilter: nf_tables: fix compilation if CONFIG_COMPAT is disabled pablo
` (6 preceding siblings ...)
2013-01-31 0:04 ` [nftables 8/9] netfilter: nf_tables: add missing code in route chain type pablo
@ 2013-01-31 0:04 ` pablo
7 siblings, 0 replies; 16+ messages in thread
From: pablo @ 2013-01-31 0:04 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber, tomasz.bursztyka
From: Pablo Neira Ayuso <pablo@netfilter.org>
CHECK net/ipv4/netfilter/nft_chain_route_ipv4.c
net/ipv4/netfilter/nft_chain_route_ipv4.c:63:22: warning: symbol 'nft_chain_route_ipv4' was not declared. Should it be static?
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/ipv4/netfilter/nft_chain_nat_ipv4.c | 2 +-
net/ipv4/netfilter/nft_chain_route_ipv4.c | 2 +-
net/ipv6/netfilter/nft_chain_nat_ipv6.c | 2 +-
net/ipv6/netfilter/nft_chain_route_ipv6.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/netfilter/nft_chain_nat_ipv4.c b/net/ipv4/netfilter/nft_chain_nat_ipv4.c
index e42d6d9..cf2c792 100644
--- a/net/ipv4/netfilter/nft_chain_nat_ipv4.c
+++ b/net/ipv4/netfilter/nft_chain_nat_ipv4.c
@@ -164,7 +164,7 @@ static unsigned int nf_nat_output(const struct nf_hook_ops *ops,
return ret;
}
-struct nf_chain_type nft_chain_nat_ipv4 = {
+static struct nf_chain_type nft_chain_nat_ipv4 = {
.family = NFPROTO_IPV4,
.name = "nat",
.type = NFT_CHAIN_T_NAT,
diff --git a/net/ipv4/netfilter/nft_chain_route_ipv4.c b/net/ipv4/netfilter/nft_chain_route_ipv4.c
index 4b4fd08..8c55ba3 100644
--- a/net/ipv4/netfilter/nft_chain_route_ipv4.c
+++ b/net/ipv4/netfilter/nft_chain_route_ipv4.c
@@ -60,7 +60,7 @@ static unsigned int nf_route_table_hook(const struct nf_hook_ops *ops,
return ret;
}
-struct nf_chain_type nft_chain_route_ipv4 = {
+static struct nf_chain_type nft_chain_route_ipv4 = {
.family = NFPROTO_IPV4,
.name = "route",
.type = NFT_CHAIN_T_ROUTE,
diff --git a/net/ipv6/netfilter/nft_chain_nat_ipv6.c b/net/ipv6/netfilter/nft_chain_nat_ipv6.c
index 9f283dd..e86dcd7 100644
--- a/net/ipv6/netfilter/nft_chain_nat_ipv6.c
+++ b/net/ipv6/netfilter/nft_chain_nat_ipv6.c
@@ -170,7 +170,7 @@ static unsigned int nf_nat_ipv6_output(const struct nf_hook_ops *ops,
return ret;
}
-struct nf_chain_type nft_chain_nat_ipv6 = {
+static struct nf_chain_type nft_chain_nat_ipv6 = {
.family = NFPROTO_IPV6,
.name = "nat",
.type = NFT_CHAIN_T_NAT,
diff --git a/net/ipv6/netfilter/nft_chain_route_ipv6.c b/net/ipv6/netfilter/nft_chain_route_ipv6.c
index c70d2de..dc4e2dd 100644
--- a/net/ipv6/netfilter/nft_chain_route_ipv6.c
+++ b/net/ipv6/netfilter/nft_chain_route_ipv6.c
@@ -58,7 +58,7 @@ static unsigned int nf_route_table_hook(const struct nf_hook_ops *ops,
return ret;
}
-struct nf_chain_type nft_chain_route_ipv6 = {
+static struct nf_chain_type nft_chain_route_ipv6 = {
.family = NFPROTO_IPV6,
.name = "route",
.type = NFT_CHAIN_T_ROUTE,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 16+ messages in thread