few more fixes for iproute2/m_ipt

few more fixes for iproute2/m_ipt

[Denys Fedoryshchenko]

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

Comments:
1)optind according iptables sources have to be set to 0. If it is set to 1, in 
batch it will mess up things. Also in iptables sources i notice that ->tflags 
and ->used need to be reset.

2)Since target->t = fw_calloc(1, size); allocated memory in function build_st, 
it have to be freed at the end, or in batch we will have memory leak. TODO: 
Probably it must be freed in all "return -1" cases in parse_ipt after 
build_st. About this i am not sure, up to Stephen.

3)lname and new_name was malloc'ed, but not freed

Point 1 fix is critical, since without it m_ipt crashing iproute2 in batch 
mode.

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

diff -Naur iproute2-original/tc/m_ipt.c iproute2-patched/tc/m_ipt.c
--- iproute2-original/tc/m_ipt.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/m_ipt.c	2008-08-05 21:39:49.000000000 +0000
@@ -277,6 +277,8 @@
 		if (!handle) {
 			fputs(dlerror(), stderr);
 			printf("\n");
+			free(l_name);
+			free(new_name);
 			return NULL;
 		}
 	}
@@ -292,12 +294,16 @@
 					fputs(error, stderr);
 					fprintf(stderr, "\n");
 					dlclose(handle);
+					free(l_name);
+					free(new_name);
 					return NULL;
 				}
 			}
 		}
 	}
 
+	free(l_name);
+	free(new_name);
 	return m;
 }
 
@@ -512,8 +518,15 @@
 	*argc_p = rargc - iargc;
 	*argv_p = argv;
 
-	optind = 1;
+	optind = 0;
 	free_opts(opts);
+	/* Clear flags if target will be used again */
+        m->tflags=0;
+        m->used=0;
+	/* Free allocated memory */
+        if (m->t)
+            free(m->t);
+
 
 	return 0;
 

Re: few more fixes for iproute2/m_ipt

[jamal]

Denys,

On Wed, 2008-06-08 at 00:42 +0300, Denys Fedoryshchenko wrote:
> Comments:
> 1)optind according iptables sources have to be set to 0. If it is set to 1, in 
> batch it will mess up things. Also in iptables sources i notice that ->tflags 
> and ->used need to be reset.

Indeed there seems to be an issue here, but:
Have you tried in your batch with a few "index"ed parameters to ipt with
your fix? I think it will break with your changes. Of course that does
not invalidate your concern. If you send me a two liner batch example
which breaks for you, I will test it.

> 2)Since target->t = fw_calloc(1, size); allocated memory in function build_st, 
> it have to be freed at the end, or in batch we will have memory leak. TODO: 
> Probably it must be freed in all "return -1" cases in parse_ipt after 
> build_st. About this i am not sure, up to Stephen.
> 
> 3)lname and new_name was malloc'ed, but not freed
> 
> Point 1 fix is critical, since without it m_ipt crashing iproute2 in batch 
> mode.

I think #2 and #3 are leggit. Send two patches - one for #1 when you
validate and the other for #2,#3 when you validate. BTW, for the
equivalent of the latter - if you feel brave - theres a lot more of that
type in the tc code.

cheers,
jamal

Re: few more fixes for iproute2/m_ipt

[Denys Fedoryshchenko]

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

On Wednesday 06 August 2008, jamal wrote:
>
> Indeed there seems to be an issue here, but:
> Have you tried in your batch with a few "index"ed parameters to ipt with
> your fix? I think it will break with your changes. Of course that does
> not invalidate your concern. If you send me a two liner batch example
> which breaks for you, I will test it.
not sure about two lines
attaching example batch

