Fixed proken MARK target in POM.

Fixed proken MARK target in POM.

[Anders Fugmann]

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

Hi.

As I require the BIT-operations provided by the MARK target in POM,
attached is a fixed version of this.

The patch extends the MARK target in mangle chains to allow:

# iptables -t mangle -A PRERPUTING -j MARK --set-mark 0x01
# iptables -t mangle -A PRERPUTING -j MARK --and-mark 0x01
# iptables -t mangle -A PRERPUTING -j MARK --or-mark 0x01

The version is widly based on the current one with only minor code 
fixes. The kernel patch applies cleanly to 2.4.20 and 2.5.54. The 
userspace patch is against 1.2.7a. The patch has been tested on 2.5.54.

The patch maintains backward compability, and I request that this patch 
is applied to mainstream (and pushed to the Andrea for kernel 
inclusion), as I as a software developer of a firewall cannot ask users 
of the software to appliy patches to POM.

Regards
Anders Fugmann

[-- Attachment #2: MARK_TARGET.patch --]
[-- Type: text/plain, Size: 1559 bytes --]

diff -r -u linux-2.4.20/include/linux/netfilter_ipv4/ipt_MARK.h linux-2.4.20-new/include/linux/netfilter_ipv4/ipt_MARK.h
--- linux-2.4.20/include/linux/netfilter_ipv4/ipt_MARK.h	2000-03-17 19:56:20.000000000 +0100
+++ linux-2.4.20-new/include/linux/netfilter_ipv4/ipt_MARK.h	2003-01-09 11:15:06.000000000 +0100
@@ -1,8 +1,15 @@
 #ifndef _IPT_MARK_H_target
 #define _IPT_MARK_H_target
 
+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*/
diff -r -u linux-2.4.20/net/ipv4/netfilter/ipt_MARK.c linux-2.4.20-new/net/ipv4/netfilter/ipt_MARK.c
--- linux-2.4.20/net/ipv4/netfilter/ipt_MARK.c	2001-09-30 21:26:08.000000000 +0200
+++ linux-2.4.20-new/net/ipv4/netfilter/ipt_MARK.c	2003-01-09 11:22:53.000000000 +0100
@@ -16,11 +16,31 @@
        void *userinfo)
 {
 	const struct ipt_mark_target_info *markinfo = targinfo;
+	int newmark;
+	
+	switch (markinfo->mode) {
+	case IPT_MARK_SET:
+		newmark = markinfo->mark;
+		break;
+		
+	case IPT_MARK_AND:
+		newmark = (*pskb)->nfmark & markinfo->mark;
+		break;
+		
+	case IPT_MARK_OR:
+		newmark = (*pskb)->nfmark | markinfo->mark;
+		break;
+
+	default:
+                newmark = (*pskb)->nfmark;
+		break;
+	}
 
-	if((*pskb)->nfmark != markinfo->mark) {
-		(*pskb)->nfmark = markinfo->mark;
-		(*pskb)->nfcache |= NFC_ALTERED;
+	if ((*pskb)->nfmark != newmark) {
+	        (*pskb)->nfmark = newmark;
+	        (*pskb)->nfcache |= NFC_ALTERED;
 	}
+
 	return IPT_CONTINUE;
 }
 

[-- Attachment #3: MARK_operations.patch.userspace --]
[-- Type: text/plain, Size: 3546 bytes --]

diff -u -r iptables-1.2.7a/extensions/libipt_MARK.c iptables-1.2.7a-new/extensions/libipt_MARK.c
--- iptables-1.2.7a/extensions/libipt_MARK.c	2002-08-05 16:26:47.000000000 +0200
+++ iptables-1.2.7a-new/extensions/libipt_MARK.c	2003-01-09 13:29:47.000000000 +0100
@@ -20,12 +20,16 @@
 	printf(
 "MARK target v%s options:\n"
 "  --set-mark value                   Set nfmark value\n"
+"  --and-mark value                   Binary AND the nfmark with value\n"
+"  --or-mark  value                   Binary OR  the nfmark with value\n"
 "\n",
 IPTABLES_VERSION);
 }
 
 static struct option opts[] = {
 	{ "set-mark", 1, 0, '1' },
+	{ "and-mark", 1, 0, '2' },
+	{ "or-mark", 1, 0, '3' },
 	{ 0 }
 };
 
@@ -45,17 +49,29 @@
 	struct ipt_mark_target_info *markinfo
 		= (struct ipt_mark_target_info *)(*target)->data;
 
-	switch (c) {
-	case '1':
-		if (string_to_number(optarg, 0, 0xffffffff, 
+	/* Do generic stuff */
+	if (c=='1' || c=='2' || c=='3') {
+	        if (string_to_number(optarg, 0, 0xffffffff, 
 				     (unsigned int *)&markinfo->mark))
-			exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", optarg);
+		        exit_error(PARAMETER_PROBLEM, "Bad MARK value `%s'", optarg);
+
 		if (*flags)
-			exit_error(PARAMETER_PROBLEM,
-			           "MARK target: Can't specify --set-mark twice");
+		        exit_error(PARAMETER_PROBLEM,
+				   "MARK target: Can't specify --set-mark twice");
 		*flags = 1;
-		break;
 
+	}
+		
+	switch (c) {
+	case '1':
+	        markinfo->mode = IPT_MARK_SET;
+		break;
+	case '2':
+	        markinfo->mode = IPT_MARK_AND;
+		break;
+	case '3':
+	        markinfo->mode = IPT_MARK_OR;
+		break;
 	default:
 		return 0;
 	}
@@ -68,7 +84,7 @@
 {
 	if (!flags)
 		exit_error(PARAMETER_PROBLEM,
-		           "MARK target: Parameter --set-mark is required");
+		           "MARK target: Additional parameter required");
 }
 
 static void
@@ -85,7 +101,17 @@
 {
 	const struct ipt_mark_target_info *markinfo =
 		(const struct ipt_mark_target_info *)target->data;
-	printf("MARK set ");
+	switch (markinfo->mode) {
+	case IPT_MARK_SET:
+	        printf("MARK set ");
+	        break;
+	case IPT_MARK_AND:
+	        printf("MARK and ");
+	        break;
+	case IPT_MARK_OR: 
+	        printf("MARK or ");
+	        break;
+	}
 	print_mark(markinfo->mark, numeric);
 }
 
@@ -95,8 +121,19 @@
 {
 	const struct ipt_mark_target_info *markinfo =
 		(const struct ipt_mark_target_info *)target->data;
+	switch (markinfo->mode) {
+	case IPT_MARK_SET:
+	        printf("--set-mark");
+	        break;
+	case IPT_MARK_AND:
+	        printf("--and-mark");
+	        break;
+	case IPT_MARK_OR: 
+	        printf("--or-mark");
+	        break;
+	}
 
-	printf("--set-mark 0x%lx ", markinfo->mark);
+	printf(" 0x%lx ", markinfo->mark);
 }
 
 static
diff -u -r iptables-1.2.7a/iptables.8 iptables-1.2.7a-new/iptables.8
--- iptables-1.2.7a/iptables.8	2002-08-07 11:37:40.000000000 +0200
+++ iptables-1.2.7a-new/iptables.8	2003-01-09 12:25:13.000000000 +0100
@@ -690,6 +690,20 @@
 table.  It can for example be used in conjunction with iproute2.
 .TP
 .BI "--set-mark " "mark"
+Sets the marking of the packet to 
+.I mark
+regardless of the current mark value of the packet.
+.TP
+.BI "--and-mark " "mark"
+Binary AND the current mark value of the packet with the supplied
+.I mark 
+value.
+.TP
+.BI "--or-mark " "mark"
+Binary OR the current mark value of the packet with the supplied
+.I mark 
+value.
+
 .SS REJECT
 This is used to send back an error packet in response to the matched
 packet: otherwise it is equivalent to 

Re: Fixed proken MARK target in POM.

[Harald Welte]

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

On Thu, Jan 09, 2003 at 02:07:37PM +0100, Anders Fugmann wrote:

> The patch maintains backward compability, and I request that this patch 
> is applied to mainstream (and pushed to the Andrea for kernel 
> inclusion), 

1) Where is the compatibility?  I cannot see how this code change would
   ensure that
	a) old, unpatched kernel works with new, patched userspace
	b) new, patched kernel works with old, unpatched userspace
   Those two conditions need to be fulfilled, otherwise we cannot make a
   change during the stable kernel series.

