Mailing List Archive: iptables: Devel

[PATCH] Re: Unclean complains about ECN

Index | Next | Previous | View Threaded

guillaume at morinfr

Aug 5, 2001, 10:30 AM

Post #1 of 7 (225 views)
Permalink

[PATCH] Re: Unclean complains about ECN

Hi Graham,

Dans un message du 05 aoû à 11:49, Graham Murray écrivait :
> When running with ECN enabled, unclean complains "TCP reserved bits
> not zero" on packets with the ECN and/or CWR bits set.

This check uses the TCP_RESERVED_BITS macro that is defined according to
RFC793 : 'Reserved: 6 bits Reserved for future use. Must be zero.'

ECN uses the last two bits of the reserved field.

Imho a packet with ECN bits should not be considered as unclean even if
ECN is not enabled on the firewall.

I see three solutions.

1) Removing this check. I've seen some network experts claiming that
reserved bits should never mean "equal to zero". But in this case,
RFC793 clearly states that these bits should be zero. Therefore, I don't
think it is the best solution.

2) Fixing the check by ignoring the ECN bits.

diff -uNr linux-2.4.7-vanilla/net/ipv4/netfilter/ipt_unclean.c linux-fixed-unclean/net/ipv4/netfilter/ipt_unclean.c
--- linux-2.4.7-vanilla/net/ipv4/netfilter/ipt_unclean.c Sun Aug 5 16:37:48 2001
+++ linux-fixed-unclean/net/ipv4/netfilter/ipt_unclean.c Sun Aug 5 17:12:03 2001
@@ -321,8 +321,8 @@
return 0;
}

- /* CHECK: TCP reserved bits zero. */
- if(tcp_flag_word(tcph) & TCP_RESERVED_BITS) {
+ /* CHECK: TCP reserved bits (except TCP ECN related bits) zero. */
+ if(tcp_flag_word(tcph) & TCP_RESERVED_BITS & ~(TCP_FLAG_CWR|TCP_FLAG_ECE)) {
limpk("TCP reserved bits not zero\n");
return 0;
}

3) Define TCP_RESERVED_BITS according to RFC ECN update. This macro is
used by netfilter (here and for the LOG target) and to define
TCP_HP_BITS (header prediction). Thus we need to modify TCP_HP_BITS
definition as well. Imho, the best solution.

diff -uNr linux-2.4.7-vanilla/include/linux/tcp.h linux-new-tcp-reserved-bits/include/linux/tcp.h
--- linux-2.4.7-vanilla/include/linux/tcp.h Sun Aug 5 16:37:43 2001
+++ linux-new-tcp-reserved-bits/include/linux/tcp.h Sun Aug 5 18:20:44 2001
@@ -110,7 +110,7 @@
TCP_FLAG_RST = __constant_htonl(0x00040000),
TCP_FLAG_SYN = __constant_htonl(0x00020000),
TCP_FLAG_FIN = __constant_htonl(0x00010000),
- TCP_RESERVED_BITS = __constant_htonl(0x0FC00000),
+ TCP_RESERVED_BITS = __constant_htonl(0x0F000000),
TCP_DATA_OFFSET = __constant_htonl(0xF0000000)
};

diff -uNr linux-2.4.7-vanilla/include/net/tcp_ecn.h linux-new-tcp-reserved-bits/include/net/tcp_ecn.h
--- linux-2.4.7-vanilla/include/net/tcp_ecn.h Sun Aug 5 16:37:44 2001
+++ linux-new-tcp-reserved-bits/include/net/tcp_ecn.h Sun Aug 5 18:22:36 2001
@@ -7,7 +7,7 @@

#include <net/inet_ecn.h>

-#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
+#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))

#define TCP_ECN_OK 1
#define TCP_ECN_QUEUE_CWR 2
@@ -137,7 +137,7 @@

#else

-#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))
+#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH) & ~(TCP_FLAG_ECE|TCP_FLAG_CWR))

#define TCP_ECN_send_syn(x...) do { } while (0)