>
> > 2)Since target->t = fw_calloc(1, size); allocated memory in function
> > build_st, it have to be freed at the end, or in batch we will have memory
> > leak. TODO: Probably it must be freed in all "return -1" cases in
> > parse_ipt after build_st. About this i am not sure, up to Stephen.
> >
> > 3)lname and new_name was malloc'ed, but not freed
> >
> > Point 1 fix is critical, since without it m_ipt crashing iproute2 in
> > batch mode.
>
> I think #2 and #3 are leggit. Send two patches - one for #1 when you
> validate and the other for #2,#3 when you validate. BTW, for the
> equivalent of the latter - if you feel brave - theres a lot more of that
> type in the tc code.
#1 is validated 100%, because i was not able to run tc in batch mode for my 
tasks(2nd or 3rd ipt rule will crash), till i implement those patches. The 
batch i gave is short version, full version have around 11k lines. But it 
crashes usually max on 2rd or 3nd ipt.

Well, i can fix what i can sniff&smell in my daily job, and especially if i 
hit a wall because of some code :-) For me it is difficult to inspect the 
code, since i am very weak in C, but i will try. 



[-- Attachment #2: batch.example --]
[-- Type: text/x-java, Size: 6348 bytes --]

filter del dev ifb0 protocol ip pref 100
class del dev ifb0 classid 1:100 
qdisc del dev ppp0 root
qdisc add dev ppp0 root handle 1: prio
qdisc add dev ppp0 parent 1:1 handle 2: est 1sec 8sec tbf buffer 2048000 latency 500ms rate 256000bit peakrate 1280000bit mtu 1512
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.24/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.26/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.6/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.19/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.102/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.105/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.106/32 flowid 3:1
filter add dev ppp0 parent 1:0 protocol ip prio 10 u32 match ip dst 0.0.0.0/0 flowid 2:1
class add dev ifb0 classid 1:100 parent 1:0 htb rate 56000bit quantum 1600
qdisc add dev ifb0 parent 1:100 handle 100: bfifo limit 14000
filter add dev ifb0 protocol ip pref 100 parent 1: handle 100 fw classid 1:100
qdisc del dev ppp0 ingress
qdisc add dev ppp0 ingress
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.102/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.105/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.106/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.24/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.6/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.26/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.19/32 flowid 1:1
filter add dev ppp0 parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:1 action ipt -j MARK --set-mark 100 action mirred egress redirect dev ifb0
filter del dev ifb0 protocol ip pref 101
class del dev ifb0 classid 1:101 
qdisc del dev ppp1 root
qdisc add dev ppp1 root handle 1: prio
qdisc add dev ppp1 parent 1:1 handle 2: est 1sec 8sec tbf buffer 2048000 latency 500ms rate 256000bit peakrate 1280000bit mtu 1512
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.24/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.26/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.6/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.19/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.102/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.105/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.106/32 flowid 3:1
filter add dev ppp1 parent 1:0 protocol ip prio 10 u32 match ip dst 0.0.0.0/0 flowid 2:1
class add dev ifb0 classid 1:101 parent 1:0 htb rate 56000bit quantum 1600
qdisc add dev ifb0 parent 1:101 handle 101: bfifo limit 14000
filter add dev ifb0 protocol ip pref 101 parent 1: handle 101 fw classid 1:101
qdisc del dev ppp1 ingress
qdisc add dev ppp1 ingress
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.102/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.105/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.106/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.24/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.6/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.26/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.19/32 flowid 1:1
filter add dev ppp1 parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:1 action ipt -j MARK --set-mark 101 action mirred egress redirect dev ifb0
filter del dev ifb0 protocol ip pref 110
class del dev ifb0 classid 1:110 
qdisc del dev ppp10 root
qdisc add dev ppp10 root handle 1: prio
qdisc add dev ppp10 parent 1:1 handle 2: est 1sec 8sec tbf buffer 2048000 latency 500ms rate 256000bit peakrate 1280000bit mtu 1512
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.24/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.26/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.6/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 194.146.153.19/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.102/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.105/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 5 u32 match ip src 2.2.2.106/32 flowid 3:1
filter add dev ppp10 parent 1:0 protocol ip prio 10 u32 match ip dst 0.0.0.0/0 flowid 2:1
class add dev ifb0 classid 1:110 parent 1:0 htb rate 56000bit quantum 1600
qdisc add dev ifb0 parent 1:110 handle 110: bfifo limit 14000
filter add dev ifb0 protocol ip pref 110 parent 1: handle 110 fw classid 1:110
qdisc del dev ppp10 ingress
qdisc add dev ppp10 ingress
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.102/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.105/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 2.2.2.106/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.24/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.6/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.26/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 5 u32 match ip dst 194.146.153.19/32 flowid 1:1
filter add dev ppp10 parent ffff: protocol ip prio 10 u32 match u32 0 0 flowid 1:1 action ipt -j MARK --set-mark 110 action mirred egress redirect dev ifb0

Re: few more fixes for iproute2/m_ipt

[jamal]

On Wed, 2008-06-08 at 13:21 +0300, Denys Fedoryshchenko wrote:

> #1 is validated 100%, because i was not able to run tc in batch mode for my 
> tasks(2nd or 3rd ipt rule will crash), till i implement those patches. The 
> batch i gave is short version, full version have around 11k lines. But it 
> crashes usually max on 2rd or 3nd ipt.

Ok, thanks - let me take a closer look. 
Actually i dont doubt that when you batch the optind maybe shaky - I am
more worried about other working scenarios that may have depended on it.
I will still say put #2 and #3 in separate patch.

> Well, i can fix what i can sniff&smell in my daily job, and especially if i 
> hit a wall because of some code :-) For me it is difficult to inspect the 
> code, since i am very weak in C, but i will try. 

