ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.

Signed-off-by Eric Dumazet <[email protected]>

CC Patrick McHardy <[email protected]>

---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

net/ipv4/netfilter/ipt_CLUSTERIP.c +9 -4

index 1e26a48..bac8739 100644
@@ -47,6 +47,7 @@ struct clusterip_config {
 	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
 	struct net_device *dev;			/* device */
 	u_int16_t num_total_nodes;		/* total number of nodes */
+	bool warned_no_conntrack;
 	unsigned long local_nodes;		/* node number array */
 
 #ifdef CONFIG_PROC_FS

@@ -301,10 +302,14 @@ clusterip_tg(struct sk_buff *skb, const struct xt_action_param *par)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct == NULL) {
-		pr_info("no conntrack!\n");
-			/* FIXME: need to drop invalid ones, since replies
-			 * to outgoing connections of other nodes will be
-			 * marked as INVALID */
+		if (unlikely(!cipinfo->config->warned_no_conntrack)) {
+			cipinfo->config->warned_no_conntrack = true;
+			pr_info("no conntrack!\n");
+		}
+		/* FIXME: need to drop invalid ones, since replies
+		 * to outgoing connections of other nodes will be
+		 * marked as INVALID
+		 */
 		return NF_DROP;
 	}

Pablo Neira Ayuso

author maintainer

2011-01-13 11:23 UTC #2 lore raw

