[PATCH] Fix CONNMARK mask value demolition

[PATCH] Fix CONNMARK mask value demolition

[Peter Warasin]

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

This patch fixes the problem that the CONNMARK mask value
has been set to 0 whenever the CONNMARK target options has not
been the last options to be processed.

Signed-off-by: Peter Warasin <peter@endian.com>

---


[-- Attachment #2: fix_CONNMARK_mask_demolition.patch --]
[-- Type: text/x-patch, Size: 674 bytes --]

Index: iptables/extensions/libxt_CONNMARK.c
===================================================================
--- iptables.orig/extensions/libxt_CONNMARK.c	2008-01-06 03:08:18.000000000 +0100
+++ iptables/extensions/libxt_CONNMARK.c	2008-01-06 03:09:54.000000000 +0100
@@ -64,13 +64,12 @@
 	struct xt_connmark_target_info *markinfo
 		= (struct xt_connmark_target_info *)(*target)->data;
 
-	markinfo->mask = 0xffffffffUL;
-
 	switch (c) {
 		char *end;
 	case '1':
 		markinfo->mode = XT_CONNMARK_SET;
 
+		markinfo->mask = 0xffffffffUL;
 		markinfo->mark = strtoul(optarg, &end, 0);
 		if (*end == '/' && end[1] != '\0')
 		    markinfo->mask = strtoul(end+1, &end, 0);

Re: [PATCH] Fix CONNMARK mask value demolition

[Peter Warasin]

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

Hi

I would not like to create the impression trying to do any pressure on
this topic but i recognized that my patch description maybe was a little
bit poor.
In order to make sure the patch is understood correctly, here the bug
report, of which that patch is the fix:

If you use the CONNMARK target, it makes a difference if you have the -j
option as last option or before the matches.


iptables -t mangle -N test

This works:
iptables -v -t mangle -I test -m connmark --mark 7 -j CONNMARK
--set-mark 0x7/0xf
CONNMARK  all opt -- in * out *  0.0.0.0/0  -> 0.0.0.0/0  CONNMARK match
0x7 CONNMARK set 0x7/0xf

This not:
iptables -v -t mangle -I test -j CONNMARK --set-mark 0x7/0xf -m connmark
--mark 7
CONNMARK  all opt -- in * out *  0.0.0.0/0  -> 0.0.0.0/0  CONNMARK match
0x7 CONNMARK set 0x7

In the second call, the CONNMARK mask (0xf) will be eaten.

peter

-- 
:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: peter@endian.com

[-- Attachment #2: peter.vcf --]
[-- Type: text/x-vcard, Size: 279 bytes --]

begin:vcard
fn:Peter Warasin
n:;Peter Warasin
org:Endian GmbH/Srl
adr:;;Pillhof 47;Frangart/Frangarto;BZ;I-39010;Italien/Italia
email;internet:peter@endian.com
tel;work:+39 0471 631763
tel;fax:+39 0471 631764
x-mozilla-html:FALSE
url:http://www.endian.com
version:2.1
end:vcard

Re: [PATCH] Fix CONNMARK mask value demolition

[Patrick McHardy]

Peter Warasin wrote:
> This patch fixes the problem that the CONNMARK mask value
> has been set to 0 whenever the CONNMARK target options has not
> been the last options to be processed.

 > @@ -64,13 +64,12 @@
 >  	struct xt_connmark_target_info *markinfo
 >  		= (struct xt_connmark_target_info *)(*target)->data;
 >
 > -	markinfo->mask = 0xffffffffUL;
 > -
 >  	switch (c) {
 >  		char *end;
 >  	case '1':
 >  		markinfo->mode = XT_CONNMARK_SET;
 >
 > +		markinfo->mask = 0xffffffffUL;

Good catch, but don't we also need set the mask for --save-mark
and --restore-mark? I would suggest to move the initialzation
to a ->init() function.

Re: [PATCH v2] Fix CONNMARK mask value demolition

[Peter Warasin]

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

Hi Patrick

Patrick McHardy wrote:
> Good catch, but don't we also need set the mask for --save-mark
> and --restore-mark? I would suggest to move the initialzation
> to a ->init() function.

Oh, yes. That's necessary.
Here's the revised patch with the init() function.

peter

-- 
:: e n d i a n
:: open source - open minds

:: peter warasin
:: http://www.endian.com   :: peter@endian.com

[-- Attachment #2: fix_CONNMARK_mask_demolition.patch --]
[-- Type: text/x-patch, Size: 1791 bytes --]

Fix CONNMARK mask initialisation

This patch fixes the problem that the CONNMARK mask value
has been set to 0 whenever the CONNMARK target options were
not the last options to be processed.
It initalizes the mask value rather than setting it for
each parse.

Signed-off-by: Peter Warasin <peter@endian.com>

---
 extensions/libxt_CONNMARK.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Index: iptables/extensions/libxt_CONNMARK.c
===================================================================
--- iptables.orig/extensions/libxt_CONNMARK.c	2008-01-06 03:08:18.000000000 +0100
+++ iptables/extensions/libxt_CONNMARK.c	2008-01-15 12:40:54.000000000 +0100
@@ -64,8 +64,6 @@
 	struct xt_connmark_target_info *markinfo
 		= (struct xt_connmark_target_info *)(*target)->data;
 
-	markinfo->mask = 0xffffffffUL;
-
 	switch (c) {
 		char *end;
 	case '1':
@@ -188,6 +186,14 @@
 	}
 }
 
+static void CONNMARK_init(struct xt_entry_target *t)
+{
+	struct xt_connmark_target_info *markinfo
+		= (struct xt_connmark_target_info *)t->data;
+
+	markinfo->mask = 0xffffffffUL;
+}
+
 static struct xtables_target connmark_target = {
 	.family		= AF_INET,
 	.name		= "CONNMARK",
@@ -195,6 +201,7 @@
 	.size		= XT_ALIGN(sizeof(struct xt_connmark_target_info)),
 	.userspacesize	= XT_ALIGN(sizeof(struct xt_connmark_target_info)),
 	.help		= CONNMARK_help,
+	.init           = CONNMARK_init,
 	.parse		= CONNMARK_parse,
 	.final_check	= CONNMARK_check,
 	.print		= CONNMARK_print,
@@ -209,6 +216,7 @@
 	.size		= XT_ALIGN(sizeof(struct xt_connmark_target_info)),
 	.userspacesize	= XT_ALIGN(sizeof(struct xt_connmark_target_info)),
 	.help		= CONNMARK_help,
+	.init           = CONNMARK_init,
 	.parse		= CONNMARK_parse,
 	.final_check	= CONNMARK_check,
 	.print		= CONNMARK_print,

[-- Attachment #3: peter.vcf --]
[-- Type: text/x-vcard, Size: 279 bytes --]

begin:vcard
fn:Peter Warasin
n:;Peter Warasin
org:Endian GmbH/Srl
adr:;;Pillhof 47;Frangart/Frangarto;BZ;I-39010;Italien/Italia
email;internet:peter@endian.com
tel;work:+39 0471 631763
tel;fax:+39 0471 631764
x-mozilla-html:FALSE
url:http://www.endian.com
version:2.1
end:vcard

Re: [PATCH v2] Fix CONNMARK mask value demolition

[Patrick McHardy]

Peter Warasin wrote:
> Hi Patrick
> 
> Patrick McHardy wrote:
>> Good catch, but don't we also need set the mask for --save-mark
>> and --restore-mark? I would suggest to move the initialzation
>> to a ->init() function.
> 
> Oh, yes. That's necessary.
> Here's the revised patch with the init() function.

Applied, thanks Peter.