I understand it could be frustrating sometimes but given
your situation you are doing fantastic work. It is highly appreciated.

cheers,
jamal

Re: few more fixes for iproute2/m_ipt

[Denys Fedoryshchenko]

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

On Wednesday 06 August 2008, jamal wrote:
> Ok, thanks - let me take a closer look.
> Actually i dont doubt that when you batch the optind maybe shaky - I am
> more worried about other working scenarios that may have depended on it.
> I will still say put #2 and #3 in separate patch.
>
Is it better like this?

Sure optind is up to you, for me it is not clear yet how it works.

Just i notice in iptables.c, function do_command where they set optind to zero 
and in comments /*re-set optind to zero in case do_command gets called second 
time */


[-- Attachment #2: 01-iproute-memleak_fix.patch --]
[-- Type: text/x-diff, Size: 832 bytes --]

diff -Naur iproute2-original/tc/m_ipt.c iproute2-patched/tc/m_ipt.c
--- iproute2-original/tc/m_ipt.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/m_ipt.c	2008-08-06 11:13:47.000000000 +0000
@@ -277,6 +277,8 @@
 		if (!handle) {
 			fputs(dlerror(), stderr);
 			printf("\n");
+			free(l_name);
+			free(new_name);
 			return NULL;
 		}
 	}
@@ -292,12 +294,16 @@
 					fputs(error, stderr);
 					fprintf(stderr, "\n");
 					dlclose(handle);
+					free(l_name);
+					free(new_name);
 					return NULL;
 				}
 			}
 		}
 	}
 
+	free(l_name);
+	free(new_name);
 	return m;
 }
 
@@ -514,6 +520,13 @@
 
 	optind = 1;
 	free_opts(opts);
+	/* Clear flags if target will be used again */
+        m->tflags=0;
+        m->used=0;
+	/* Free allocated memory */
+        if (m->t)
+            free(m->t);
+
 
 	return 0;
 

