ipt_MARK extension with backwards compatibility (kernel side).

We've been chasing this for a while; thanks to Bart for the final piece!

How to extend an extension:
1) If you already have a flags word in your structure which is checked by existing versions of the code, you can simply add new flags: this will fail on old kernels and work on new kernels.

2) If not, you must extend the size of the structure, so old kernels will fail, and new kernels will be able to tell whether they are to use the new or old structure. The IPT_ALIGN'ed size of the structure must change for this to work!

3) In your userspace extension, use the *flags arg to note if you need the new structure. and in your final_check() function, reduce the targetsize or matchsize to the old structure size if you didn't use the new features.

4) Inside the kernel, create a dummy "struct ipt_target" or "struct ipt_match" for the backwards compatibility mode. This must have the same name as the real one, but must not be registered. In your checkentry() routine, set t->u.kernel.target or m->u.kernel.match to the old version if the old size is used.

5) See ipt_MARK for an example.

Name: Add bitops to ipt_MARK without breaking compatibility
Status: Tested under nfsim

Anders Fugmann <[email protected]> wrote a patch to add bitops to ipt_MARK. I made a version which doesn't rely on any infrastructure changes and is still backwards compatible (it'd be neater with interface changes, but <shrug>).

Bart De Schuymer <[email protected]> provided the idea of overriding the target type.

Index: linux-2.6.10-rc2-bk8-Netfilter/include/linux/netfilter_ipv4/ipt_MARK.h
===================================================================
@@ -1,8 +1,18 @@
 #ifndef _IPT_MARK_H_target
 #define _IPT_MARK_H_target
 
-struct ipt_mark_target_info {
+struct ipt_mark_target_old_info {
 	unsigned long mark;
 };
 
+enum {
+	IPT_MARK_SET=0,
+	IPT_MARK_AND,
+	IPT_MARK_OR
+};
+
+struct ipt_mark_target_info {
+	unsigned long mark;
+	u_int8_t mode;
+};
 #endif /*_IPT_MARK_H_target*/

A bad analogy is like a leaky screwdriver -- Richard Braakman

that is good news, we finally drop that heavy anchor :)

My idea, I don't know how crazy it is. Instead of using the size to guess the target/match version, we could steal 1 byte from char name[] to define a new field called version, so we could register different versions of a match/target.

Possible scenarios:
a) Old kernel, new iptables binary: since names are manipulated with str* functions, it shouldn't be any problem with the version stuff because it will be ignored since info after first '\0' is ignored.
b) New kernel, old iptables: version value is zero, so kernel guess that it must handle the thing with first version of the target/match.

Possible Inconvenients?:
a) Current target/match with a name 29 byte long. Hm I think that there's no target/match like that.
b) Could gcc mess things with alignments and break compatibility?
c) Something I'm missing 8)

Attached just the beginning of a possible patch, things like ipt_register_* version aware should be implemented.

--
Pablo

===== include/linux/netfilter_ipv4/ip_tables.h 1.12 vs edited =====

@@ -25,6 +25,7 @@
 #include <linux/compiler.h>
 #include <linux/netfilter_ipv4.h>
 
+#define IPT_TARGET_MATCH_MAXNAMELEN 29
 #define IPT_FUNCTION_MAXNAMELEN 30
 #define IPT_TABLE_MAXNAMELEN 32
 

@@ -53,7 +54,9 @@
 			u_int16_t match_size;
 
 			/* Used by userspace */