2) Which Andrea are you talking about?  Therer is no Andrea involved in
   netfilter/iptabls updates.

> as I as a software developer of a firewall cannot ask users 
> of the software to appliy patches to POM.

I regard this as critic to my recent patch-o-matic announcement, where I
was asking people to use patch-o-matic.  I was talking about
bugfixes/updates which are in 2.4.20 (or 2.4.21-preX)... but people are
still running older kernels than that... and they should indeed use
patch-o-matic for the netfilter fixes.

Apart from that, everything in patch-o-matic are new features or
extensions, which are either not fully stable or we strongly doubt that
they are useful for the big public.  And if somebody wants to test one
of those non-standard extensions, we can ask people to use patch-o-matic.

> Regards
> Anders Fugmann
-- 
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
"If this were a dictatorship, it'd be a heck of a lot easier, just so long
 as I'm the dictator."  --  George W. Bush Dec 18, 2000

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

Re: Fixed proken MARK target in POM.

[Anders Fugmann]

Harald Welte wrote:
> On Thu, Jan 09, 2003 at 02:07:37PM +0100, Anders Fugmann wrote:
> 
> 
>>The patch maintains backward compability, and I request that this patch 
>>is applied to mainstream (and pushed to the Andrea for kernel 
>>inclusion), 
> 
> 
> 1) Where is the compatibility?  I cannot see how this code change would
>    ensure that
> 	a) old, unpatched kernel works with new, patched userspace
> 	b) new, patched kernel works with old, unpatched userspace
>    Those two conditions need to be fulfilled, otherwise we cannot make a
>    change during the stable kernel series.
I meant compability in form of userspace syntax, but I will make a new 
patch that sorts out the things above (if possible).