[-- Attachment #3: 02-iproute-optind_zero.patch --]
[-- Type: text/x-diff, Size: 390 bytes --]

diff -Naur iproute2-patched/tc/m_ipt.c iproute2-patched2/tc/m_ipt.c
--- iproute2-patched/tc/m_ipt.c	2008-08-06 11:13:47.000000000 +0000
+++ iproute2-patched2/tc/m_ipt.c	2008-08-06 11:14:58.000000000 +0000
@@ -518,7 +518,7 @@
 	*argc_p = rargc - iargc;
 	*argv_p = argv;
 
-	optind = 1;
+	optind = 0;
 	free_opts(opts);
 	/* Clear flags if target will be used again */
         m->tflags=0;

Re: few more fixes for iproute2/m_ipt

[jamal]

On Wed, 2008-06-08 at 14:26 +0300, Denys Fedoryshchenko wrote:

> Is it better like this?
> 
> Sure optind is up to you, for me it is not clear yet how it works.
> 
> Just i notice in iptables.c, function do_command where they set optind to zero 
> and in comments /*re-set optind to zero in case do_command gets called second 
> time */

I have done extensive testing with and with optind=0 and optind=1 and i
didnt see any breakage with either.

I have a feeling that setting optind to 0 in your case to avoid the
crash maybe hiding something else - but i cant find what that something
else is since i am just simulating what you are doing.
If the iptables folks have changed it to reset to 0, then I dont see any
harm in resetting. 

So ACK to both your patches.

cheers,
jamal
PS:- dont wanna sound anal - and you dont have to do this if you dont
have time; but if you put the resetting of optind and the flags in a
separate patch from the freeing, that would be even better.

Re: few more fixes for iproute2/m_ipt

[Denys Fedoryshchenko]

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

On Wednesday 06 August 2008, jamal wrote:
> cheers,
> jamal
> PS:- dont wanna sound anal - and you dont have to do this if you dont
> have time; but if you put the resetting of optind and the flags in a
> separate patch from the freeing, that would be even better.
Yes, i did already
Attaching patches



[-- Attachment #2: 01-iproute-memleak_fix.patch --]
[-- Type: text/x-diff, Size: 832 bytes --]

diff -Naur iproute2-original/tc/m_ipt.c iproute2-patched/tc/m_ipt.c
--- iproute2-original/tc/m_ipt.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/m_ipt.c	2008-08-06 11:13:47.000000000 +0000
@@ -277,6 +277,8 @@
 		if (!handle) {
 			fputs(dlerror(), stderr);
 			printf("\n");
+			free(l_name);
+			free(new_name);
 			return NULL;
 		}
 	}
@@ -292,12 +294,16 @@
 					fputs(error, stderr);
 					fprintf(stderr, "\n");
 					dlclose(handle);
+					free(l_name);
+					free(new_name);
 					return NULL;
 				}
 			}
 		}
 	}
 
+	free(l_name);
+	free(new_name);
 	return m;
 }
 
@@ -514,6 +520,13 @@
 
 	optind = 1;
 	free_opts(opts);
+	/* Clear flags if target will be used again */
+        m->tflags=0;
+        m->used=0;
+	/* Free allocated memory */
+        if (m->t)
+            free(m->t);
+
 
 	return 0;
 