-			char name[IPT_FUNCTION_MAXNAMELEN];
+			char name[IPT_TARGET_MATCH_MAXNAMELEN];
+			/* Version */
+			u_int8_t version;
 		} user;
 		struct {
 			u_int16_t match_size;

@@ -76,7 +79,9 @@
 			u_int16_t target_size;
 
 			/* Used by userspace */
-			char name[IPT_FUNCTION_MAXNAMELEN];
+			char name[IPT_TARGET_MATCH_MAXNAMELEN];
+			/* Version */
+			u_int8_t version;
 		} user;
 		struct {
 			u_int16_t target_size;

@@ -344,7 +349,10 @@
 {
 	struct list_head list;
 
-	const char name[IPT_FUNCTION_MAXNAMELEN];
+	const char name[IPT_TARGET_MATCH_MAXNAMELEN];
+
+	/* Version */
+	u_int8_t version;
 
 	/* Return true or false: return FALSE and set *hotdrop = 1 to
            force immediate packet drop. */

@@ -378,7 +386,10 @@
 {
 	struct list_head list;
 
-	const char name[IPT_FUNCTION_MAXNAMELEN];
+	const char name[IPT_TARGET_MATCH_MAXNAMELEN];
+
+	/* Version */
+	u_int8_t version;
 
 	/* Called when user tries to insert an entry of this type:
            hook_mask is a bitmask of hooks from which it can be

===== include/linux/netfilter_ipv4/listhelp.h 1.4 vs edited =====

@@ -118,6 +118,24 @@
 	return 1;
 }
 
+/* Returns false if same name already in list, otherwise does insert. */
+static inline int
+list_named_version_insert(struct list_head *head, void *new)
+{
+	struct list_head *i;
+	u_int8_t ver, new_ver;
+
+	list_for_each(i, head) {
+		ver = i + sizeof(list_head) + NAMELEN_TARGET_MATCH_MAXLEN;
+		new_ver = i + sizeof(list_head) + NAMELEN_TARGET_MATCH_MAXLEN;
+		if (__list_cmp_name(i, new + sizeof(struct list_head))
+		    && ver == new_ver)
+			return 0;
+	}
+	list_prepend(head, new);
+	return 1;
+}
+
 /* Find this named element in the list. */
 #define list_named_find(head, name)			\
 LIST_FIND(head, __list_cmp_name, void *, name)

===== net/ipv4/netfilter/ip_tables.c 1.34 vs edited =====

@@ -1339,7 +1339,7 @@
 	if (ret != 0)
 		return ret;
 
-	if (!list_named_insert(&ipt_target, target)) {
+	if (!list_named_version_insert(&ipt_target, target)) {
 		duprintf("ipt_register_target: `%s' already in list!\n",
 			 target->name);
 		ret = -EINVAL;

@@ -1365,7 +1365,7 @@
 	if (ret != 0)
 		return ret;
 
-	if (!list_named_insert(&ipt_match, match)) {
+	if (!list_named_version_insert(&ipt_match, match)) {
 		duprintf("ipt_register_match: `%s' already in list!\n",
 			 match->name);
 		ret = -EINVAL;

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Pablo Neira wrote:

| My idea, I don't know how crazy it is. Instead of using the size to
| guess the target/match version, we could steal 1 byte from char name[]
| to define a new field called version, so we could register different
| versions of a match/target.

Once again, we need a field more...
Wasn't there an suggestion to reuse the 'nfcache' field too?? Wasn't the need to an unique rule id posted?
What more???

Why not break compatibilty, if it hamper the development?

Admit it, the design has some flaws, why not eliminating them? You can not preserve the compatibility forever! It you do it NOW (in the next 2.6 kernel release) you can rely on the circumstance, that it will updated, because in the current kernel cycle, it will be done more often. The longer you wait, the more difficult it will be!

To prevent further incompatibility, you can insert some reserved fields to the main structure...

Comments welcome :-)

Regards
~ Sven

- --
~ Sven Anders <[email protected]>

~ ANDURAS service solutions AG
~ Innstraße 71 - 94036 Passau - Germany
~ Web: www.anduras.de - Tel: +49 (0)851-4 90 50-0 - Fax: +49 (0)851-4 90 50-55

Rechtsform: Aktiengesellschaft - Sitz: Passau - Amtsgericht Passau HRB 6032 Mitglieder des Vorstands: Sven Anders, Marcus Junker, Michael Schön Vorsitzender des Aufsichtsrats: Dipl. Kfm. Karlheinz Antesberger -----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD4DBQFBqFrD5lKZ7Feg4EcRAohvAKCD53V3uhC/b2EBWQvjSlHzKQdXrQCYu7iy FnXEAMJ0SKVKwID2d3yUIw==
=2wlB
-----END PGP SIGNATURE-----

Rusty's solution allows backwards compatibility and adding new features. Your suggestion only allows adding new features. Furthermore, both solutions add the same kind of code complexity.
A unique rule id per table is easily constructed. As all the rules of the chains are located after each other, one can just use the number of previous rules, or the address offset from the first rule.
Ergo, backwards compatibility need not be broken.

cheers,
Bart

I don't see things that way, compatibility could be broken if all developers get their brains broken or become insane because they aren't able to workaround a problem. That didn't happen at the moment :)

it's not that easy, you can't always guess where they will come from. So we can't reserve a field in every structure.

I agree with Bart, backward compatibility is important.

--
Pablo

Yes, but in a completely different place, this time in a manner which can be done without too much disturbance, and at the same time building a good framework to allow targets & matches to evolve (which has not been possible up to now).

this because the core kernel does not allow us to add more skb fields than there are today. Not related to what is being discussed now. Modifying the SKB structure is simply not an option unless there is extremely good reasons why.

It was, and as a compromise we now have the COMMENT match.

We can within POM, but not that easily in the main kernel.

This is what is being done with the pkttables design. Just takes a bit longer to get there than desireable..

Regards
Henrik

I finally found some spare time to go back this issue, I've finished two patches for the kernel part of my idea of adding versions to targets/matches. One for ip_tables, and other for ipt_MARK, this based on Rusty's.

iptables (user space) patches is still missing :(

It works for me (TM) on a x86/gcc-2.95 with both patches applied and using an old binary version of iptables to test that doesn't break backward compatibility.

Please, say crap/cool/dirty/crazy/whatever about this.

--
Pablo

===== include/linux/netfilter_ipv4/ip_tables.h 1.12 vs edited =====

@@ -53,7 +53,9 @@
 			u_int16_t match_size;
 
 			/* Used by userspace */
-			char name[IPT_FUNCTION_MAXNAMELEN];
+			char name[IPT_FUNCTION_MAXNAMELEN-1];
+
+			u_int8_t version;
 		} user;
 		struct {
 			u_int16_t match_size;

@@ -76,7 +78,9 @@
 			u_int16_t target_size;
 
 			/* Used by userspace */
-			char name[IPT_FUNCTION_MAXNAMELEN];
+			char name[IPT_FUNCTION_MAXNAMELEN-1];
+
+			u_int8_t version;
 		} user;
 		struct {
 			u_int16_t target_size;

@@ -344,7 +348,9 @@
 {
 	struct list_head list;
 
-	const char name[IPT_FUNCTION_MAXNAMELEN];
+	const char name[IPT_FUNCTION_MAXNAMELEN-1];
+
+	u_int8_t version;
 
 	/* Return true or false: return FALSE and set *hotdrop = 1 to
            force immediate packet drop. */

@@ -378,7 +384,9 @@
 {
 	struct list_head list;
 
-	const char name[IPT_FUNCTION_MAXNAMELEN];
+	const char name[IPT_FUNCTION_MAXNAMELEN-1];
+
+	u_int8_t version;
 
 	/* Called when user tries to insert an entry of this type:
            hook_mask is a bitmask of hooks from which it can be

@@ -414,7 +422,7 @@
 extern void ipt_unregister_match(struct ipt_match *match);
 
 extern struct ipt_target *
-__ipt_find_target_lock(const char *name, int *error);
+__ipt_find_target_lock(const char *name, const u_int8_t ver, int *error);
 extern void __ipt_mutex_up(void);
 
 /* Furniture shopping... */

===== net/ipv4/netfilter/ip_tables.c 1.34 vs edited =====

===== include/linux/netfilter_ipv4/ipt_MARK.h 1.1 vs edited =====

@@ -1,8 +1,19 @@
 #ifndef _IPT_MARK_H_target
 #define _IPT_MARK_H_target
 
-struct ipt_mark_target_info {
+struct ipt_mark_target_info_v0 {
 	unsigned long mark;
+};
+
+enum {
+	IPT_MARK_SET=0,
+	IPT_MARK_AND,
+	IPT_MARK_OR
+};
+
+struct ipt_mark_target_info_v1 {
+	unsigned long mark;
+	u_int8_t mode;
 };
 
 #endif /*_IPT_MARK_H_target*/

===== net/ipv4/netfilter/ipt_MARK.c 1.7 vs edited =====

This is so evil that my first reaction was to say "ick!". My second reaction was to say "cool"!. My third reaction was "ick!" again.

OK. For the other variant, I wrote a userspace patch, built it, and used nfsim to test it. Unfortunately, SVN is having trouble at the moment, so the test script I used is below (use env var NFSIM_IPTABLES_PREFIX to override /sbin for iptables)

If you ensure that works, I'm relatively happy with this. Rusty.

# Test the ipt_MARK target, which has a backwards compatbility mode.

# Test old-style first, insert and delete. iptables -t mangle -A PREROUTING -j MARK --set-mark 7 iptables -t mangle -D PREROUTING -j MARK --set-mark 7

iptables -t mangle -A PREROUTING -s 192.160.0.2 -j MARK --set-mark 7 iptables -t mangle -D PREROUTING -s 192.160.0.2 -j MARK --set-mark 7

# Test that it works.
iptables -t mangle -A PREROUTING -j MARK --set-mark 7
expect gen_ip send:eth1 MARK 7 {IPv4 192.168.0.2 192.168.1.2 10 17 1 2} gen_ip IF=eth0 192.168.0.2 192.168.1.2 10 17 1 2
iptables -t mangle -D PREROUTING -j MARK --set-mark 7

# Test new-style, insert and delete. iptables -t mangle -A PREROUTING -j MARK --or-mark 7 iptables -t mangle -D PREROUTING -j MARK --or-mark 7

iptables -t mangle -A PREROUTING -j MARK --and-mark 7 iptables -t mangle -D PREROUTING -j MARK --and-mark 7

iptables -t mangle -A PREROUTING -s 192.160.0.2 -j MARK --or-mark 7 iptables -t mangle -D PREROUTING -s 192.160.0.2 -j MARK --or-mark 7

iptables -t mangle -A PREROUTING -s 192.160.0.2 -j MARK --and-mark 7 iptables -t mangle -D PREROUTING -s 192.160.0.2 -j MARK --and-mark 7

# Test that they work.
iptables -t mangle -A PREROUTING -j MARK --or-mark 7
expect gen_ip send:eth1 MARK 7 {IPv4 192.168.0.2 192.168.1.2 10 17 1 2} gen_ip IF=eth0 192.168.0.2 192.168.1.2 10 17 1 2
iptables -t mangle -D PREROUTING -j MARK --or-mark 7

iptables -t mangle -A PREROUTING -j MARK --set-mark 3 iptables -t mangle -A PREROUTING -j MARK --or-mark 4
expect gen_ip send:eth1 MARK 7 {IPv4 192.168.0.2 192.168.1.2 10 17 1 2} gen_ip IF=eth0 192.168.0.2 192.168.1.2 10 17 1 2
iptables -t mangle -D PREROUTING -j MARK --set-mark 3 iptables -t mangle -D PREROUTING -j MARK --or-mark 4

iptables -t mangle -A PREROUTING -j MARK --set-mark 3 iptables -t mangle -A PREROUTING -j MARK --and-mark 6
expect gen_ip send:eth1 MARK 2 {IPv4 192.168.0.2 192.168.1.2 10 17 1 2} gen_ip IF=eth0 192.168.0.2 192.168.1.2 10 17 1 2
iptables -t mangle -D PREROUTING -j MARK --set-mark 3 iptables -t mangle -D PREROUTING -j MARK --and-mark 6

# Now mix them up: check we delete the right one. iptables -t mangle -A PREROUTING -j MARK --set-mark 7 iptables -t mangle -A PREROUTING -j MARK --or-mark 7 iptables -t mangle -A PREROUTING -j MARK --and-mark 7

# Delete old-style.
iptables -t mangle -D PREROUTING -j MARK --set-mark 7 expect iptables iptables: command failed iptables -t mangle -D PREROUTING -j MARK --set-mark 7 iptables -t mangle -A PREROUTING -j MARK --set-mark 7

# Delete or.
iptables -t mangle -D PREROUTING -j MARK --or-mark 7 expect iptables iptables: command failed iptables -t mangle -D PREROUTING -j MARK --or-mark 7 iptables -t mangle -A PREROUTING -j MARK --or-mark 7

# Delete and.
iptables -t mangle -D PREROUTING -j MARK --and-mark 7 expect iptables iptables: command failed iptables -t mangle -D PREROUTING -j MARK --and-mark 7 iptables -t mangle -A PREROUTING -j MARK --and-mark 7

A bad analogy is like a leaky screwdriver -- Richard Braakman