* [nf-next/nf_tables-experiments - PATCH 0/2] chain rename and rule replacement
@ 2012-10-31 9:28 Tomasz Bursztyka
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name Tomasz Bursztyka
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 2/2] nf_tables: Add support for replacing a rule by another one Tomasz Bursztyka
0 siblings, 2 replies; 6+ messages in thread
From: Tomasz Bursztyka @ 2012-10-31 9:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: Tomasz Bursztyka
Hi,
Here is two patches, there title is self-explaining.
Renaming chain's name applies only on user's chain. Not on builtin or base ones.
Please review,
Tomasz Bursztyka (2):
nf_tables: Add support for changing users chain's name
nf_tables: Add support for replacing a rule by another one.
include/linux/netfilter/nf_tables.h | 1 +
net/netfilter/nf_tables_api.c | 82 ++++++++++++++++++++++++++++++++++---
2 files changed, 77 insertions(+), 6 deletions(-)
--
1.7.12.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name
2012-10-31 9:28 [nf-next/nf_tables-experiments - PATCH 0/2] chain rename and rule replacement Tomasz Bursztyka
@ 2012-10-31 9:28 ` Tomasz Bursztyka
2012-10-31 13:49 ` Pablo Neira Ayuso
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 2/2] nf_tables: Add support for replacing a rule by another one Tomasz Bursztyka
1 sibling, 1 reply; 6+ messages in thread
From: Tomasz Bursztyka @ 2012-10-31 9:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: Tomasz Bursztyka
Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
include/linux/netfilter/nf_tables.h | 1 +
net/netfilter/nf_tables_api.c | 58 ++++++++++++++++++++++++++++++++++++-
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
index 0115a2f..542b654 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -67,6 +67,7 @@ enum nft_chain_attributes {
NFTA_CHAIN_HOOK,
NFTA_CHAIN_POLICY,
NFTA_CHAIN_USE,
+ NFTA_CHAIN_NEW_NAME,
__NFTA_CHAIN_MAX
};
#define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index e0e4616..fd1b624 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -676,6 +676,62 @@ nf_tables_chain_policy(struct nft_chain *chain, const struct nlattr *attr)
return 0;
}
+static int nf_tables_mvchain(struct sk_buff *skb, const struct nlmsghdr *nlh,
+ struct nft_table *table,
+ struct nft_chain *old_chain,
+ const struct nlattr * const nla[])
+{
+ const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
+ int family = nfmsg->nfgen_family;
+ struct nft_chain *new_chain;
+ const struct nlattr *name;
+ unsigned int size;
+
+ if (!nla[NFTA_CHAIN_NEW_NAME])
+ return -EINVAL;
+
+ if (old_chain->flags & NFT_CHAIN_BUILTIN ||
+ old_chain->flags & NFT_BASE_CHAIN)
+ return -EOPNOTSUPP;
+
+ if (old_chain->use > 0)
+ return -EBUSY;
+
+ name = nla[NFTA_CHAIN_NEW_NAME];
+ new_chain = nf_tables_chain_lookup(table, name);
+ if (IS_ERR(new_chain)) {
+ if (PTR_ERR(new_chain) != -ENOENT)
+ return PTR_ERR(new_chain);
+ new_chain = NULL;
+ }
+
+ if (new_chain != NULL)
+ return -EEXIST;
+
+ size = nla_len(name);
+ new_chain = kzalloc(sizeof(*new_chain) + size, GFP_KERNEL);
+ if (new_chain == NULL)
+ return -ENOMEM;
+
+ list_del(&old_chain->list);
+
+ INIT_LIST_HEAD(&new_chain->rules);
+ nla_strlcpy(new_chain->name, name, size);
+
+ /* Copying content from old chain */
+ new_chain->flags = old_chain->flags;
+ list_replace_init(&old_chain->rules, &new_chain->rules);
+
+ list_add_tail(&new_chain->list, &table->chains);
+
+ nf_tables_chain_notify(skb, nlh, table, old_chain, NFT_MSG_DELCHAIN,
+ family);
+ kfree(old_chain);
+ nf_tables_chain_notify(skb, nlh, table, new_chain, NFT_MSG_NEWCHAIN,
+ family);
+ return 0;
+}
+
static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
const struct nlmsghdr *nlh,
const struct nlattr * const nla[])
@@ -714,7 +770,7 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
if (nlh->nlmsg_flags & NLM_F_EXCL)
return -EEXIST;
if (nlh->nlmsg_flags & NLM_F_REPLACE)
- return -EOPNOTSUPP;
+ return nf_tables_mvchain(skb, nlh, table, chain, nla);
if ((chain->flags & NFT_BASE_CHAIN) && nla[NFTA_CHAIN_POLICY]) {
return nf_tables_chain_policy(chain,
--
1.7.12.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [nf-next/nf_tables-experiments - PATCH 2/2] nf_tables: Add support for replacing a rule by another one.
2012-10-31 9:28 [nf-next/nf_tables-experiments - PATCH 0/2] chain rename and rule replacement Tomasz Bursztyka
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name Tomasz Bursztyka
@ 2012-10-31 9:28 ` Tomasz Bursztyka
1 sibling, 0 replies; 6+ messages in thread
From: Tomasz Bursztyka @ 2012-10-31 9:28 UTC (permalink / raw)
To: netfilter-devel; +Cc: Tomasz Bursztyka
Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
---
net/netfilter/nf_tables_api.c | 24 +++++++++++++++++++-----
1 file changed, 19 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index fd1b624..3ce8aa1 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1321,7 +1321,7 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
const struct nft_af_info *afi;
const struct nft_table *table;
struct nft_chain *chain;
- struct nft_rule *rule;
+ struct nft_rule *rule, *old_rule = NULL;
struct nft_expr_info info[NFT_RULE_MAXEXPRS];
struct nft_expr *expr;
struct nft_ctx ctx;
@@ -1357,9 +1357,11 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
if (rule != NULL) {
if (nlh->nlmsg_flags & NLM_F_EXCL)
return -EEXIST;
- if (nlh->nlmsg_flags & NLM_F_REPLACE)
- return -EOPNOTSUPP;
- return 0;
+ if (nlh->nlmsg_flags & NLM_F_REPLACE) {
+ old_rule = rule;
+ rule = NULL;
+ } else
+ return 0;
}
} else
handle = nf_tables_rule_alloc_handle(chain);
@@ -1402,7 +1404,19 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
expr = nft_expr_next(expr);
}
- if (nlh->nlmsg_flags & NLM_F_APPEND)
+ if (nlh->nlmsg_flags & NLM_F_REPLACE) {
+ if (old_rule == NULL)
+ goto err2;
+
+ list_replace_rcu(&old_rule->list, &rule->list);
+
+ // FIXME: this makes deletion performance *really* suck
+ synchronize_rcu();
+
+ nf_tables_rule_notify(skb, nlh, table, chain, old_rule,
+ NFT_MSG_DELRULE, nfmsg->nfgen_family);
+ nf_tables_rule_destroy(&ctx, 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);
--
1.7.12.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name Tomasz Bursztyka
@ 2012-10-31 13:49 ` Pablo Neira Ayuso
2012-10-31 14:29 ` Tomasz Bursztyka
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2012-10-31 13:49 UTC (permalink / raw)
To: Tomasz Bursztyka; +Cc: netfilter-devel
Hi Tomasz,
The patchset looks good, I just have a comment on this one.
>From net/netfilter/nf_tables_core.c:
case NFT_GOTO:
chain = data[NFT_REG_VERDICT].chain;
In the GOTO case, we already point to the chain object. That chain
object is loaded in nft_immediate.c _eval(). However, the private data
of immediate is initialized in the _init() path. That means we would
need to refresh the entire rule-set to point to the correct new chain
object, otherwise we would crash.
I'd follow a simpler solution to avoid issues. Use a fixed chain name
length (the same length as iptables does, or just 32 bytes).
We can revisit this later to see if we can support renaming and
dynamically allocated chain objects at the same time.
Would you resend a new version of this patch?
Thanks.
On Wed, Oct 31, 2012 at 11:28:28AM +0200, Tomasz Bursztyka wrote:
> Signed-off-by: Tomasz Bursztyka <tomasz.bursztyka@linux.intel.com>
> ---
> include/linux/netfilter/nf_tables.h | 1 +
> net/netfilter/nf_tables_api.c | 58 ++++++++++++++++++++++++++++++++++++-
> 2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/netfilter/nf_tables.h b/include/linux/netfilter/nf_tables.h
> index 0115a2f..542b654 100644
> --- a/include/linux/netfilter/nf_tables.h
> +++ b/include/linux/netfilter/nf_tables.h
> @@ -67,6 +67,7 @@ enum nft_chain_attributes {
> NFTA_CHAIN_HOOK,
> NFTA_CHAIN_POLICY,
> NFTA_CHAIN_USE,
> + NFTA_CHAIN_NEW_NAME,
> __NFTA_CHAIN_MAX
> };
> #define NFTA_CHAIN_MAX (__NFTA_CHAIN_MAX - 1)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index e0e4616..fd1b624 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -676,6 +676,62 @@ nf_tables_chain_policy(struct nft_chain *chain, const struct nlattr *attr)
> return 0;
> }
>
> +static int nf_tables_mvchain(struct sk_buff *skb, const struct nlmsghdr *nlh,
> + struct nft_table *table,
> + struct nft_chain *old_chain,
> + const struct nlattr * const nla[])
> +{
> + const struct nfgenmsg *nfmsg = nlmsg_data(nlh);
> + int family = nfmsg->nfgen_family;
> + struct nft_chain *new_chain;
> + const struct nlattr *name;
> + unsigned int size;
> +
> + if (!nla[NFTA_CHAIN_NEW_NAME])
> + return -EINVAL;
> +
> + if (old_chain->flags & NFT_CHAIN_BUILTIN ||
> + old_chain->flags & NFT_BASE_CHAIN)
> + return -EOPNOTSUPP;
> +
> + if (old_chain->use > 0)
> + return -EBUSY;
> +
> + name = nla[NFTA_CHAIN_NEW_NAME];
> + new_chain = nf_tables_chain_lookup(table, name);
> + if (IS_ERR(new_chain)) {
> + if (PTR_ERR(new_chain) != -ENOENT)
> + return PTR_ERR(new_chain);
> + new_chain = NULL;
> + }
> +
> + if (new_chain != NULL)
> + return -EEXIST;
> +
> + size = nla_len(name);
> + new_chain = kzalloc(sizeof(*new_chain) + size, GFP_KERNEL);
> + if (new_chain == NULL)
> + return -ENOMEM;
> +
> + list_del(&old_chain->list);
> +
> + INIT_LIST_HEAD(&new_chain->rules);
> + nla_strlcpy(new_chain->name, name, size);
> +
> + /* Copying content from old chain */
> + new_chain->flags = old_chain->flags;
> + list_replace_init(&old_chain->rules, &new_chain->rules);
> +
> + list_add_tail(&new_chain->list, &table->chains);
> +
> + nf_tables_chain_notify(skb, nlh, table, old_chain, NFT_MSG_DELCHAIN,
> + family);
> + kfree(old_chain);
> + nf_tables_chain_notify(skb, nlh, table, new_chain, NFT_MSG_NEWCHAIN,
> + family);
> + return 0;
> +}
> +
> static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
> const struct nlmsghdr *nlh,
> const struct nlattr * const nla[])
> @@ -714,7 +770,7 @@ static int nf_tables_newchain(struct sock *nlsk, struct sk_buff *skb,
> if (nlh->nlmsg_flags & NLM_F_EXCL)
> return -EEXIST;
> if (nlh->nlmsg_flags & NLM_F_REPLACE)
> - return -EOPNOTSUPP;
> + return nf_tables_mvchain(skb, nlh, table, chain, nla);
>
> if ((chain->flags & NFT_BASE_CHAIN) && nla[NFTA_CHAIN_POLICY]) {
> return nf_tables_chain_policy(chain,
> --
> 1.7.12.4
>
> --
> 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] 6+ messages in thread
* Re: [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name
2012-10-31 13:49 ` Pablo Neira Ayuso
@ 2012-10-31 14:29 ` Tomasz Bursztyka
2012-10-31 15:38 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Tomasz Bursztyka @ 2012-10-31 14:29 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
Hi,
> > From net/netfilter/nf_tables_core.c:
>
> case NFT_GOTO:
> chain = data[NFT_REG_VERDICT].chain;
>
> In the GOTO case, we already point to the chain object. That chain
> object is loaded in nft_immediate.c _eval(). However, the private data
> of immediate is initialized in the _init() path. That means we would
> need to refresh the entire rule-set to point to the correct new chain
> object, otherwise we would crash.
Missed that one completely...
> I'd follow a simpler solution to avoid issues. Use a fixed chain name
> length (the same length as iptables does, or just 32 bytes).
>
> We can revisit this later to see if we can support renaming and
> dynamically allocated chain objects at the same time.
First solution is easy to implement, and do not imply any performance
drop, only a bit of memory loss for short names.
Second one, I cannot see anything but a 2 pass system (a hash table
handling unique handle related to a chain pointer).
But then the gain of bytes - with variable name size - versus the size
of the hash table etc... might be pointless.
Maybe I am just wrong. Is there any other solution?
> Would you resend a new version of this patch?
Ok, I will go with first solution, unless you or someone else comes up
with a better proposition.
Tomasz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name
2012-10-31 14:29 ` Tomasz Bursztyka
@ 2012-10-31 15:38 ` Pablo Neira Ayuso
0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2012-10-31 15:38 UTC (permalink / raw)
To: Tomasz Bursztyka; +Cc: netfilter-devel
On Wed, Oct 31, 2012 at 04:29:01PM +0200, Tomasz Bursztyka wrote:
> Hi,
> >> From net/netfilter/nf_tables_core.c:
> >
> > case NFT_GOTO:
> > chain = data[NFT_REG_VERDICT].chain;
> >
> >In the GOTO case, we already point to the chain object. That chain
> >object is loaded in nft_immediate.c _eval(). However, the private data
> >of immediate is initialized in the _init() path. That means we would
> >need to refresh the entire rule-set to point to the correct new chain
> >object, otherwise we would crash.
>
> Missed that one completely...
>
> >I'd follow a simpler solution to avoid issues. Use a fixed chain name
> >length (the same length as iptables does, or just 32 bytes).
> >
> >We can revisit this later to see if we can support renaming and
> >dynamically allocated chain objects at the same time.
>
> First solution is easy to implement, and do not imply any
> performance drop, only a bit of memory loss for short names.
>
> Second one, I cannot see anything but a 2 pass system (a hash table
> handling unique handle related to a chain pointer).
> But then the gain of bytes - with variable name size - versus the
> size of the hash table etc... might be pointless.
> Maybe I am just wrong. Is there any other solution?
I don't see any at this moment.
> >Would you resend a new version of this patch?
>
> Ok, I will go with first solution, unless you or someone else comes
> up with a better proposition.
Agreed. It's simple and we can still revisit this later.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-31 15:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-31 9:28 [nf-next/nf_tables-experiments - PATCH 0/2] chain rename and rule replacement Tomasz Bursztyka
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 1/2] nf_tables: Add support for changing users chain's name Tomasz Bursztyka
2012-10-31 13:49 ` Pablo Neira Ayuso
2012-10-31 14:29 ` Tomasz Bursztyka
2012-10-31 15:38 ` Pablo Neira Ayuso
2012-10-31 9:28 ` [nf-next/nf_tables-experiments - PATCH 2/2] nf_tables: Add support for replacing a rule by another one Tomasz Bursztyka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).