Re: netfilter spurious ELOOP

Re: netfilter spurious ELOOP

[David Miller]

From: Francis Dupont <Francis.Dupont@fdupont.fr>
Date: Wed, 25 Mar 2009 00:02:05 +0100

Adding correct CC:'s

> summary: iptables command gets spurious ELOOP errors
> 
> report: when a rule with a target like MARK --set-mark 0x80000001
> then adding new other rules can failed with "Too many levels of symbolic
> links" (aka ELOOP) error.
> The problem is in kernel net/ipv4/netfilter/ip_tables.c in the 
> mark_source_chains() routine which checks the verdict field of
> targets even for not standard targets.
> 
> keywords: netfilter target eloop
> 
> environment: recent gentoo and fedora. Problem not fixed in
> linux-2.6.29 (last stable version taken from kernel.org some minutes ago).
> 
> proposed fix (checked):
> diff --unified=10 net/ipv4/netfilter/ip_tables.c*
> at the end of the message.
> 
> request: can you send to me at both my personal and professional addresses
> a bug/ticket number as soon as possible?
> 
> Request
> 
> Francis.Dupont@fdupont.fr
> 
> PS: the patch:
> 
> --- net/ipv4/netfilter/ip_tables.c	2009-03-23 16:12:14.000000000 -0700
> +++ net/ipv4/netfilter/ip_tables.c+fix	2009-03-24 15:55:45.000000000 -0700
> @@ -489,21 +489,23 @@
>  			e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
>  
>  			/* Unconditional return/END. */
>  			if ((e->target_offset == sizeof(struct ipt_entry)
>  			    && (strcmp(t->target.u.user.name,
>  				       IPT_STANDARD_TARGET) == 0)
>  			    && t->verdict < 0
>  			    && unconditional(&e->ip)) || visited) {
>  				unsigned int oldpos, size;
>  
> -				if (t->verdict < -NF_MAX_VERDICT - 1) {
> +				if ((t->verdict < -NF_MAX_VERDICT - 1) &&
> +				    (strcmp(t->target.u.user.name,
> +					    IPT_STANDARD_TARGET) == 0)) {
>  					duprintf("mark_source_chains: bad "
>  						"negative verdict (%i)\n",
>  								t->verdict);
>  					return 0;
>  				}
>  
>  				/* Return: backtrack through the last
>  				   big jump. */
>  				do {
>  					e->comefrom ^= (1<<NF_INET_NUMHOOKS);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: netfilter spurious ELOOP

[Patrick McHardy]

David Miller wrote:
> From: Francis Dupont <Francis.Dupont@fdupont.fr>
> Date: Wed, 25 Mar 2009 00:02:05 +0100
> 
> Adding correct CC:'s
> 
>> summary: iptables command gets spurious ELOOP errors
>>
>> report: when a rule with a target like MARK --set-mark 0x80000001
>> then adding new other rules can failed with "Too many levels of symbolic
>> links" (aka ELOOP) error.
>> The problem is in kernel net/ipv4/netfilter/ip_tables.c in the 
>> mark_source_chains() routine which checks the verdict field of
>> targets even for not standard targets.
>>
>> keywords: netfilter target eloop
>>
>> environment: recent gentoo and fedora. Problem not fixed in
>> linux-2.6.29 (last stable version taken from kernel.org some minutes ago).

Just to clarify: does the problem happens when you have the MARK rule
above in a user-defined chain that has more then one jump leading to
it or does it also happen in other cases?

Re: netfilter spurious ELOOP

[Francis Dupont]

> Just to clarify: does the problem happens when you have the MARK rule
> above in a user-defined chain that has more then one jump leading to
> it or does it also happen in other cases?

=> I triggered the bug with a real world example:
 - first add a rule with a MARK target using a set mark with the first/sign
  bit set to one. This target is coded with this mark put at the same
  place than the verdict field of standard targets. (note this should
  be triggered by a lot of targets but I got it with MARK)
 - try to add another rule (with -A or -I but this works too with restore,
  the idea is to get a replace ioctl with an illegal value in a verdict
  position).
 - if you are (un?)lucky you get the ELOOP error.

If you read my proposed fix the problem is pretty easy to understand.
I asked diff to give enough context for human (i.e., more than needed
to apply it as a patch).

Thanks

Francis_Dupont@isc.org

PS: I really need a bug-ticket-etc number because some business is implied
(BTW IMHO you prefer to get the report once and by the most direct path,
don't you?)
PPS: here I've cut & paste the config I used to track the bug:

-------------------------------- save file --------------------------------
# Generated by iptables-save v1.4.2 on Tue Mar 24 18:54:43 2009
*filter
:INPUT ACCEPT [11843:1222672]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [7216:1221713]
COMMIT
# Completed on Tue Mar 24 18:54:43 2009
# Generated by iptables-save v1.4.2 on Tue Mar 24 18:54:43 2009
*mangle
:PREROUTING ACCEPT [1209557:93278988]
:INPUT ACCEPT [1209182:93208843]
:FORWARD ACCEPT [0:0]
:OUTPUT ACCEPT [668677:2806960697]
:POSTROUTING ACCEPT [668677:2806960697]
:MARKOUT1 - [0:0]
-A PREROUTING -d 10.0.200.2/32 -p tcp -m tcp --dport 5001 -j MARKOUT1 
-A MARKOUT1 -j MARK --set-xmark 0x80000001/0xffffffff 
-A MARKOUT1 -j CONNMARK --save-mark --nfmask 0x3fffffff --ctmask 0x3fffffff 
-A MARKOUT1 -j ACCEPT 
COMMIT
# Completed on Tue Mar 24 18:54:43 2009
-------------------------------- cut here  --------------------------------

I got the bug with the UDP counterpart:

iptables -t mangle -A PREROUTING -d 10.0.200.2/32 -p udp --dport 5001 \
-j MARKOUT1

Re: netfilter spurious ELOOP

[Patrick McHardy]

Francis Dupont wrote:
>> Just to clarify: does the problem happens when you have the MARK rule
>> above in a user-defined chain that has more then one jump leading to
>> it or does it also happen in other cases?
> 
> => I triggered the bug with a real world example:
>  - first add a rule with a MARK target using a set mark with the first/sign
>   bit set to one. This target is coded with this mark put at the same
>   place than the verdict field of standard targets. (note this should
>   be triggered by a lot of targets but I got it with MARK)
>  - try to add another rule (with -A or -I but this works too with restore,
>   the idea is to get a replace ioctl with an illegal value in a verdict
>   position).
>  - if you are (un?)lucky you get the ELOOP error.
>
> PS: I really need a bug-ticket-etc number because some business is implied

I'm not a service center, sorry :) Feel free to create an entry in
the netfilter bugzilla, I'll mark it resolved once the patch is
upstream.

> PPS: here I've cut & paste the config I used to track the bug:#
> ....
> :MARKOUT1 - [0:0]
> -A PREROUTING -d 10.0.200.2/32 -p tcp -m tcp --dport 5001 -j MARKOUT1 
> -A MARKOUT1 -j MARK --set-xmark 0x80000001/0xffffffff 
> -A MARKOUT1 -j CONNMARK --save-mark --nfmask 0x3fffffff --ctmask 0x3fffffff 
> -A MARKOUT1 -j ACCEPT 
> 
> I got the bug with the UDP counterpart:
> 
> iptables -t mangle -A PREROUTING -d 10.0.200.2/32 -p udp --dport 5001 \
> -j MARKOUT1

Thanks, that answers my question. I'll apply your patch and send it to
-stable once its in the mainline kernel.

Re: netfilter spurious ELOOP

[Patrick McHardy]

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

Patrick McHardy wrote:
> Thanks, that answers my question. I'll apply your patch and send it to
> -stable once its in the mainline kernel.

The same bug was also present in ip6_tables and arp_tables.
This is the patch I've committed:



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2774 bytes --]

commit 1f9352ae2253a97b07b34dcf16ffa3b4ca12c558
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed Mar 25 19:26:35 2009 +0100

    netfilter: {ip,ip6,arp}_tables: fix incorrect loop detection
    
    Commit e1b4b9f ([NETFILTER]: {ip,ip6,arp}_tables: fix exponential worst-case
    search for loops) introduced a regression in the loop detection algorithm,
    causing sporadic incorrectly detected loops.
    
    When a chain has already been visited during the check, it is treated as
    having a standard target containing a RETURN verdict directly at the
    beginning in order to not check it again. The real target of the first
    rule is then incorrectly treated as STANDARD target and checked not to
    contain invalid verdicts.
    
    Fix by making sure the rule does actually contain a standard target.
    
    Based on patch by Francis Dupont <Francis_Dupont@isc.org>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 4b35dba..4f454ce 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -388,7 +388,9 @@ static int mark_source_chains(struct xt_table_info *newinfo,
 			    && unconditional(&e->arp)) || visited) {
 				unsigned int oldpos, size;
 
-				if (t->verdict < -NF_MAX_VERDICT - 1) {
+				if ((strcmp(t->target.u.user.name,
+					    ARPT_STANDARD_TARGET) == 0) &&
+				    t->verdict < -NF_MAX_VERDICT - 1) {
 					duprintf("mark_source_chains: bad "
 						"negative verdict (%i)\n",
 								t->verdict);
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 41c59e3..82ee7c9 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -488,7 +488,9 @@ mark_source_chains(struct xt_table_info *newinfo,
 			    && unconditional(&e->ip)) || visited) {
 				unsigned int oldpos, size;
 
-				if (t->verdict < -NF_MAX_VERDICT - 1) {
+				if ((strcmp(t->target.u.user.name,
+			    		    IPT_STANDARD_TARGET) == 0) &&
+				    t->verdict < -NF_MAX_VERDICT - 1) {
 					duprintf("mark_source_chains: bad "
 						"negative verdict (%i)\n",
 								t->verdict);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index e59662b..e89cfa3 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -517,7 +517,9 @@ mark_source_chains(struct xt_table_info *newinfo,
 			    && unconditional(&e->ipv6)) || visited) {
 				unsigned int oldpos, size;
 
-				if (t->verdict < -NF_MAX_VERDICT - 1) {
+				if ((strcmp(t->target.u.user.name,
+					    IP6T_STANDARD_TARGET) == 0) &&
+				    t->verdict < -NF_MAX_VERDICT - 1) {
 					duprintf("mark_source_chains: bad "
 						"negative verdict (%i)\n",
 								t->verdict);

Re: netfilter spurious ELOOP

[Patrick McHardy]

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

Patrick McHardy wrote:
> The same bug was also present in ip6_tables and arp_tables.
> This is the patch I've committed:

Thomas, I think this is likely to fix the ELOOP problem you reported
in 2007 that we've never managed to track down:


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2774 bytes --]

commit 1f9352ae2253a97b07b34dcf16ffa3b4ca12c558
Author: Patrick McHardy <kaber@trash.net>
Date:   Wed Mar 25 19:26:35 2009 +0100

    netfilter: {ip,ip6,arp}_tables: fix incorrect loop detection
    
    Commit e1b4b9f ([NETFILTER]: {ip,ip6,arp}_tables: fix exponential worst-case
    search for loops) introduced a regression in the loop detection algorithm,
    causing sporadic incorrectly detected loops.
    
    When a chain has already been visited during the check, it is treated as
    having a standard target containing a RETURN verdict directly at the
    beginning in order to not check it again. The real target of the first
    rule is then incorrectly treated as STANDARD target and checked not to
    contain invalid verdicts.
    
    Fix by making sure the rule does actually contain a standard target.
    
    Based on patch by Francis Dupont <Francis_Dupont@isc.org>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 4b35dba..4f454ce 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -388,7 +388,9 @@ static int mark_source_chains(struct xt_table_info *newinfo,
 			    && unconditional(&e->arp)) || visited) {
 				unsigned int oldpos, size;
 
-				if (t->verdict < -NF_MAX_VERDICT - 1) {
+				if ((strcmp(t->target.u.user.name,
+					    ARPT_STANDARD_TARGET) == 0) &&
+				    t->verdict < -NF_MAX_VERDICT - 1) {
 					duprintf("mark_source_chains: bad "
 						"negative verdict (%i)\n",
 								t->verdict);
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 41c59e3..82ee7c9 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -488,7 +488,9 @@ mark_source_chains(struct xt_table_info *newinfo,
 			    && unconditional(&e->ip)) || visited) {
 				unsigned int oldpos, size;
 
-				if (t->verdict < -NF_MAX_VERDICT - 1) {
+				if ((strcmp(t->target.u.user.name,
+			    		    IPT_STANDARD_TARGET) == 0) &&
+				    t->verdict < -NF_MAX_VERDICT - 1) {
 					duprintf("mark_source_chains: bad "
 						"negative verdict (%i)\n",
 								t->verdict);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index e59662b..e89cfa3 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -517,7 +517,9 @@ mark_source_chains(struct xt_table_info *newinfo,
 			    && unconditional(&e->ipv6)) || visited) {
 				unsigned int oldpos, size;
 
-				if (t->verdict < -NF_MAX_VERDICT - 1) {
+				if ((strcmp(t->target.u.user.name,
+					    IP6T_STANDARD_TARGET) == 0) &&
+				    t->verdict < -NF_MAX_VERDICT - 1) {
 					duprintf("mark_source_chains: bad "
 						"negative verdict (%i)\n",
 								t->verdict);

Re: netfilter spurious ELOOP

[Thomas Jarosch]

Hi Patrick,

On Wednesday, 25. March 2009 19:41:49 Patrick McHardy wrote:
> Patrick McHardy wrote:
> > The same bug was also present in ip6_tables and arp_tables.
> > This is the patch I've committed:
>
> Thomas, I think this is likely to fix the ELOOP problem you reported
> in 2007 that we've never managed to track down:

Yes, you are right. It took me some hours to resurrect the original
setup from 2007 and trigger the problem. The patch fixed the issue :-)

Tested-by: Thomas Jarosch <thomas.jarosch@intra2net.com>

Thanks,
Thomas

Re: netfilter spurious ELOOP

[Patrick McHardy]

Thomas Jarosch wrote:
> Hi Patrick,
> 
> On Wednesday, 25. March 2009 19:41:49 Patrick McHardy wrote:
>> Patrick McHardy wrote:
>>> The same bug was also present in ip6_tables and arp_tables.
>>> This is the patch I've committed:
>> Thomas, I think this is likely to fix the ELOOP problem you reported
>> in 2007 that we've never managed to track down:
> 
> Yes, you are right. It took me some hours to resurrect the original
> setup from 2007 and trigger the problem. The patch fixed the issue :-)
> 
> Tested-by: Thomas Jarosch <thomas.jarosch@intra2net.com>

Thanks for the feedback.