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

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

[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.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Patrick McHardy <kaber@trash.net>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |   13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 1e26a48..bac8739 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -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;
 	}
 

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

[Pablo Neira Ayuso]

Hi Eric,

On 13/01/11 12:13, Eric Dumazet wrote:
> 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.

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

[Patrick McHardy]

On 13.01.2011 12:23, Pablo Neira Ayuso wrote:
> Hi Eric,
> 
> On 13/01/11 12:13, Eric Dumazet wrote:
>> 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.

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

[Pablo Neira Ayuso]

On 13/01/11 12:28, Patrick McHardy wrote:
> On 13.01.2011 12:23, Pablo Neira Ayuso wrote:
>> Hi Eric,
>>
>> On 13/01/11 12:13, Eric Dumazet wrote:
>>> 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.

Hey! You're back! :-)

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

[Eric Dumazet]

Le jeudi 13 janvier 2011 à 12:28 +0100, Patrick McHardy a écrit :
> On 13.01.2011 12:23, Pablo Neira Ayuso wrote:
> > Hi Eric,
> > 
> > On 13/01/11 12:13, Eric Dumazet wrote:
> >> 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.

Ah yes indeed :)

Thanks !

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

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.

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

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Patrick McHardy <kaber@trash.net>
CC: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 1e26a48..b5cf3e4 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Dumazet]

Le jeudi 13 janvier 2011 à 12:23 +0100, Pablo Neira Ayuso a écrit :
> Hi Eric,
> 
> On 13/01/11 12:13, Eric Dumazet wrote:
> > 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!"

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.

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

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Patrick McHardy <kaber@trash.net>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 1e26a48..2968571 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On 13/01/11 12:32, Eric Dumazet wrote:
> Le jeudi 13 janvier 2011 à 12:23 +0100, Pablo Neira Ayuso a écrit :
>> Hi Eric,
>>
>> On 13/01/11 12:13, Eric Dumazet wrote:
>>> 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 ;)

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).

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

[Eric Dumazet]

Le jeudi 13 janvier 2011 à 12:54 +0100, Pablo Neira Ayuso a écrit :

> 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 <eric.dumazet@gmail.com>
CC: Patrick McHardy <kaber@trash.net>
CC: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/ipv4/netfilter/ipt_CLUSTERIP.c |    7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 1e26a48..403ca57 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Engelhardt]

On Thursday 2011-01-13 14:38, Eric Dumazet wrote:

>Le jeudi 13 janvier 2011 à 12:54 +0100, Pablo Neira Ayuso a écrit :
>
>> 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.

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Dumazet]

Le jeudi 13 janvier 2011 à 15:02 +0100, Jan Engelhardt a écrit :
> On Thursday 2011-01-13 14:38, Eric Dumazet wrote:
> 
> >Le jeudi 13 janvier 2011 à 12:54 +0100, Pablo Neira Ayuso a écrit :
> >
> >> 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.
> 
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On 13/01/11 15:39, Eric Dumazet wrote:
> Le jeudi 13 janvier 2011 à 15:02 +0100, Jan Engelhardt a écrit :
>> On Thursday 2011-01-13 14:38, Eric Dumazet wrote:
>>
>>> Le jeudi 13 janvier 2011 à 12:54 +0100, Pablo Neira Ayuso a écrit :
>>>
>>>> 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.
>>
>> 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 ?

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

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

> 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.

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pablo Neira Ayuso]

On 13/01/11 17:30, Pablo Neira Ayuso wrote:
> On 13/01/11 15:39, Eric Dumazet wrote:
>> 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.

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

[Eric Dumazet]

Le jeudi 13 janvier 2011 à 17:35 +0100, Pablo Neira Ayuso a écrit :
> On 13/01/11 17:30, Pablo Neira Ayuso wrote:
> > On 13/01/11 15:39, Eric Dumazet wrote:
> >> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Patrick McHardy]

On 13.01.2011 17:30, Pablo Neira Ayuso wrote:
> 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 <pablo@netfilter.org>

Applied, thanks.