Patchwork [nf-next/nf_tables-experiments,-,1/2] nf_tables: Add support for changing users chain's name

login
register
mail settings
Submitter Tomasz Bursztyka
Date Oct. 31, 2012, 9:28 a.m.
Message ID <1351675709-14127-2-git-send-email-tomasz.bursztyka@linux.intel.com>
Download mbox | patch
Permalink /patch/195777/
State Superseded
Headers show

Comments

Tomasz Bursztyka - Oct. 31, 2012, 9:28 a.m.
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(-)
Pablo Neira - Oct. 31, 2012, 1:49 p.m.
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
--
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
Tomasz Bursztyka - Oct. 31, 2012, 2:29 p.m.
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
--
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
Pablo Neira - Oct. 31, 2012, 3:38 p.m.
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.
--
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

Patch

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,