Both patches have been tested. I currently run the second one (3rd
solution) on my own firewall flawlessly.

Any comments on this welcome.

Regards,

--
Guillaume Morin <guillaume [at] morinfr>

If it doesn't work, force it. If it breaks, it needed replacing anyway.

david at blue-labs

Aug 5, 2001, 1:41 PM

Post #2 of 7 (213 views)
Permalink

Re: [PATCH] Re: Unclean complains about ECN [In reply to]

ECN is no longer a proposed rfc, it's been accepted as an experimental
protocol in STD1. This field is no longer strictly reserved and may
contain a value other than zero.

David

Guillaume Morin wrote:

>Hi Graham,
>
>Dans un message du 05 aoû à 11:49, Graham Murray écrivait :
>
>>When running with ECN enabled, unclean complains "TCP reserved bits
>>not zero" on packets with the ECN and/or CWR bits set.
>>
>
>This check uses the TCP_RESERVED_BITS macro that is defined according to
>RFC793 : 'Reserved: 6 bits Reserved for future use. Must be zero.'
>

guillaume at morinfr

Aug 5, 2001, 11:49 PM

Post #3 of 7 (212 views)
Permalink

Re: [PATCH] Re: Unclean complains about ECN [In reply to]

Unfortunately, my testing host had ECN support disabled by proc. So I
missed a check that involved ECN in both patches. Here are updated
patches. They compile and are tested. The second one is in production on
my firewall.

Dans un message du 05 aoû à 19:30, Guillaume Morin écrivait :
> 2) Fixing the check by ignoring the ECN bits.

diff -uNr linux-2.4.7-vanilla/net/ipv4/netfilter/ipt_unclean.c linux-fixed-unclean/net/ipv4/netfilter/ipt_unclean.c
--- linux-2.4.7-vanilla/net/ipv4/netfilter/ipt_unclean.c Sun Aug 5 16:37:48 2001
+++ linux-fixed-unclean/net/ipv4/netfilter/ipt_unclean.c Mon Aug 6 08:31:29 2001
@@ -257,6 +257,8 @@
#define TH_PUSH 0x08
#define TH_ACK 0x10
#define TH_URG 0x20
+#define TH_ECE 0x40
+#define TH_CWR 0x80

/* TCP-specific checks. */
static int
@@ -321,14 +323,14 @@
return 0;
}

- /* CHECK: TCP reserved bits zero. */
- if(tcp_flag_word(tcph) & TCP_RESERVED_BITS) {
+ /* CHECK: TCP reserved bits (except TCP ECN related bit) zero. */
+ if(tcp_flag_word(tcph) & TCP_RESERVED_BITS & ~(TCP_FLAG_CWR|TCP_FLAG_ECE)) {
limpk("TCP reserved bits not zero\n");
return 0;
}

/* CHECK: TCP flags. */
- tcpflags = ((u_int8_t *)tcph)[13];
+ tcpflags = (((u_int8_t *)tcph)[13] & ~(TH_ECE|TH_CWR));
if (tcpflags != TH_SYN
&& tcpflags != (TH_SYN|TH_ACK)
&& tcpflags != (TH_RST|TH_ACK)

> 3) Define TCP_RESERVED_BITS according to RFC ECN update. This macro is
> used by netfilter (here and for the LOG target) and to define
> TCP_HP_BITS (header prediction). Thus we need to modify TCP_HP_BITS
> definition as well. Imho, the best solution.

diff -uNr linux-2.4.7-vanilla/include/linux/tcp.h linux-new-tcp-reserved-bits/include/linux/tcp.h
--- linux-2.4.7-vanilla/include/linux/tcp.h Sun Aug 5 16:37:43 2001
+++ linux-new-tcp-reserved-bits/include/linux/tcp.h Sun Aug 5 18:56:23 2001
@@ -110,7 +110,7 @@
TCP_FLAG_RST = __constant_htonl(0x00040000),
TCP_FLAG_SYN = __constant_htonl(0x00020000),
TCP_FLAG_FIN = __constant_htonl(0x00010000),
- TCP_RESERVED_BITS = __constant_htonl(0x0FC00000),
+ TCP_RESERVED_BITS = __constant_htonl(0x0F000000),
TCP_DATA_OFFSET = __constant_htonl(0xF0000000)
};