> 
> 2) Which Andrea are you talking about?  Therer is no Andrea involved in
>    netfilter/iptabls updates.
Andrea Arcangeli (2.4.x maintainer, just to recite what you already 
know), but as you wrote in the previous mail, all patches will go 
through netfilter-devel (And let the core team choose what to have 
included in the kernel).

> 
> 
>>as I as a software developer of a firewall cannot ask users 
>>of the software to appliy patches to POM.
> 
> 
> I regard this as critic to my recent patch-o-matic announcement, where I
> was asking people to use patch-o-matic.  I was talking about
Actually not, but thats ok.

> bugfixes/updates which are in 2.4.20 (or 2.4.21-preX)... but people are
> still running older kernels than that... and they should indeed use
> patch-o-matic for the netfilter fixes.
Agreed. Compability between kernel and userspace should be maintained no 
matter version, and patches exist in POM for users of older kernels.

> 
> Apart from that, everything in patch-o-matic are new features or
> extensions, which are either not fully stable or we strongly doubt that
> they are useful for the big public.  And if somebody wants to test one
> of those non-standard extensions, we can ask people to use patch-o-matic.
Yes. I'm aware of that. My question is "How can a patch be proven stable 
enough to be included, and is the functionality provided 'importent' 
enough for the general public?

Regards
Anders Fugmann

Re: Fixed proken MARK target in POM.

[Anders Fugmann]

Anders Fugmann wrote:
> Harald Welte wrote:
> 
>> On Thu, Jan 09, 2003 at 02:07:37PM +0100, Anders Fugmann wrote:
>>
>>
>>> The patch maintains backward compability, and I request that this 
>>> patch is applied to mainstream (and pushed to the Andrea for kernel 
>>> inclusion), 
>>
>>
>>
>> 1) Where is the compatibility?  I cannot see how this code change would
>>    ensure that
>>     a) old, unpatched kernel works with new, patched userspace
>>     b) new, patched kernel works with old, unpatched userspace
>>    Those two conditions need to be fulfilled, otherwise we cannot make a
>>    change during the stable kernel series.
Looking at bit further, this seems non-trivial as I guess that the 
userspace should compile no matter kernel-headers version - no?

Are defines tolerated? Something like:
#define MARK_BITOPS

and then in the userspace do a:
#ifdef MARK_BITOPS
Whenever the new fileds in the structure is used.

But IMHO this clutters the code up and I would rather avoid that.
Do you know of modules that had the same problem, or do you regard it as 
unresolvable to add an extra field to a structure in the stable series?

Regards
Anders Fugmann

Re: Fixed proken MARK target in POM.

[Harald Welte]

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

On Fri, Jan 10, 2003 at 05:38:42PM +0100, Anders Fugmann wrote:
> Looking at bit further, this seems non-trivial as I guess that the 
> userspace should compile no matter kernel-headers version - no?

yes, exactly. This would be the ideal case.

If this is not possible (or at least not feasible), we have a set of
kernel headers inside the iptables package itself.
(CVS:userspace/include/linux/...)

The current include path order makes 'real' kernel headers precede the
iptables-supplied headers.  If necessarry, we could change this order,
making the iptables-supplied headers precede.

> Are defines tolerated? Something like:
> #define MARK_BITOPS
> 
> and then in the userspace do a:
> #ifdef MARK_BITOPS
> Whenever the new fileds in the structure is used.

I don't understand how this could solve the problem.  #defines are about
compile time.  We need one runtime iptables that copes with old and new
kernels (and vice-versa).