[-- Attachment #3: 02-iproute-optind_zero.patch --]
[-- Type: text/x-diff, Size: 390 bytes --]

diff -Naur iproute2-patched/tc/m_ipt.c iproute2-patched2/tc/m_ipt.c
--- iproute2-patched/tc/m_ipt.c	2008-08-06 11:13:47.000000000 +0000
+++ iproute2-patched2/tc/m_ipt.c	2008-08-06 11:14:58.000000000 +0000
@@ -518,7 +518,7 @@
 	*argc_p = rargc - iargc;
 	*argv_p = argv;
 
-	optind = 1;
+	optind = 0;
 	free_opts(opts);
 	/* Clear flags if target will be used again */
         m->tflags=0;

Re: few more fixes for iproute2/m_ipt

[jamal]

On Wed, 2008-06-08 at 15:50 +0300, Denys Fedoryshchenko wrote:

> +       /* Clear flags if target will be used again */
> +        m->tflags=0;
> +        m->used=0;
> +       /* Free allocated memory */
> +        if (m->t)
> +            free(m->t);
> + 

I meant moving the above to 02-iproute-optind_zero.patch

No big deal if you dont have time. 

cheers,
jamal

Re: few more fixes for iproute2/m_ipt

[Denys Fedoryshchenko]

On Wednesday 06 August 2008, jamal wrote:
> On Wed, 2008-06-08 at 15:50 +0300, Denys Fedoryshchenko wrote:
> > +       /* Clear flags if target will be used again */
> > +        m->tflags=0;
> > +        m->used=0;
> > +       /* Free allocated memory */
> > +        if (m->t)
> > +            free(m->t);
> > +
>
> I meant moving the above to 02-iproute-optind_zero.patch
>
> No big deal if you dont have time.
>
> cheers,
> jamal

No problem with that, i will do now patch.

Re: few more fixes for iproute2/m_ipt

[Denys Fedoryshchenko]

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

On Wednesday 06 August 2008, jamal wrote:
> On Wed, 2008-06-08 at 15:50 +0300, Denys Fedoryshchenko wrote:
> > +       /* Clear flags if target will be used again */
> > +        m->tflags=0;
> > +        m->used=0;
> > +       /* Free allocated memory */
> > +        if (m->t)
> > +            free(m->t);
> > +
>
> I meant moving the above to 02-iproute-optind_zero.patch
>
> No big deal if you dont have time.
>
> cheers,
> jamal

This one?


[-- Attachment #2: 01-iproute-memleak_optind-fix.patch --]
[-- Type: text/x-diff, Size: 891 bytes --]

diff -Naur iproute2-original/tc/m_ipt.c iproute2-patched/tc/m_ipt.c
--- iproute2-original/tc/m_ipt.c	2008-08-05 19:15:56.000000000 +0000
+++ iproute2-patched/tc/m_ipt.c	2008-08-05 21:39:49.000000000 +0000
@@ -277,6 +277,8 @@
 		if (!handle) {
 			fputs(dlerror(), stderr);
 			printf("\n");
+			free(l_name);
+			free(new_name);
 			return NULL;
 		}
 	}
@@ -292,12 +294,16 @@
 					fputs(error, stderr);
 					fprintf(stderr, "\n");
 					dlclose(handle);
+					free(l_name);
+					free(new_name);
 					return NULL;
 				}
 			}
 		}
 	}
 
+	free(l_name);
+	free(new_name);
 	return m;
 }
 
@@ -512,8 +518,15 @@
 	*argc_p = rargc - iargc;
 	*argv_p = argv;
 
-	optind = 1;
+	optind = 0;
 	free_opts(opts);
+	/* Clear flags if target will be used again */
+        m->tflags=0;
+        m->used=0;
+	/* Free allocated memory */
+        if (m->t)
+            free(m->t);
+
 
 	return 0;
 

Re: few more fixes for iproute2/m_ipt

[jamal]

On Wed, 2008-06-08 at 15:59 +0300, Denys Fedoryshchenko wrote:

> 
> This one?

Sure thing Denys. Again - appreciated. 
Do you know how to use git?
When you send such patches so that Stephen can apply them, it is nicer
if you have proper comments and signed offs. If you havent learnt how to
use it, could be a fun thing to try out.

I have tested the version of the patch i suggested earlier. I could not
reproduce the crash but it is very clear from printfs it was a bug.
If you dont mind please test it as well on your side and then send the
patch to Stephen with my ACK.

cheers,
jamal

Re: few more fixes for iproute2/m_ipt

[Stephen Hemminger]

On Wed, 6 Aug 2008 00:42:53 +0300
Denys Fedoryshchenko <denys@visp.net.lb> wrote:

> 1)optind according iptables sources have to be set to 0. If it is set to 1, in 
> batch it will mess up things. Also in iptables sources i notice that ->tflags 
> and ->used need to be reset.
> 
> 2)Since target->t = fw_calloc(1, size); allocated memory in function build_st, 
> it have to be freed at the end, or in batch we will have memory leak. TODO: 
> Probably it must be freed in all "return -1" cases in parse_ipt after 
> build_st. About this i am not sure, up to Stephen.
> 
> 3)lname and new_name was malloc'ed, but not freed

Applied the last patch for this.