iptables versioning feature causes ABI breakage

iptables versioning feature causes ABI breakage

[Jamal Hadi Salim]

Folks,

Could whoever that made the versioning changes in iptables please
contact me?
I am CCing netdev because netfilter-devel seems to bounce my emails.

I need to punish him/her/them by having them look at my
tc code to make sure that i didnt miss some trick they used 
(I just found out how the target name was being abused to carry
versions;->). In the future please please remember there are other
users of these goods other than iptables.

I am willing to say "works only with iptables >= 1.3.1"  and make
sure tc pukes on lower versions. It would be better if i could say
"ok, user space is 1.2.1 which doesnt have versioning support - heres
what i need to do"

Kernel/userspace mismatch otoh may be a problem even if i decice in user
space i am gonna stick with iptables that has the versioning feature.
How do i detect that a kernel which has iptables support also has
versioning feature?

cheers,
jamal

Re: iptables versioning feature causes ABI breakage

[Harald Welte]

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

On Thu, Mar 24, 2005 at 07:11:40AM -0500, Jamal Hadi Salim wrote:
> 
> Folks,
> 
> Could whoever that made the versioning changes in iptables please
> contact me?

That would be Rusty, and IIRC he's still in holidays.

> I am CCing netdev because netfilter-devel seems to bounce my emails.

I'll add you to a posting filter.  They're only administratively held
back for moderator approval, not really bounced.

> versions;->). In the future please please remember there are other
> users of these goods other than iptables.

well, the versioning was developed about the same time that you got your
tc interface included, if I'm not mistaken.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: iptables versioning feature causes ABI breakage

[jamal]

On Thu, 2005-03-24 at 07:33, Harald Welte wrote:

> > versions;->). In the future please please remember there are other
> > users of these goods other than iptables.
> 
> well, the versioning was developed about the same time that you got your
> tc interface included, if I'm not mistaken.

You are probably correct.
The tc code has been around since about 2.6.8 and the ipt code probably
went in around 2.6.9/10; there has been fixes made to net/sched/ipt.c
when general interfaces were changed i cant rememember what that was.. 

In any case, we can make it better going forward to reduce maintainance
overhead on my part. As i was saying earlier - I am stealing a lot of
the code from iptables - so if it is that shareable, maybe it could be
converted to a library.

cheers,
jamal

Re: iptables versioning feature causes ABI breakage

[Pablo Neira]

Jamal,

Jamal Hadi Salim wrote:
> Folks,
> 
> Could whoever that made the versioning changes in iptables please
> contact me?

I was also involved in that change together with Rusty around january. 
It's a new infrastructure to extend matches/targets without breaking 
backward compatibility with previous releases of target/matches/iptables.

I must confess that I didn't have a look at your ipt_action stuff before 
, now I did. As Patrick pointed out, we'll keep in mind your ipt thing 
in future.

Your patch for tc to fix the problem looks fine. BTW, you can now use 
extended MARK features like --and-mark and --or-mark ;->

--
Pablo

Re: iptables versioning feature causes ABI breakage

[Jamal Hadi Salim]

On Thu, 2005-03-24 at 16:17, Pablo Neira wrote:
> Jamal,
> 
> Jamal Hadi Salim wrote:
> > Folks,
> > 
> > Could whoever that made the versioning changes in iptables please
> > contact me?
> 
> I was also involved in that change together with Rusty around january. 
> It's a new infrastructure to extend matches/targets without breaking 
> backward compatibility with previous releases of target/matches/iptables.
> 

I think in the future this will go fine; all actions have a version
field as well that is set to 0 at the moment.

> I must confess that I didn't have a look at your ipt_action stuff before 
> , now I did. As Patrick pointed out, we'll keep in mind your ipt thing 
> in future.
> 
> Your patch for tc to fix the problem looks fine. BTW, you can now use 
> extended MARK features like --and-mark and --or-mark ;->
> 

cool.

Would you like to pay for your sins;-> by looking at my code to see it
doesnt break or miss things?

cheers,
jamal

Re: iptables versioning feature causes ABI breakage

[Pablo Neira]

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

Jamal Hadi Salim wrote:
> On Thu, 2005-03-24 at 16:17, Pablo Neira wrote:
>>I was also involved in that change together with Rusty around january. 
>>It's a new infrastructure to extend matches/targets without breaking 
>>backward compatibility with previous releases of target/matches/iptables.
> 
> I think in the future this will go fine; all actions have a version
> field as well that is set to 0 at the moment.

Hm, it's not set to 0, it's set to the highest revision of the target 
found in the kernel.

