[PATCH 0/3] netfilter fixes for 2.6.30-rc

[PATCH 0/3] netfilter fixes for 2.6.30-rc

[Pablo Neira Ayuso]

Hi Patrick,

This patchset contains three fixes for 2.6.30-rc. Two for DCCP
conntrack support: one adding the role attribute which is required
to create consistent conntrack entries and another to fix a missing
initialization in Holger's ctnetlink message shrinking. There's
another patch for the new cluster match, I noticed a problem while
testing the user-space part in iptables (not in the git tree yet).

Please, apply!

---

Pablo Neira Ayuso (3):
      netfilter: conntrack: fix EINVAL during DCCP loading
      netfilter: iptables: fix use of cluster match with 32 nodes
      netfilter: conntrack: add missing role attributes for DCCP


 include/linux/netfilter/nfnetlink_conntrack.h |    1 +
 include/linux/netfilter/xt_cluster.h          |    2 ++
 net/netfilter/nf_conntrack_proto_dccp.c       |   16 +++++++++++++++-
 net/netfilter/xt_cluster.c                    |    8 +++++++-
 4 files changed, 25 insertions(+), 2 deletions(-)

-- 
Signature

[PATCH 1/3] netfilter: conntrack: add missing role attributes for DCCP

[Pablo Neira Ayuso]

This patch adds missing role attribute to the DCCP type, otherwise
the creation of entries is not of any use.

The attribute added is CTA_PROTOINFO_DCCP_ROLE which contains the
role of the conntrack original tuple.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---

 include/linux/netfilter/nfnetlink_conntrack.h |    1 +
 net/netfilter/nf_conntrack_proto_dccp.c       |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
index 29fe9ea..1a865e4 100644
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -100,6 +100,7 @@ enum ctattr_protoinfo_tcp {
 enum ctattr_protoinfo_dccp {
 	CTA_PROTOINFO_DCCP_UNSPEC,
 	CTA_PROTOINFO_DCCP_STATE,
+	CTA_PROTOINFO_DCCP_ROLE,
 	__CTA_PROTOINFO_DCCP_MAX,
 };
 #define CTA_PROTOINFO_DCCP_MAX (__CTA_PROTOINFO_DCCP_MAX - 1)
diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
index 50dac8d..3c266e6 100644
--- a/net/netfilter/nf_conntrack_proto_dccp.c
+++ b/net/netfilter/nf_conntrack_proto_dccp.c
@@ -633,6 +633,8 @@ static int dccp_to_nlattr(struct sk_buff *skb, struct nlattr *nla,
 	if (!nest_parms)
 		goto nla_put_failure;
 	NLA_PUT_U8(skb, CTA_PROTOINFO_DCCP_STATE, ct->proto.dccp.state);
+	NLA_PUT_U8(skb, CTA_PROTOINFO_DCCP_ROLE,
+		   ct->proto.dccp.role[IP_CT_DIR_ORIGINAL]);
 	nla_nest_end(skb, nest_parms);
 	read_unlock_bh(&dccp_lock);
 	return 0;
@@ -644,6 +646,7 @@ nla_put_failure:
 
 static const struct nla_policy dccp_nla_policy[CTA_PROTOINFO_DCCP_MAX + 1] = {
 	[CTA_PROTOINFO_DCCP_STATE]	= { .type = NLA_U8 },
+	[CTA_PROTOINFO_DCCP_ROLE]	= { .type = NLA_U8 },
 };
 
 static int nlattr_to_dccp(struct nlattr *cda[], struct nf_conn *ct)
@@ -661,11 +664,21 @@ static int nlattr_to_dccp(struct nlattr *cda[], struct nf_conn *ct)
 		return err;
 
 	if (!tb[CTA_PROTOINFO_DCCP_STATE] ||
-	    nla_get_u8(tb[CTA_PROTOINFO_DCCP_STATE]) >= CT_DCCP_IGNORE)
+	    !tb[CTA_PROTOINFO_DCCP_ROLE] ||
+	    nla_get_u8(tb[CTA_PROTOINFO_DCCP_ROLE]) > CT_DCCP_ROLE_MAX ||
+	    nla_get_u8(tb[CTA_PROTOINFO_DCCP_STATE]) >= CT_DCCP_IGNORE) {
 		return -EINVAL;
+	}
 
 	write_lock_bh(&dccp_lock);
 	ct->proto.dccp.state = nla_get_u8(tb[CTA_PROTOINFO_DCCP_STATE]);
+	if (nla_get_u8(tb[CTA_PROTOINFO_DCCP_ROLE]) == CT_DCCP_ROLE_CLIENT) {
+		ct->proto.dccp.role[IP_CT_DIR_ORIGINAL] = CT_DCCP_ROLE_CLIENT;
+		ct->proto.dccp.role[IP_CT_DIR_REPLY] = CT_DCCP_ROLE_SERVER;
+	} else {
+		ct->proto.dccp.role[IP_CT_DIR_ORIGINAL] = CT_DCCP_ROLE_SERVER;
+		ct->proto.dccp.role[IP_CT_DIR_REPLY] = CT_DCCP_ROLE_CLIENT;
+	}
 	write_unlock_bh(&dccp_lock);
 	return 0;
 }