> But IMHO this clutters the code up and I would rather avoid that.
> Do you know of modules that had the same problem, or do you regard it as 
> unresolvable to add an extra field to a structure in the stable series?

We have quite a couple of other cases where we have the same problem -
without any solution :((  But during a stable kernel series, we cannot
put compatibility aside.  People are not supposed to upgrade their
iptables after they upgraded their kernel...

> Regards
> Anders Fugmann

-- 
Live long and prosper
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
GCS/E/IT d- s-: a-- C+++ UL++++$ P+++ L++++$ E--- W- N++ o? K- w--- O- M- 
V-- PS+ PE-- Y+ PGP++ t++ 5-- !X !R tv-- b+++ DI? !D G+ e* h+ r% y+(*)

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

Re: Fixed proken MARK target in POM.

[Anders Fugmann]

Harald Welte wrote:
>>Are defines tolerated? Something like:
>>#define MARK_BITOPS
>>
>>and then in the userspace do a:
>>#ifdef MARK_BITOPS
>>Whenever the new fileds in the structure is used.
> 
> 
> I don't understand how this could solve the problem.  #defines are about
> compile time.  We need one runtime iptables that copes with old and new
> kernels (and vice-versa).
I can see a way to make it somewhat compatible.

Kernel:
   Make a totally new structure (Keep the old one for compability)
   When checkentry is called, examine the size of argument:
	If it equals the old size, set a module-global variable
	meaning that the module should run in compability mode.	
   Othervice use the new structure.

This should sort out kernel compability with whatever iptables version.

Userspace:
   This is a bit more tricky, as compability is defined by compile time. 
That is, if iptables is compiled against 2.4.21 with the new MARK 
target, it will not work with a kernel without the MARK target. If this 
is acceptable, then a #define MARK_BITOPS can help solve the userspace 
compability. Something like

#ifdef MARK_BITOPS
   Accept --or-mark and --and-mark, and use the new structure.
#else
   Only accept --set-mark and usethe old structure.
#endif

Is this acceptable (no backward compability with iptables - it only 
works with the kernel its compiled against and newer, but not older)?
	
> 
> We have quite a couple of other cases where we have the same problem -
> without any solution :((  But during a stable kernel series, we cannot
> put compatibility aside.  People are not supposed to upgrade their
> iptables after they upgraded their kernel...
If the above procedure is acceptable, then it migh be a template for 
such updates in the stable kernel series. It is important that the old 
structures are removed for 2.6 though.

Regards
Anders Fugmann

Re: Fixed proken MARK target in POM.

[Harald Welte]

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

On Fri, Jan 10, 2003 at 04:28:12PM +0100, Anders Fugmann wrote:

> >Apart from that, everything in patch-o-matic are new features or
> >extensions, which are either not fully stable or we strongly doubt that
> >they are useful for the big public.  And if somebody wants to test one
> >of those non-standard extensions, we can ask people to use patch-o-matic.
>
> Yes. I'm aware of that. My question is "How can a patch be proven stable 
> enough to be included, and is the functionality provided 'importent' 
> enough for the general public?

Well, It's hard to give a set of rules for this.  Mainly it reduces to
the case that I (or somebody else of the coreteam) has the personal
feeling that the code is OK and the functionality is important enough.

This sounds a bit like dictatorship, but our impression of what is
'important' is not a fixed one.  We judge by the practical cases we know
of, and by what users tell us.  So if there is a discussion about a
particular patch [or feature] on the list, and we see there is a big
demand by the users, we are likely to be inclined that this feature is
'needed'.  This has just happeed with the 'string' match, which I
personally dislike, but I am now convinced that there is a reasonable
demand for that particular function [and which will now be submitted to
the kernel anytime soon].

It's not different than in other subsystems of the kernel.  If you want
a feature to get into the core network stack, you need to write good
code and convince Alexey, DaveM and/or Andi Kleen that it is needed ;)

btw: having a bit-wise mark match and MARK target has always been one of
my favourites.  So if we can solve the compatibility issue, the patch
will certainly make it ;)

> Regards
> Anders Fugmann

-- 
Live long and prosper
- Harald Welte / laforge@gnumonks.org               http://www.gnumonks.org/
============================================================================
GCS/E/IT d- s-: a-- C+++ UL++++$ P+++ L++++$ E--- W- N++ o? K- w--- O- M- 
V-- PS+ PE-- Y+ PGP++ t++ 5-- !X !R tv-- b+++ DI? !D G+ e* h+ r% y+(*)

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