tc qdisc add dev eth0 ingress
tc filter add dev eth0 parent ffff: protocol ip prio 6 u32 match ip src 
10.0.2.24/32 flowid 1:16 action ipt -j MARK --and-mark 0x1

That example works fine and those options are only available in the 
brand new revision of MARK.

>>Your patch for tc to fix the problem looks fine. BTW, you can now use 
>>extended MARK features like --and-mark and --or-mark ;->
>>
> 
> cool.
> 
> Would you like to pay for your sins;-> by looking at my code to see it
> doesnt break or miss things?

I've found some problems, attached a patch to fix some things:

- a leak in option handling
- it doesn't check that the target received the correct parameters 
(m->final_check).

--
Pablo

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

===== tc/m_ipt.c 1.5 vs edited =====
--- 1.5/tc/m_ipt.c	2005-03-24 12:53:31 +01:00
+++ edited/tc/m_ipt.c	2005-03-31 02:05:42 +02:00
@@ -69,6 +69,7 @@
 };
 
 static struct iptables_target *t_list = NULL;
+static struct option *opts = original_opts;
 static unsigned int global_option_offset = 0;
 #define OPTION_OFFSET 256
 
@@ -169,18 +170,13 @@
 	return result;
 }
 
-static struct option *
-copy_options(struct option *oldopts)
+static void free_opts(struct option *opts)
 {
-	struct option *merge;
-	unsigned int num_old;
-	for (num_old = 0; oldopts[num_old].name; num_old++) ;
-	merge = malloc(sizeof (struct option) * (num_old + 1));
-	if (NULL == merge)
-		return NULL;
-	memcpy(merge, oldopts, num_old * sizeof (struct option));
-	memset(merge + num_old, 0, sizeof (struct option));
-	return merge;
+	if (opts != original_opts) {
+		free(opts);
+		opts = original_opts;
+		global_option_offset = 0;
+	}
 }
 
 static struct option *
@@ -385,7 +381,6 @@
 	int c;
 	int rargc = *argc_p;
 	char **argv = *argv_p;
-	struct option *opts;
 	int argc = 0, iargc = 0;
 	char k[16];
 	int res = -1;
@@ -409,11 +404,6 @@
 		return -1;
 	}
 
-	opts = copy_options(original_opts);
-
-	if (NULL == opts)
-		return -1;
-
 	while (1) {
 		c = getopt_long(argc, argv, "j:", opts, NULL);
 		if (c == -1)
@@ -440,23 +430,14 @@
 		default:
 			memset(&fw, 0, sizeof (fw));
 			if (m) {
-				unsigned int fake_flags = 0;
 				m->parse(c - m->option_offset, argv, 0,
-					 &fake_flags, NULL, &m->t);
+					 &m->tflags, NULL, &m->t);
 			} else {
 				fprintf(stderr," failed to find target %s\n\n", optarg);
 				return -1;
 
 			}
 			ok++;
-
-			/*m->final_check(m->t); -- Is this necessary?
-			** useful when theres depencies
-			** eg ipt_TCPMSS.c has have the TCP match loaded
-			** before this can be used;
-			**  also seems the ECN target needs it 
-			*/
-
 			break;
 
 		}
@@ -466,6 +447,7 @@
 		if (matches(argv[optind], "index") == 0) {
 			if (get_u32(&index, argv[optind + 1], 10)) {
 				fprintf(stderr, "Illegal \"index\"\n");
+				free_opts(opts);
 				return -1;
 			}
 			iok++;
@@ -479,6 +461,10 @@
 		return -1;
 	}
 
+	/* check that we passed the correct parameters to the target */
+	if (m)
+		m->final_check(m->tflags);
+
 	{
 		struct tcmsg *t = NLMSG_DATA(n);
 		if (t->tcm_parent != TC_H_ROOT
@@ -519,6 +505,7 @@
 	*argv_p = argv;
 	
 	optind = 1;
+	free_opts(opts);
 
 	return 0;
 
@@ -529,16 +516,10 @@
 {
 	struct rtattr *tb[TCA_IPT_MAX + 1];
 	struct ipt_entry_target *t = NULL;
-	struct option *opts;
 
 	if (arg == NULL)
 		return -1;
 
-	opts = copy_options(original_opts);
-
-	if (NULL == opts)
-		return -1;
-
 	parse_rtattr_nested(tb, TCA_IPT_MAX, arg);
 
 	if (tb[TCA_IPT_TABLE] == NULL) {
@@ -601,6 +582,7 @@
 		fprintf(f, " \n");
 
 	}
+	free_opts(opts);
 
 	return 0;
 }