Re: [PATCH 1/3] netfilter: conntrack: add missing role attributes for DCCP

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch adds missing role attribute to the DCCP type, otherwise
> the creation of entries is not of any use.
> 
> The attribute added is CTA_PROTOINFO_DCCP_ROLE which contains the
> role of the conntrack original tuple.

Applied, thanks Pablo.

[PATCH 2/3] netfilter: iptables: fix use of cluster match with 32 nodes

[Pablo Neira Ayuso]

This patch fixes a problem when you use 32 nodes in the cluster
match:

% iptables -I PREROUTING -t mangle -i eth0 -m cluster \
  --cluster-total-nodes  32  --cluster-local-node  32 \
  --cluster-hash-seed 0xdeadbeef -j MARK --set-mark 0xffff
iptables: Invalid argument. Run `dmesg' for more information.
% dmesg | tail -1
xt_cluster: this node mask cannot be higher than the total number of nodes

The problem is related to this checking:

if (info->node_mask >= (1 << info->total_nodes)) {
	printk(KERN_ERR "xt_cluster: this node mask cannot be "
			"higher than the total number of nodes\n");
	return false;
}

(1 << 32) is 1. Thus, the checking fails. This patch skips the case
in which total_nodes is 32 and it adds an extra validation to ensure
that we don't go over 32 nodes.

BTW, I said this before but I insist: I have only tested the cluster
match with 2 nodes getting ~45% extra performance in an active-active setup.
The maximum limit of 32 nodes is still completely arbitrary. I'd really
appreciate if people that have more nodes in their setups let me know.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---

 include/linux/netfilter/xt_cluster.h |    2 ++
 net/netfilter/xt_cluster.c           |    8 +++++++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter/xt_cluster.h b/include/linux/netfilter/xt_cluster.h
index 5e0a0d0..12c7b13 100644
--- a/include/linux/netfilter/xt_cluster.h
+++ b/include/linux/netfilter/xt_cluster.h
@@ -12,4 +12,6 @@ struct xt_cluster_match_info {
 	u_int32_t		flags;
 };
 
+#define XT_CLUSTER_NODES_MAX    32
+
 #endif /* _XT_CLUSTER_MATCH_H */
diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
index 6c48476..04af29e 100644
--- a/net/netfilter/xt_cluster.c
+++ b/net/netfilter/xt_cluster.c
@@ -135,7 +135,13 @@ static bool xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
 {
 	struct xt_cluster_match_info *info = par->matchinfo;
 
-	if (info->node_mask >= (1 << info->total_nodes)) {
+	if (info->total_nodes > XT_CLUSTER_NODES_MAX) {
+		printk(KERN_ERR "xt_cluster: too many total nodes (%u > %u)\n",
+				info->total_nodes, XT_CLUSTER_NODES_MAX);
+		return false;
+	}
+	if (info->total_nodes < XT_CLUSTER_NODES_MAX &&
+	    info->node_mask >= (1 << info->total_nodes)) {
 		printk(KERN_ERR "xt_cluster: this node mask cannot be "
 				"higher than the total number of nodes\n");
 		return false;

Re: [PATCH 2/3] netfilter: iptables: fix use of cluster match with 32 nodes

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> This patch fixes a problem when you use 32 nodes in the cluster
> match:
> 
> % iptables -I PREROUTING -t mangle -i eth0 -m cluster \
>   --cluster-total-nodes  32  --cluster-local-node  32 \
>   --cluster-hash-seed 0xdeadbeef -j MARK --set-mark 0xffff
> iptables: Invalid argument. Run `dmesg' for more information.
> % dmesg | tail -1
> xt_cluster: this node mask cannot be higher than the total number of nodes
> 
> The problem is related to this checking:
> 
> if (info->node_mask >= (1 << info->total_nodes)) {
> 	printk(KERN_ERR "xt_cluster: this node mask cannot be "
> 			"higher than the total number of nodes\n");
> 	return false;
> }
> 
> (1 << 32) is 1. Thus, the checking fails. This patch skips the case
> in which total_nodes is 32 and it adds an extra validation to ensure
> that we don't go over 32 nodes.
> 
> BTW, I said this before but I insist: I have only tested the cluster
> match with 2 nodes getting ~45% extra performance in an active-active setup.
> The maximum limit of 32 nodes is still completely arbitrary. I'd really
> appreciate if people that have more nodes in their setups let me know.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Looks good, but I think we can simpify it a bit further:

> diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
> index 6c48476..04af29e 100644
> --- a/net/netfilter/xt_cluster.c
> +++ b/net/netfilter/xt_cluster.c
> @@ -135,7 +135,13 @@ static bool xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
>  {
>  	struct xt_cluster_match_info *info = par->matchinfo;
>  
> -	if (info->node_mask >= (1 << info->total_nodes)) {

This could either use 1ULL << info->total_nodes to make sure
we don't have an undefined operation, or

> +	if (info->total_nodes > XT_CLUSTER_NODES_MAX) {
> +		printk(KERN_ERR "xt_cluster: too many total nodes (%u > %u)\n",
> +				info->total_nodes, XT_CLUSTER_NODES_MAX);
> +		return false;
> +	}
> +	if (info->total_nodes < XT_CLUSTER_NODES_MAX &&
> +	    info->node_mask >= (1 << info->total_nodes)) {

we could alternatively use fls.

>  		printk(KERN_ERR "xt_cluster: this node mask cannot be "
>  				"higher than the total number of nodes\n");
>  		return false;
> 

Let me know what you think, either way is fine with me.

Re: [PATCH 2/3] netfilter: iptables: fix use of cluster match with 32 nodes

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 212 bytes --]

Patrick McHardy wrote:
> Let me know what you think, either way is fine with me.

Thanks Patrick, I have done using 1ULL << value. New patch attached.

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers

[-- Attachment #2: cluster.patch --]
[-- Type: text/x-diff, Size: 2348 bytes --]

netfilter: iptables: fix use of cluster match with 32 nodes

This patch fixes a problem when you use 32 nodes in the cluster
match:

% iptables -I PREROUTING -t mangle -i eth0 -m cluster \
  --cluster-total-nodes  32  --cluster-local-node  32 \
  --cluster-hash-seed 0xdeadbeef -j MARK --set-mark 0xffff
iptables: Invalid argument. Run `dmesg' for more information.
% dmesg | tail -1
xt_cluster: this node mask cannot be higher than the total number of nodes

The problem is related to this checking:

if (info->node_mask >= (1 << info->total_nodes)) {
	printk(KERN_ERR "xt_cluster: this node mask cannot be "
			"higher than the total number of nodes\n");
	return false;
}

(1 << 32) is 1. Thus, the checking fails.

BTW, I said this before but I insist: I have only tested the cluster
match with 2 nodes getting ~45% extra performance in an active-active setup.
The maximum limit of 32 nodes is still completely arbitrary. I'd really
appreciate if people that have more nodes in their setups let me know.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---

 include/linux/netfilter/xt_cluster.h |    2 ++
 net/netfilter/xt_cluster.c           |    8 +++++++-
 2 files changed, 9 insertions(+), 1 deletions(-)


diff --git a/include/linux/netfilter/xt_cluster.h b/include/linux/netfilter/xt_cluster.h
index 5e0a0d0..8866826 100644
--- a/include/linux/netfilter/xt_cluster.h
+++ b/include/linux/netfilter/xt_cluster.h
@@ -12,4 +12,6 @@ struct xt_cluster_match_info {
 	u_int32_t		flags;
 };
 
+#define XT_CLUSTER_NODES_MAX	32
+
 #endif /* _XT_CLUSTER_MATCH_H */
diff --git a/net/netfilter/xt_cluster.c b/net/netfilter/xt_cluster.c
index 6c48476..69a639f 100644
--- a/net/netfilter/xt_cluster.c
+++ b/net/netfilter/xt_cluster.c
@@ -135,7 +135,13 @@ static bool xt_cluster_mt_checkentry(const struct xt_mtchk_param *par)
 {
 	struct xt_cluster_match_info *info = par->matchinfo;
 
-	if (info->node_mask >= (1 << info->total_nodes)) {
+	if (info->total_nodes > XT_CLUSTER_NODES_MAX) {
+		printk(KERN_ERR "xt_cluster: you have exceeded the maximum "
+				"number of cluster nodes (%u > %u)\n",
+				info->total_nodes, XT_CLUSTER_NODES_MAX);
+		return false;
+	}
+	if (info->node_mask >= (1ULL << info->total_nodes)) {
 		printk(KERN_ERR "xt_cluster: this node mask cannot be "
 				"higher than the total number of nodes\n");
 		return false;

[PATCH 3/3] netfilter: conntrack: fix EINVAL during DCCP loading

[Pablo Neira Ayuso]

# modprobe nf_conntrack_proto_dccp
FATAL: Error inserting nf_conntrack_proto_dccp (/lib/modules/2.6.30-rc1-00666-gd4b5cc5/kernel/net/netfilter/nf_conntrack_proto_dccp.ko): Invalid argument

This patch fixes a problem during introduced in commit
a400c30edb1958ceb53c4b8ce78989189b36df47.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---

 net/netfilter/nf_conntrack_proto_dccp.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_dccp.c b/net/netfilter/nf_conntrack_proto_dccp.c
index 3c266e6..8e757dd 100644
--- a/net/netfilter/nf_conntrack_proto_dccp.c
+++ b/net/netfilter/nf_conntrack_proto_dccp.c
@@ -790,6 +790,7 @@ static struct nf_conntrack_l4proto dccp_proto6 __read_mostly = {
 	.print_conntrack	= dccp_print_conntrack,
 #if defined(CONFIG_NF_CT_NETLINK) || defined(CONFIG_NF_CT_NETLINK_MODULE)
 	.to_nlattr		= dccp_to_nlattr,
+	.nlattr_size		= dccp_nlattr_size,
 	.from_nlattr		= nlattr_to_dccp,
 	.tuple_to_nlattr	= nf_ct_port_tuple_to_nlattr,
 	.nlattr_tuple_size	= nf_ct_port_nlattr_tuple_size,