Re: [PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

Hi Eric,

Quoted 4 lines · Eric Dumazet

ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.

I think that this should be converted to pr_debug() instead, there's also another reference to "unknown protocol" that should be converted as well.

Patrick McHardy

2011-01-13 11:28 UTC #3 lore raw

Re: [PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

Quoted 11 lines · Pablo Neira Ayuso

Hi Eric,
Quoted 4 lines · Eric Dumazet
ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.
I think that this should be converted to pr_debug() instead, there's also another reference to "unknown protocol" that should be converted as well.

I think the FIXME could also be removed, we *do* drop invalid packets in CLUSTERIP.

Pablo Neira Ayuso

author maintainer

2011-01-13 11:29 UTC #4 lore raw

Re: [PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

Quoted 9 lines · Patrick McHardy

Quoted 8 lines · Pablo Neira Ayuso
Hi Eric,
Quoted 4 lines · Eric Dumazet
ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.

Quoted 6 lines

Quoted 3 lines
I think that this should be converted to pr_debug() instead, there's also another reference to "unknown protocol" that should be converted as well.
I think the FIXME could also be removed, we *do* drop invalid packets in CLUSTERIP.

Hey! You're back! :-)

Eric Dumazet

2011-01-13 11:36 UTC #5 lore raw

Re: [PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

In reply to Patrick McHardy

Quoted 9 lines · Patrick McHardy

Quoted 8 lines · Pablo Neira Ayuso
Hi Eric,
Quoted 4 lines · Eric Dumazet
ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.

Quoted 6 lines

Quoted 3 lines
I think that this should be converted to pr_debug() instead, there's also another reference to "unknown protocol" that should be converted as well.
I think the FIXME could also be removed, we *do* drop invalid packets in CLUSTERIP.

Ah yes indeed :)

Thanks !

[PATCH v3] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

Pablo suggested to use same logic for the "unknown protocol" message Patrick asked to remove an obsolete comment.

Signed-off-by Eric Dumazet <[email protected]>

CC Patrick McHardy <[email protected]>

CC Pablo Neira Ayuso <[email protected]>

---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

net/ipv4/netfilter/ipt_CLUSTERIP.c +11 -7

index 1e26a48..b5cf3e4 100644
@@ -47,6 +47,8 @@ struct clusterip_config {
 	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
 	struct net_device *dev;			/* device */
 	u_int16_t num_total_nodes;		/* total number of nodes */
+	bool warned_no_conntrack;
+	bool warned_unknown_protocol;
 	unsigned long local_nodes;		/* node number array */
 
 #ifdef CONFIG_PROC_FS

@@ -228,7 +230,7 @@ clusterip_del_node(struct clusterip_config *c, u_int16_t nodenum)
 
 static inline u_int32_t
 clusterip_hashfn(const struct sk_buff *skb,
-		 const struct clusterip_config *config)
+		 struct clusterip_config *config)
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	unsigned long hashval;

@@ -236,7 +238,7 @@ clusterip_hashfn(const struct sk_buff *skb,
 	int poff;
 
 	poff = proto_ports_offset(iph->protocol);
-	if (poff >= 0) {
+	if (likely(poff >= 0)) {
 		const u_int16_t *ports;
 		u16 _ports[2];

@@ -246,8 +248,10 @@ clusterip_hashfn(const struct sk_buff *skb,
 			dport = ports[1];
 		}
 	} else {
-		if (net_ratelimit())
+		if (unlikely(!config->warned_unknown_protocol)) {
+			config->warned_unknown_protocol = true;
 			pr_info("unknown protocol %u\n", iph->protocol);
+		}
 	}
 
 	switch (config->hash_mode) {

@@ -301,10 +305,10 @@ clusterip_tg(struct sk_buff *skb, const struct xt_action_param *par)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct == NULL) {
-		pr_info("no conntrack!\n");
-			/* FIXME: need to drop invalid ones, since replies
-			 * to outgoing connections of other nodes will be
-			 * marked as INVALID */
+		if (unlikely(!cipinfo->config->warned_no_conntrack)) {
+			cipinfo->config->warned_no_conntrack = true;
+			pr_info("no conntrack!\n");
+		}
 		return NF_DROP;
 	}

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2011-01-13 11:32 UTC #6 lore raw

Re: [PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

In reply to Pablo Neira Ayuso

Quoted 11 lines · Pablo Neira Ayuso

Hi Eric,
Quoted 4 lines · Eric Dumazet
ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.
I think that this should be converted to pr_debug() instead, there's also another reference to "unknown protocol" that should be converted as well.

Problem is pr_debug() is a noop most of the time,
and printk(KERN_DEBUG is a bit ugly ...

If we print the message once, better to really print it ;)

[PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

Pablo suggested to use same logic for the "unknown protocol" message

Signed-off-by Eric Dumazet <[email protected]>

CC Patrick McHardy <[email protected]>

---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

net/ipv4/netfilter/ipt_CLUSTERIP.c +15 -7

index 1e26a48..2968571 100644
@@ -47,6 +47,8 @@ struct clusterip_config {
 	u_int8_t clustermac[ETH_ALEN];		/* the MAC address */
 	struct net_device *dev;			/* device */
 	u_int16_t num_total_nodes;		/* total number of nodes */
+	bool warned_no_conntrack;
+	bool warned_unknown_protocol;
 	unsigned long local_nodes;		/* node number array */
 
 #ifdef CONFIG_PROC_FS

@@ -228,7 +230,7 @@ clusterip_del_node(struct clusterip_config *c, u_int16_t nodenum)
 
 static inline u_int32_t
 clusterip_hashfn(const struct sk_buff *skb,
-		 const struct clusterip_config *config)
+		 struct clusterip_config *config)
 {
 	const struct iphdr *iph = ip_hdr(skb);
 	unsigned long hashval;

@@ -236,7 +238,7 @@ clusterip_hashfn(const struct sk_buff *skb,
 	int poff;
 
 	poff = proto_ports_offset(iph->protocol);
-	if (poff >= 0) {
+	if (likely(poff >= 0)) {
 		const u_int16_t *ports;
 		u16 _ports[2];

@@ -246,8 +248,10 @@ clusterip_hashfn(const struct sk_buff *skb,
 			dport = ports[1];
 		}
 	} else {
-		if (net_ratelimit())
+		if (unlikely(!config->warned_unknown_protocol)) {
+			config->warned_unknown_protocol = true;
 			pr_info("unknown protocol %u\n", iph->protocol);
+		}
 	}
 
 	switch (config->hash_mode) {

@@ -301,10 +305,14 @@ clusterip_tg(struct sk_buff *skb, const struct xt_action_param *par)
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (ct == NULL) {
-		pr_info("no conntrack!\n");
-			/* FIXME: need to drop invalid ones, since replies
-			 * to outgoing connections of other nodes will be
-			 * marked as INVALID */
+		if (unlikely(!cipinfo->config->warned_no_conntrack)) {
+			cipinfo->config->warned_no_conntrack = true;
+			pr_info("no conntrack!\n");
+		}
+		/* FIXME: need to drop invalid ones, since replies
+		 * to outgoing connections of other nodes will be
+		 * marked as INVALID
+		 */
 		return NF_DROP;
 	}

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer

2011-01-13 11:54 UTC #7 lore raw

Re: [PATCH] netfilter: ipt_CLUSTERIP: dont flood with "no conntrack!"

Quoted 9 lines · Eric Dumazet

Quoted 8 lines · Pablo Neira Ayuso
Hi Eric,
Quoted 4 lines · Eric Dumazet
ipt_CLUSTERIP users might hit this annoying printk, if they forgot an "iptables -I INPUT -m state --state INVALID -j DROP" before CLUSTERIP rule. We could use net_ratelimit() here, or not log the message at all. I chose to log it once per config.

Quoted 8 lines

Quoted 3 lines
I think that this should be converted to pr_debug() instead, there's also another reference to "unknown protocol" that should be converted as well.
Problem is pr_debug() is a noop most of the time,
and printk(KERN_DEBUG is a bit ugly ...
If we print the message once, better to really print it ;)

But printing this does not provide any useful information. The first packet that does not belong to the cluster node that has received the packet, or the first invalid packet, will trigger this.

Moreover, this confuses users since they can do nothing if they receive this message.

Moreover, this target should be supersedes by the cluster match, which has been there for quite some time (it's also more flexible).

Eric Dumazet

2011-01-13 13:38 UTC #8 lore raw

PATCH v4 1/1 netfilter: ipt_CLUSTERIP: remove "no conntrack!"

Quoted 9 lines · Pablo Neira Ayuso

But printing this does not provide any useful information. The first packet that does not belong to the cluster node that has received the packet, or the first invalid packet, will trigger this.
Moreover, this confuses users since they can do nothing if they receive this message.
Moreover, this target should be supersedes by the cluster match, which has been there for quite some time (it's also more flexible).

Now you mentioned it, cluster match is not as flexible right now, its hashing is on source_ip only.

Many people still use ipt_CLUSTERIP, and probably for couple of years.

This issue was raised several times, we should fix CLUSTERIP for good.

Thanks

[PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

When a packet is meant to be handled by another node of the cluster, silently drop it instead of flooding kernel log.

Note : INVALID packets are also dropped without notice.

Signed-off-by Eric Dumazet <[email protected]>

CC Patrick McHardy <[email protected]>

CC Pablo Neira Ayuso <[email protected]>

---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

net/ipv4/netfilter/ipt_CLUSTERIP.c +1 -6

index 1e26a48..403ca57 100644
@@ -300,13 +300,8 @@ clusterip_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	 * that the ->target() function isn't called after ->destroy() */
 
 	ct = nf_ct_get(skb, &ctinfo);
-	if (ct == NULL) {
-		pr_info("no conntrack!\n");
-			/* FIXME: need to drop invalid ones, since replies
-			 * to outgoing connections of other nodes will be
-			 * marked as INVALID */
+	if (ct == NULL)
 		return NF_DROP;
-	}
 
 	/* special case: ICMP error handling. conntrack distinguishes between
 	 * error messages (RELATED) and information requests (see below) */

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jan Engelhardt

2011-01-13 14:02 UTC #9 lore raw

Re: [PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

Quoted 6 lines · Eric Dumazet

Quoted 4 lines · Pablo Neira Ayuso
But printing this does not provide any useful information. The first packet that does not belong to the cluster node that has received the packet, or the first invalid packet, will trigger this.

Quoted 8 lines

Quoted 5 lines
Moreover, this confuses users since they can do nothing if they receive this message.
Moreover, this target should be supersedes by the cluster match, which has been there for quite some time (it's also more flexible).
Now you mentioned it, cluster match is not as flexible right now, its hashing is on source_ip only.

I think in that case, xt_cluster should be improved rather
than an old module.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2011-01-13 14:39 UTC #10 lore raw

Re: [PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

Quoted 11 lines · Jan Engelhardt

Quoted 9 lines · Eric Dumazet
Quoted 7 lines · Pablo Neira Ayuso
But printing this does not provide any useful information. The first packet that does not belong to the cluster node that has received the packet, or the first invalid packet, will trigger this.
Moreover, this confuses users since they can do nothing if they receive this message.

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Moreover, this target should be supersedes by the cluster match, which has been there for quite some time (it's also more flexible).
Now you mentioned it, cluster match is not as flexible right now, its hashing is on source_ip only.
I think in that case, xt_cluster should be improved rather than an old module.

Amen

We should not improve IPv4 support then, I see.

My customers use this old module, and upgrading to xt_cluster is not an option.

Should we discuss this forever or fix it ?

In the end, people are forced to add useless iptables rule to DROP INVALID packets before entering ipt_CLUSTERIP, after googling or eventually asking to experts.

Last time this was discussed, this went nowhere :

http://www.spinics.net/lists/netfilter/msg48676.html

Come on guys, we can do it, dont be afraid.

A non rate limited printk() in kernel is forbidden, especially in network stack.

Then, cluster match can be improved, I am sure you already have a patch for it.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer Acked-by

2011-01-13 16:30 UTC #11 lore raw

Re: [PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

Quoted 21 lines · Eric Dumazet

Quoted 11 lines · Jan Engelhardt
Quoted 9 lines · Eric Dumazet
Quoted 7 lines · Pablo Neira Ayuso
But printing this does not provide any useful information. The first packet that does not belong to the cluster node that has received the packet, or the first invalid packet, will trigger this.
Moreover, this confuses users since they can do nothing if they receive this message.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Moreover, this target should be supersedes by the cluster match, which has been there for quite some time (it's also more flexible).
Now you mentioned it, cluster match is not as flexible right now, its hashing is on source_ip only.
I think in that case, xt_cluster should be improved rather than an old module.

Quoted 8 lines

Amen
We should not improve IPv4 support then, I see.
My customers use this old module, and upgrading to xt_cluster is not an option.
Should we discuss this forever or fix it ?

hey hey, I'm fine with fixing things. Patch v4 is OK.

Acked-by Pablo Neira Ayuso <[email protected]>

Quoted 8 lines

In the end, people are forced to add useless iptables rule to DROP INVALID packets before entering ipt_CLUSTERIP, after googling or eventually asking to experts.
Last time this was discussed, this went nowhere :
http://www.spinics.net/lists/netfilter/msg48676.html

Quoted 7 lines

Come on guys, we can do it, dont be afraid.
A non rate limited printk() in kernel is forbidden, especially in network stack.
Then, cluster match can be improved, I am sure you already have a patch for it.

what scenario could benefit from the destination-based hashing? --
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso

author maintainer

2011-01-13 16:35 UTC #12 lore raw

Re: [PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

Quoted 5 lines · Pablo Neira Ayuso

Quoted 2 lines · Eric Dumazet
Then, cluster match can be improved, I am sure you already have a patch for it.
what scenario could benefit from the destination-based hashing?

I'm telling this because it doesn't make too sense to me.

Eric Dumazet

2011-01-13 16:48 UTC #13 lore raw

Re: [PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

Quoted 8 lines · Pablo Neira Ayuso

Quoted 5 lines · Pablo Neira Ayuso
Quoted 2 lines · Eric Dumazet
Then, cluster match can be improved, I am sure you already have a patch for it.
what scenario could benefit from the destination-based hashing?
I'm telling this because it doesn't make too sense to me.

Me too ;)

But hash(source_IP, source_PORT) definitely make sense in some workloads.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

Applied

2011-01-18 15:28 UTC #14 lore raw

Re: [PATCH v4] netfilter: ipt_CLUSTERIP: remove "no conntrack!"

In reply to Pablo Neira Ayuso

Quoted 4 lines · Pablo Neira Ayuso

On 13/01/11 15:39, Eric Dumazet wrote:
hey hey, I'm fine with fixing things. Patch v4 is OK.
Acked-by Pablo Neira Ayuso <[email protected]>

Applied, thanks.