diff -uNr linux-2.4.7-vanilla/include/net/tcp_ecn.h linux-new-tcp-reserved-bits/include/net/tcp_ecn.h
--- linux-2.4.7-vanilla/include/net/tcp_ecn.h Sun Aug 5 16:37:44 2001
+++ linux-new-tcp-reserved-bits/include/net/tcp_ecn.h Sun Aug 5 18:58:08 2001
@@ -7,7 +7,7 @@

#include <net/inet_ecn.h>

-#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH)|TCP_FLAG_ECE|TCP_FLAG_CWR)
+#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))

#define TCP_ECN_OK 1
#define TCP_ECN_QUEUE_CWR 2
@@ -137,7 +137,7 @@

#else

-#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH))
+#define TCP_HP_BITS (~(TCP_RESERVED_BITS|TCP_FLAG_PSH) & ~(TCP_FLAG_ECE|TCP_FLAG_CWR))

#define TCP_ECN_send_syn(x...) do { } while (0)
diff -uNr linux-2.4.7-vanilla/net/ipv4/netfilter/ipt_unclean.c linux-new-tcp-reserved-bits/net/ipv4/netfilter/ipt_unclean.c
--- linux-2.4.7-vanilla/net/ipv4/netfilter/ipt_unclean.c Sun Aug 5 16:37:48 2001
+++ linux-new-tcp-reserved-bits/net/ipv4/netfilter/ipt_unclean.c Mon Aug 6 08:19:04 2001
@@ -257,6 +257,8 @@
#define TH_PUSH 0x08
#define TH_ACK 0x10
#define TH_URG 0x20
+#define TH_ECE 0x40
+#define TH_CWR 0x80

/* TCP-specific checks. */
static int
@@ -328,7 +330,7 @@
}

/* CHECK: TCP flags. */
- tcpflags = ((u_int8_t *)tcph)[13];
+ tcpflags = (((u_int8_t *)tcph)[13] & ~(TH_ECE|TH_CWR));
if (tcpflags != TH_SYN
&& tcpflags != (TH_SYN|TH_ACK)
&& tcpflags != (TH_RST|TH_ACK)

Any comments are welcome. Regards,

--
Guillaume Morin <guillaume [at] morinfr>

Unwisely, Santa offered a teddy bear to James, unaware that he had
been mauled by a grizzly earlier that year (T. Burton)

laforge at gnumonks

Aug 22, 2001, 5:08 PM

Post #4 of 7 (213 views)
Permalink

Re: [PATCH] Re: Unclean complains about ECN [In reply to]

On Sun, Aug 05, 2001 at 07:30:17PM +0200, Guillaume Morin wrote:
> Hi Graham,
>
> Dans un message du 05 aoû à 11:49, Graham Murray écrivait :
> > When running with ECN enabled, unclean complains "TCP reserved bits
> > not zero" on packets with the ECN and/or CWR bits set.
>
> This check uses the TCP_RESERVED_BITS macro that is defined according to
> RFC793 : 'Reserved: 6 bits Reserved for future use. Must be zero.'
>
> ECN uses the last two bits of the reserved field.
>
> Imho a packet with ECN bits should not be considered as unclean even if
> ECN is not enabled on the firewall.

Yes, you are correct.

For a full discussion of ECN, etc. see the variuos pages on the internet,
and even mailinglists like linux-kernel.

> I see three solutions.
>
> 1) Removing this check. I've seen some network experts claiming that
> reserved bits should never mean "equal to zero". But in this case,
> RFC793 clearly states that these bits should be zero. Therefore, I don't
> think it is the best solution.

The statement means: Must be zero on the sender. It does not mean, must
be zero in all cases.

It's just that any old host sending tcp packets doesn't send them with
one of the reserved bits set, which would cause them to be misinterpreted
by newer TCP stacks.

> 2) Fixing the check by ignoring the ECN bits.

yes. But fixing it different from your patch.

Why do you want to alter the TCP_RESERVED_BITS macro? The bits are reserved,
and this reserved bit notion is used at other places in the core network stack.

The problem is solely about the unclean match.

I will submit a fix to the unclean match containing the RST and the ECN fix.

> Guillaume Morin <guillaume [at] morinfr>

--
Live long and prosper
- Harald Welte / laforge [at] gnumonks 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+(*)

guillaume at morinfr

Aug 23, 2001, 11:09 AM

Post #5 of 7 (212 views)
Permalink

Re: [PATCH] Re: Unclean complains about ECN [In reply to]

Dans un message du 22 aoû à 21:08, Harald Welte écrivait :
> > 2) Fixing the check by ignoring the ECN bits.
>
> yes. But fixing it different from your patch.

In this message, there was two fixes (for solution 2 and 3). In the
later discussion, I've only kept the one from solution 3 which is the
best imho.

> Why do you want to alter the TCP_RESERVED_BITS macro? The bits are
> reserved, and this reserved bit notion is used at other places in the
> core network stack.

I think that the ECE and CWR bits should be not considered reserved
anymore. See in include/linux/tcp.h the tcphdr struct :
__u16 doff:4,
res1:4,
cwr:1,
ece:1,
urg:1,
ack:1,
psh:1,
rst:1,
syn:1,
fin:1;

Furthermore, my patch contains the fix for the _only_ other place where
this macro is used (for the header prediction part in tcp_ecn.h).
Therefore I think it makes sense.

> I will submit a fix to the unclean match containing the RST and the
> ECN fix.

This would be the patch with my second solution, but I do not think it
is the right way to fix it. But you're the boss :)

Regards,

--
Guillaume Morin <guillaume [at] morinfr>

Sauvez un arbre, mangez un castor

laforge at gnumonks

Aug 23, 2001, 2:24 PM

Post #6 of 7 (212 views)
Permalink

Re: [PATCH] Re: Unclean complains about ECN [In reply to]

On Thu, Aug 23, 2001 at 08:09:13PM +0200, Guillaume Morin wrote:

> > Why do you want to alter the TCP_RESERVED_BITS macro? The bits are
> > reserved, and this reserved bit notion is used at other places in the
> > core network stack.
>
> I think that the ECE and CWR bits should be not considered reserved
> anymore. See in include/linux/tcp.h the tcphdr struct :

Well, according to rfc793 they still are, and I guess they will be, until
there is a new RFC released as TCP standard document.

But that's nothing to be decided by us, there are other people to make
those decisions.

> Furthermore, my patch contains the fix for the _only_ other place where
> this macro is used (for the header prediction part in tcp_ecn.h).
> Therefore I think it makes sense.

actually you are right, I can see your point.

I will ask the net.gods on netdev [at] oss about what to do with this
issue.

> This would be the patch with my second solution, but I do not think it
> is the right way to fix it. But you're the boss :)

well, not as soon as we touch something beyond netfilter :)

> Regards,

--
Live long and prosper
- Harald Welte / laforge [at] gnumonks 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+(*)

guillaume at morinfr

Aug 23, 2001, 4:44 PM

Post #7 of 7 (211 views)
Permalink

Re: [PATCH] Re: Unclean complains about ECN [In reply to]

Dans un message du 23 aoû à 18:24, Harald Welte écrivait :
> > Furthermore, my patch contains the fix for the _only_ other place where
> > this macro is used (for the header prediction part in tcp_ecn.h).
> > Therefore I think it makes sense.
>
> actually you are right, I can see your point.
> I will ask the net.gods on netdev [at] oss about what to do with this
> issue.

Good idea, please keep me posted on that issue.

Regards,

--
Guillaume Morin <guillaume [at] morinfr>

Sauvez un arbre, mangez un castor

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads