CTA_PROTO_NUM u_int8_t or u_int16_t

CTA_PROTO_NUM u_int8_t or u_int16_t

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 588 bytes --]

Hello,

AFAIK ip proto is u8 but I found that it is represented as u16 and used 
both as u_int16_t and u_int8_t:

include/linux/netfilter_ipv4/ipt_conntrack.h:           u16 protonum;

net/ipv4/netfilter/ip_conntrack_netlink.c:      NFA_PUT(skb, CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
net/ipv4/netfilter/ip_conntrack_netlink.c:      [CTA_PROTO_NUM-1]       = sizeof(u_int16_t),
net/ipv4/netfilter/ip_conntrack_netlink.c:      tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);

Was this intentionally?

Best regards,

 				Krzysztof Olędzki

Re: CTA_PROTO_NUM u_int8_t or u_int16_t

[Pablo Neira]

Krzysztof Oledzki wrote:
> AFAIK ip proto is u8 but I found that it is represented as u16 and used
> both as u_int16_t and u_int8_t:
> 
> include/linux/netfilter_ipv4/ipt_conntrack.h:           u16 protonum;
> 
> net/ipv4/netfilter/ip_conntrack_netlink.c:      NFA_PUT(skb,
> CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
> net/ipv4/netfilter/ip_conntrack_netlink.c:      [CTA_PROTO_NUM-1]      
> = sizeof(u_int16_t),
> net/ipv4/netfilter/ip_conntrack_netlink.c:      tuple->dst.protonum =
> *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> 
> Was this intentionally?

No :(, it has slipped through at some stage. I was aware of that, I
found it some days ago. The problem is that if we fix that we could
break backward compatibility for 2.6.14, so I'm still thinking about how
to fix it.

-- 
Pablo

Re: CTA_PROTO_NUM u_int8_t or u_int16_t

[Patrick McHardy]

Pablo Neira wrote:
> Krzysztof Oledzki wrote:
> 
>>AFAIK ip proto is u8 but I found that it is represented as u16 and used
>>both as u_int16_t and u_int8_t:
>>
>>include/linux/netfilter_ipv4/ipt_conntrack.h:           u16 protonum;
>>
>>net/ipv4/netfilter/ip_conntrack_netlink.c:      NFA_PUT(skb,
>>CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
>>net/ipv4/netfilter/ip_conntrack_netlink.c:      [CTA_PROTO_NUM-1]      
>>= sizeof(u_int16_t),
>>net/ipv4/netfilter/ip_conntrack_netlink.c:      tuple->dst.protonum =
>>*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
>>
>>Was this intentionally?
> 
> No :(, it has slipped through at some stage. I was aware of that, I
> found it some days ago. The problem is that if we fix that we could
> break backward compatibility for 2.6.14, so I'm still thinking about how
> to fix it.

If you have to break compatibility, please do it before 2.6.15 is
released. But the easiest solution looks like keeping it a u_int16_t
and adjusting the NFA_PUT.

Re: CTA_PROTO_NUM u_int8_t or u_int16_t

[Pablo Neira]

Patrick McHardy wrote:
> Pablo Neira wrote:
> 
>> Krzysztof Oledzki wrote:
>>
>>> AFAIK ip proto is u8 but I found that it is represented as u16 and used
>>> both as u_int16_t and u_int8_t:
>>>
>>> include/linux/netfilter_ipv4/ipt_conntrack.h:           u16 protonum;
>>>
>>> net/ipv4/netfilter/ip_conntrack_netlink.c:      NFA_PUT(skb,
>>> CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
>>> net/ipv4/netfilter/ip_conntrack_netlink.c:     
>>> [CTA_PROTO_NUM-1]      = sizeof(u_int16_t),
>>> net/ipv4/netfilter/ip_conntrack_netlink.c:      tuple->dst.protonum =
>>> *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
>>>
>>> Was this intentionally?
>>
>>
>> No :(, it has slipped through at some stage. I was aware of that, I
>> found it some days ago. The problem is that if we fix that we could
>> break backward compatibility for 2.6.14, so I'm still thinking about how
>> to fix it.
> 
> If you have to break compatibility, please do it before 2.6.15 is
> released. But the easiest solution looks like keeping it a u_int16_t
> and adjusting the NFA_PUT.

I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
will go sooner or later.

-- 
Pablo

Re: CTA_PROTO_NUM u_int8_t or u_int16_t

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1404 bytes --]



On Mon, 21 Nov 2005, Pablo Neira wrote:

> Patrick McHardy wrote:
>> Pablo Neira wrote:
>>
>>> Krzysztof Oledzki wrote:
>>>
>>>> AFAIK ip proto is u8 but I found that it is represented as u16 and used
>>>> both as u_int16_t and u_int8_t:
>>>>
>>>> include/linux/netfilter_ipv4/ipt_conntrack.h:           u16 protonum;
>>>>
>>>> net/ipv4/netfilter/ip_conntrack_netlink.c:      NFA_PUT(skb,
>>>> CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
>>>> net/ipv4/netfilter/ip_conntrack_netlink.c:
>>>> [CTA_PROTO_NUM-1]      = sizeof(u_int16_t),
>>>> net/ipv4/netfilter/ip_conntrack_netlink.c:      tuple->dst.protonum =
>>>> *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
>>>>
>>>> Was this intentionally?
>>>
>>>
>>> No :(, it has slipped through at some stage. I was aware of that, I
>>> found it some days ago. The problem is that if we fix that we could
>>> break backward compatibility for 2.6.14, so I'm still thinking about how
>>> to fix it.
>>
>> If you have to break compatibility, please do it before 2.6.15 is
>> released. But the easiest solution looks like keeping it a u_int16_t
>> and adjusting the NFA_PUT.
>
> I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
> will go sooner or later.

Fair enough. But what if we find similar problem later, this time in 
nf_conntrack_netlink? ;)

Best regards,

 				Krzysztof Olędzki

Re: CTA_PROTO_NUM u_int8_t or u_int16_t

[Patrick McHardy]

Pablo Neira wrote:
> Patrick McHardy wrote:
> 
>>If you have to break compatibility, please do it before 2.6.15 is
>>released. But the easiest solution looks like keeping it a u_int16_t
>>and adjusting the NFA_PUT.
> 
> I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
> will go sooner or later.

I'm not sure that will be an avantage to breaking it immediately.
People should be able to use ctnetlink application with nfctnetlink,
so it will still break, but at that point the interface will already
be more established. We could just leave it a u_int16_t, but it won't
be in network byte order as the other attributes, which IMO needs to
be fixed if we ever want to use ctnetlink over the network without
adding lots of code in userspace just to fix up this single field.

[PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Pablo Neira]

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

Patrick McHardy wrote:
> Pablo Neira wrote:
> 
>> Patrick McHardy wrote:
>>
>>> If you have to break compatibility, please do it before 2.6.15 is
>>> released. But the easiest solution looks like keeping it a u_int16_t
>>> and adjusting the NFA_PUT.
>>
>> I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
>> will go sooner or later.
> 
> I'm not sure that will be an avantage to breaking it immediately.
> People should be able to use ctnetlink application with nfctnetlink,
> so it will still break, but at that point the interface will already
> be more established. We could just leave it a u_int16_t, but it won't
> be in network byte order as the other attributes, which IMO needs to
> be fixed if we ever want to use ctnetlink over the network without
> adding lots of code in userspace just to fix up this single field.

Better to fix it, break backward compatibility now and forget about this
issue. Patch attached. Thanks for the feedback Patrick.

-- 
Pablo

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

CTA_PROTO_NUM is u_int8_t, not u_int16_t

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Index: netfilter-2.6.14.git/net/ipv4/netfilter/ip_conntrack_netlink.c
===================================================================
--- netfilter-2.6.14.git.orig/net/ipv4/netfilter/ip_conntrack_netlink.c	2005-11-22 02:46:00.000000000 +0100
+++ netfilter-2.6.14.git/net/ipv4/netfilter/ip_conntrack_netlink.c	2005-11-22 19:57:16.000000000 +0100
@@ -502,7 +502,7 @@ ctnetlink_parse_tuple_ip(struct nfattr *
 }
 
 static const size_t cta_min_proto[CTA_PROTO_MAX] = {
-	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
+	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
@@ -527,7 +527,7 @@ ctnetlink_parse_tuple_proto(struct nfatt
 
 	if (!tb[CTA_PROTO_NUM-1])
 		return -EINVAL;
-	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+	tuple->dst.protonum = *(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
 
 	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
 

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1952 bytes --]



On Tue, 22 Nov 2005, Pablo Neira wrote:

> Patrick McHardy wrote:
>> Pablo Neira wrote:
>>
>>> Patrick McHardy wrote:
>>>
>>>> If you have to break compatibility, please do it before 2.6.15 is
>>>> released. But the easiest solution looks like keeping it a u_int16_t
>>>> and adjusting the NFA_PUT.
>>>
>>> I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
>>> will go sooner or later.
>>
>> I'm not sure that will be an avantage to breaking it immediately.
>> People should be able to use ctnetlink application with nfctnetlink,
>> so it will still break, but at that point the interface will already
>> be more established. We could just leave it a u_int16_t, but it won't
>> be in network byte order as the other attributes, which IMO needs to
>> be fixed if we ever want to use ctnetlink over the network without
>> adding lots of code in userspace just to fix up this single field.
>
> Better to fix it, break backward compatibility now and forget about this
> issue.

Isn't it possible to add kernel version checking into 
libnetfilter_conntrack and send CTA_PROTO_NUM as u_int16_t under 2.6.14 
and as u_int8_t under 2.6.15+? The protonum is alredy defined as u_int8_t 
in struct nfct_tuple (libnetfilter_conntrack.h).

> Patch attached. Thanks for the feedback Patrick.

What about ip_conntrack_old_tuple struct from 
include/linux/netfilter_ipv4/ipt_conntrack.h:

/* This is exposed to userspace, so remains frozen in time. */
struct ip_conntrack_old_tuple
{
         struct {
                 __u32 ip;
                 union {
                         __u16 all;
                 } u;
         } src;

         struct {
                 __u32 ip;
                 union {
                         __u16 all;
                 } u;

                 /* The protocol. */
                 u16 protonum;
         } dst;
};


Best regards,

 			Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Harald Welte]

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

On Tue, Nov 22, 2005 at 09:29:51PM +0100, Krzysztof Oledzki wrote:
> On Tue, 22 Nov 2005, Pablo Neira wrote:
> 
> >Patrick McHardy wrote:
> >>Pablo Neira wrote:
> >>>Patrick McHardy wrote:
> >>>>If you have to break compatibility, please do it before 2.6.15 is
> >>>>released. But the easiest solution looks like keeping it a u_int16_t
> >>>>and adjusting the NFA_PUT.
> >>>I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
> >>>will go sooner or later.
> >>I'm not sure that will be an avantage to breaking it immediately.
> >>People should be able to use ctnetlink application with nfctnetlink,
> >>so it will still break, but at that point the interface will already
> >>be more established. We could just leave it a u_int16_t, but it won't
> >>be in network byte order as the other attributes, which IMO needs to
> >>be fixed if we ever want to use ctnetlink over the network without
> >>adding lots of code in userspace just to fix up this single field.
> >Better to fix it, break backward compatibility now and forget about this
> >issue.
> 
> Isn't it possible to add kernel version checking into libnetfilter_conntrack and send 
> CTA_PROTO_NUM as u_int16_t under 2.6.14 and as u_int8_t under 2.6.15+? The protonum is 
> alredy defined as u_int8_t in struct nfct_tuple (libnetfilter_conntrack.h).

It can be fixed without any compatibility issues, at least in one
direction.

If the new kernel accepts both a 8 bit and 16bit value, then new
userspace programs (who send 8bit) and old userspace programs would run
on new kernels.  only for old kernels you need the old app.

another alternative was to introduce a new CTA_PROTO_NUM8 value, which
is more explicit (but somehow stupid).

-- 
- 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: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Harald Welte wrote:
> It can be fixed without any compatibility issues, at least in one
> direction.
> 
> If the new kernel accepts both a 8 bit and 16bit value, then new
> userspace programs (who send 8bit) and old userspace programs would run
> on new kernels.  only for old kernels you need the old app.
 >
> another alternative was to introduce a new CTA_PROTO_NUM8 value, which
> is more explicit (but somehow stupid).

Lets do that and call it CTA_PROTO, I don't like the _NUM anyway :)

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Pablo Neira]

Harald Welte wrote:
> On Tue, Nov 22, 2005 at 09:29:51PM +0100, Krzysztof Oledzki wrote:
> 
>>On Tue, 22 Nov 2005, Pablo Neira wrote:
>>
>>
>>>Patrick McHardy wrote:
>>>
>>>>Pablo Neira wrote:
>>>>
>>>>>Patrick McHardy wrote:
>>>>>
>>>>>>If you have to break compatibility, please do it before 2.6.15 is
>>>>>>released. But the easiest solution looks like keeping it a u_int16_t
>>>>>>and adjusting the NFA_PUT.
>>>>>
>>>>>I think that I can fix it in nf_conntrack_netlink. ip_conntrack_netlink
>>>>>will go sooner or later.
>>>>
>>>>I'm not sure that will be an avantage to breaking it immediately.
>>>>People should be able to use ctnetlink application with nfctnetlink,
>>>>so it will still break, but at that point the interface will already
>>>>be more established. We could just leave it a u_int16_t, but it won't
>>>>be in network byte order as the other attributes, which IMO needs to
>>>>be fixed if we ever want to use ctnetlink over the network without
>>>>adding lots of code in userspace just to fix up this single field.
>>>
>>>Better to fix it, break backward compatibility now and forget about this
>>>issue.
>>
>>Isn't it possible to add kernel version checking into libnetfilter_conntrack and send 
>>CTA_PROTO_NUM as u_int16_t under 2.6.14 and as u_int8_t under 2.6.15+? The protonum is 
>>alredy defined as u_int8_t in struct nfct_tuple (libnetfilter_conntrack.h).
> 
> 
> It can be fixed without any compatibility issues, at least in one
> direction.
> 
> If the new kernel accepts both a 8 bit and 16bit value, then new
> userspace programs (who send 8bit) and old userspace programs would run
> on new kernels.  only for old kernels you need the old app.
> 
> another alternative was to introduce a new CTA_PROTO_NUM8 value, which
> is more explicit (but somehow stupid).

Why don't we send a patch to -stable? I think that most people will use
lastest stable branch in 2.6.14, so only < 2.6.14.3 would be broken. I
still don't like too much the idea of adding a new field just because of
this bugfix :(

-- 
Pablo

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Pablo Neira wrote:
> Harald Welte wrote:
> 
>>another alternative was to introduce a new CTA_PROTO_NUM8 value, which
>>is more explicit (but somehow stupid).
> 
> 
> Why don't we send a patch to -stable? I think that most people will use
> lastest stable branch in 2.6.14, so only < 2.6.14.3 would be broken. I
> still don't like too much the idea of adding a new field just because of
> this bugfix :(

I would be fine with this.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 643 bytes --]



On Wed, 23 Nov 2005, Patrick McHardy wrote:

> Pablo Neira wrote:
>> Harald Welte wrote:
>>
>>> another alternative was to introduce a new CTA_PROTO_NUM8 value, which
>>> is more explicit (but somehow stupid).
>>
>>
>> Why don't we send a patch to -stable? I think that most people will use
>> lastest stable branch in 2.6.14, so only < 2.6.14.3 would be broken. I
>> still don't like too much the idea of adding a new field just because of
>> this bugfix :(
>
> I would be fine with this.

Can we make it before 2.6.14.3? I don't know how long we are going to wait 
for 2.6.14.4.

Best regards,

 			Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Harald Welte]

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

On Wed, Nov 23, 2005 at 11:31:18AM +0100, Krzysztof Oledzki wrote:
> 
> 
> On Wed, 23 Nov 2005, Patrick McHardy wrote:
> 
> >Pablo Neira wrote:
> >>Harald Welte wrote:
> >>>another alternative was to introduce a new CTA_PROTO_NUM8 value, which
> >>>is more explicit (but somehow stupid).
> >>Why don't we send a patch to -stable? I think that most people will use
> >>lastest stable branch in 2.6.14, so only < 2.6.14.3 would be broken. I
> >>still don't like too much the idea of adding a new field just because of
> >>this bugfix :(
> >I would be fine with this.
> 
> Can we make it before 2.6.14.3? I don't know how long we are going to wait for 2.6.14.4.

what about the following fix?  If no objections arise, I'll submit it
tomorrow.

diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -58,12 +58,13 @@ enum ctattr_ip {
 
 enum ctattr_l4proto {
 	CTA_PROTO_UNSPEC,
-	CTA_PROTO_NUM,
+	CTA_PROTO_NUM,		/* old 16bit CTA_PROTO, pre-2.6.15 */
 	CTA_PROTO_SRC_PORT,
 	CTA_PROTO_DST_PORT,
 	CTA_PROTO_ICMP_ID,
 	CTA_PROTO_ICMP_TYPE,
 	CTA_PROTO_ICMP_CODE,
+	CTA_PROTO,		/* new CTA_PROTO value, 8bit width */
 	__CTA_PROTO_MAX
 };
 #define CTA_PROTO_MAX (__CTA_PROTO_MAX - 1)
diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
--- a/net/ipv4/netfilter/ip_conntrack_netlink.c
+++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
@@ -57,7 +57,7 @@ ctnetlink_dump_tuples_proto(struct sk_bu
 	struct ip_conntrack_protocol *proto;
 	int ret = 0;
 
-	NFA_PUT(skb, CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
+	NFA_PUT(skb, CTA_PROTO, sizeof(u_int8_t), &tuple->dst.protonum);
 
 	/* If no protocol helper is found, this function will return the
 	 * generic protocol helper, so proto won't *ever* be NULL */
@@ -508,6 +508,7 @@ static const size_t cta_min_proto[CTA_PR
 	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_ICMP_CODE-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_ICMP_ID-1]	= sizeof(u_int16_t),
+	[CTA_PROTO-1]		= sizeof(u_int8_t),
 };
 
 static inline int
@@ -525,9 +526,14 @@ ctnetlink_parse_tuple_proto(struct nfatt
 	if (nfattr_bad_size(tb, CTA_PROTO_MAX, cta_min_proto))
 		return -EINVAL;
 
-	if (!tb[CTA_PROTO_NUM-1])
+	/* CTA_PROTO_NUM has to be kept for backwards compatibility */
+	if (tb[CTA_PROTO-1])
+		tuple->dst.protonum = *(u_int8_t *)NFA_DATA(tb[CTA_PROTO-1]);
+	else if (tb[CTA_PROTO_NUM-1])
+		tuple->dst.protonum = 
+				*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+	else
 		return -EINVAL;
-	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
 
 	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
 
-- 
- 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: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Harald Welte]

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

Sorry, forget that other patch.

The only way how we can thoroughly solve (and avoid) this problem, also
for future cases, is to behave like 'real' TLV parsers (e.g. ASN1).

That is, for any kind of numeric values, we don't make an assumption
that the attribute has a certain fixed size.  Instead, we derive the
(u8/u16/u32/u64) size from the length of the attribute, i.e.
NFA_PAYLOAD() is 1/2/4/8 bytes long.

This is a quick and dirty patch to demonstrate what I mean:

=====

diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
--- a/net/ipv4/netfilter/ip_conntrack_netlink.c
+++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
@@ -502,12 +502,13 @@ ctnetlink_parse_tuple_ip(struct nfattr *
 }
 
 static const size_t cta_min_proto[CTA_PROTO_MAX] = {
-	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
+	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_ICMP_CODE-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_ICMP_ID-1]	= sizeof(u_int16_t),
+	[CTA_PROTO-1]		= sizeof(u_int8_t),
 };
 
 static inline int
@@ -527,7 +528,18 @@ ctnetlink_parse_tuple_proto(struct nfatt
 
 	if (!tb[CTA_PROTO_NUM-1])
 		return -EINVAL;
-	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+
+	switch (NFA_PAYLOAD(tb[CTA_PROTO_NUM-1]))
+	case sizeof(u_int8_t):
+		tuple->dst.protonum = 
+				*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+		break;
+	case sizeof(u_int16_t):
+		tuple->dst.protonum = 
+				*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+	default:
+		return -EINVAL;
+	}
 
 	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);

=====

Obviously, this needs to be moved into a nfnetlink core funciton,
something like a function nfattr_parse_number() that would then be
called from all places that parse a number.

Userspace parsers (libnetfilter_conntrack) would have to introduce the
same semantics.

It might be a bit too much overhead, I'm not really decided yet.  But in
the end, if everybody plays according to that rule, we don't have any
such issues in the future.

Comments?

-- 
- 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: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2575 bytes --]



On Thu, 24 Nov 2005, Harald Welte wrote:

> Sorry, forget that other patch.
>
> The only way how we can thoroughly solve (and avoid) this problem, also
> for future cases, is to behave like 'real' TLV parsers (e.g. ASN1).
>
> That is, for any kind of numeric values, we don't make an assumption
> that the attribute has a certain fixed size.  Instead, we derive the
> (u8/u16/u32/u64) size from the length of the attribute, i.e.
> NFA_PAYLOAD() is 1/2/4/8 bytes long.
>
> This is a quick and dirty patch to demonstrate what I mean:
>
> =====
>
> diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
> --- a/net/ipv4/netfilter/ip_conntrack_netlink.c
> +++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
> @@ -502,12 +502,13 @@ ctnetlink_parse_tuple_ip(struct nfattr *
> }
>
> static const size_t cta_min_proto[CTA_PROTO_MAX] = {
> -	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
> +	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
> 	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
> 	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
> 	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
> 	[CTA_PROTO_ICMP_CODE-1]	= sizeof(u_int8_t),
> 	[CTA_PROTO_ICMP_ID-1]	= sizeof(u_int16_t),
> +	[CTA_PROTO-1]		= sizeof(u_int8_t),
> };

Why we need to add CTA_PROTO here?

> static inline int
> @@ -527,7 +528,18 @@ ctnetlink_parse_tuple_proto(struct nfatt
>
> 	if (!tb[CTA_PROTO_NUM-1])
> 		return -EINVAL;
> -	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +
> +	switch (NFA_PAYLOAD(tb[CTA_PROTO_NUM-1]))
> +	case sizeof(u_int8_t):
> +		tuple->dst.protonum =
> +				*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +		break;
> +	case sizeof(u_int16_t):
> +		tuple->dst.protonum =
> +				*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +	default:
> +		return -EINVAL;
> +	}

Nice, nice... ;)

> Obviously, this needs to be moved into a nfnetlink core funciton,
> something like a function nfattr_parse_number() that would then be
> called from all places that parse a number.
>
> Userspace parsers (libnetfilter_conntrack) would have to introduce the
> same semantics.

Old userspace applications always send u_int16_t. New applications still 
need to send CTA_PROTO_NUM as u_int16_t for 2.6.14 but u_int8_t for 
2.6.15+ kernels. I'm not sure if we can provide that without checking 
current kernel version. Hopefully, when reading messages from kernel, 
userspace expects CTA_PROTO_NUM to be u_int8_t and this is what kernel 
sends currently.

Best regards,

 				Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Harald Welte wrote:
> Sorry, forget that other patch.
> 
> The only way how we can thoroughly solve (and avoid) this problem, also
> for future cases, is to behave like 'real' TLV parsers (e.g. ASN1).
> 
> That is, for any kind of numeric values, we don't make an assumption
> that the attribute has a certain fixed size.  Instead, we derive the
> (u8/u16/u32/u64) size from the length of the attribute, i.e.
> NFA_PAYLOAD() is 1/2/4/8 bytes long.
> 
> This is a quick and dirty patch to demonstrate what I mean:
> 
> =====
> 
> diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
> --- a/net/ipv4/netfilter/ip_conntrack_netlink.c
> +++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
> @@ -502,12 +502,13 @@ ctnetlink_parse_tuple_ip(struct nfattr *
>  }
>  
>  static const size_t cta_min_proto[CTA_PROTO_MAX] = {
> -	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
> +	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
>  	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
>  	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
>  	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
>  	[CTA_PROTO_ICMP_CODE-1]	= sizeof(u_int8_t),
>  	[CTA_PROTO_ICMP_ID-1]	= sizeof(u_int16_t),
> +	[CTA_PROTO-1]		= sizeof(u_int8_t),
>  };
>  
>  static inline int
> @@ -527,7 +528,18 @@ ctnetlink_parse_tuple_proto(struct nfatt
>  
>  	if (!tb[CTA_PROTO_NUM-1])
>  		return -EINVAL;
> -	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +
> +	switch (NFA_PAYLOAD(tb[CTA_PROTO_NUM-1]))
> +	case sizeof(u_int8_t):
> +		tuple->dst.protonum = 
> +				*(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +		break;
> +	case sizeof(u_int16_t):
> +		tuple->dst.protonum = 
> +				*(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
> 
> =====
> 
> Obviously, this needs to be moved into a nfnetlink core funciton,
> something like a function nfattr_parse_number() that would then be
> called from all places that parse a number.
> 
> Userspace parsers (libnetfilter_conntrack) would have to introduce the
> same semantics.

That won't work for this case since usually u_int16_t's are encoded
in network byte order but in this case its always host byte order.

> It might be a bit too much overhead, I'm not really decided yet.  But in
> the end, if everybody plays according to that rule, we don't have any
> such issues in the future.
> 
> Comments?

I think just the first patch is fine. I really hope we don't find
more of these.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1241 bytes --]



On Fri, 25 Nov 2005, Patrick McHardy wrote:
<CUT>
> I think just the first patch is fine. I really hope we don't find
> more of these.

It is not. It breaks old binaries:

diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
--- a/net/ipv4/netfilter/ip_conntrack_netlink.c
+++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
@@ -57,7 +57,7 @@ ctnetlink_dump_tuples_proto(struct sk_bu
         struct ip_conntrack_protocol *proto;
         int ret = 0;

-       NFA_PUT(skb, CTA_PROTO_NUM, sizeof(u_int8_t), &tuple->dst.protonum);
+       NFA_PUT(skb, CTA_PROTO, sizeof(u_int8_t), &tuple->dst.protonum);

         /* If no protocol helper is found, this function will return the
          * generic protocol helper, so proto won't *ever* be NULL */

Old binaries are not able to deal with CTA_PROTO and will not be able to 
parse this attribute received from kernel. We should keep CTA_PROTO_NUM 
here, but this leads to u_int8_t/u_int16_t mismatch in the code.

Anyway, new userspace applications will only need to send both 
CTA_PROTO_NUM and CTA_PROTO to work with both new and old kernel. No 
kernel version checking. Great.

Best regards,

 			Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Krzysztof Oledzki wrote:
> Old binaries are not able to deal with CTA_PROTO and will not be able to 
> parse this attribute received from kernel. We should keep CTA_PROTO_NUM 
> here, but this leads to u_int8_t/u_int16_t mismatch in the code.

I didn't notice that, we of course need to dump both attributes. But I
still think Pablo's suggestion is fine too and avoids all these hacks.
The only people affected will be the ones not using the stable series.

BTW, any reason why ctnetlink is not marked as experimental?

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Pablo Neira]

Patrick McHardy wrote:
> Krzysztof Oledzki wrote:
> 
>> Old binaries are not able to deal with CTA_PROTO and will not be able
>> to parse this attribute received from kernel. We should keep
>> CTA_PROTO_NUM here, but this leads to u_int8_t/u_int16_t mismatch in
>> the code.
> 
> I didn't notice that, we of course need to dump both attributes. But I
> still think Pablo's suggestion is fine too and avoids all these hacks.
> The only people affected will be the ones not using the stable series.

Same feeling. I don't like the idea of breaking backward compatibility,
but in this case the hacks don't look really nice either :(. Since this
stuff was just pushed forward to 2.6.14, I still think that we should
send a patch to fix it in -stable.

-- 
Pablo

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 961 bytes --]



On Fri, 25 Nov 2005, Pablo Neira wrote:

> Patrick McHardy wrote:
>> Krzysztof Oledzki wrote:
>>
>>> Old binaries are not able to deal with CTA_PROTO and will not be able
>>> to parse this attribute received from kernel. We should keep
>>> CTA_PROTO_NUM here, but this leads to u_int8_t/u_int16_t mismatch in
>>> the code.
>>
>> I didn't notice that, we of course need to dump both attributes. But I
>> still think Pablo's suggestion is fine too and avoids all these hacks.
>> The only people affected will be the ones not using the stable series.
>
> Same feeling. I don't like the idea of breaking backward compatibility,
> but in this case the hacks don't look really nice either :(. Since this
> stuff was just pushed forward to 2.6.14, I still think that we should
> send a patch to fix it in -stable.

But 2.6.14.3 just went out and I'm not sure if there will be another 
2.6.14.x relase.

Best regards,

 				Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 521 bytes --]



On Fri, 25 Nov 2005, Patrick McHardy wrote:

> Krzysztof Oledzki wrote:
>> Old binaries are not able to deal with CTA_PROTO and will not be able to 
>> parse this attribute received from kernel. We should keep CTA_PROTO_NUM 
>> here, but this leads to u_int8_t/u_int16_t mismatch in the code.
>
> I didn't notice that, we of course need to dump both attributes.
Both? We already send CTA_PROTO_NUM as u_int8_t to userspace and 
libnfnetlink has no problem with that.

Best regards,

 			Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Harald Welte]

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

On Fri, Nov 25, 2005 at 12:54:56AM +0100, Krzysztof Oledzki wrote:
> 
> 
> On Fri, 25 Nov 2005, Patrick McHardy wrote:
> <CUT>
> >I think just the first patch is fine. I really hope we don't find
> >more of these.
> 
> It is not. It breaks old binaries:

yes.  old libnetfilter_conntrack is broken, because it makes wrong size
assumptions.  But if we'd start introdcucing the "new scheme" I
proposed, we can guarantee not to run into any of these.

-- 
- 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: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 700 bytes --]



On Fri, 25 Nov 2005, Harald Welte wrote:

> On Fri, Nov 25, 2005 at 12:54:56AM +0100, Krzysztof Oledzki wrote:
>>
>>
>> On Fri, 25 Nov 2005, Patrick McHardy wrote:
>> <CUT>
>>> I think just the first patch is fine. I really hope we don't find
>>> more of these.
>>
>> It is not. It breaks old binaries:
>
> yes.  old libnetfilter_conntrack is broken, because it makes wrong size
> assumptions.  But if we'd start introdcucing the "new scheme" I
> proposed, we can guarantee not to run into any of these.
Only if new libnetfilter_conntrack will send different messages to 
differnet kernels (u_int16_t for 2.6.14 and u_int8_t for 2.6.15+).

Best regards,

 			Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Harald Welte]

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

On Fri, Nov 25, 2005 at 10:23:54AM +0100, Krzysztof Oledzki wrote:
> 
> 
> On Fri, 25 Nov 2005, Harald Welte wrote:
> 
> >On Fri, Nov 25, 2005 at 12:54:56AM +0100, Krzysztof Oledzki wrote:
> >>On Fri, 25 Nov 2005, Patrick McHardy wrote:
> >><CUT>
> >>>I think just the first patch is fine. I really hope we don't find
> >>>more of these.
> >>It is not. It breaks old binaries:
> >yes.  old libnetfilter_conntrack is broken, because it makes wrong size
> >assumptions.  But if we'd start introdcucing the "new scheme" I
> >proposed, we can guarantee not to run into any of these.
> Only if new libnetfilter_conntrack will send different messages to
> differnet kernels (u_int16_t for 2.6.14 and u_int8_t for 2.6.15+).

That's what I meant with 'old lib is broken' (old kernels, too).

-- 
- 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: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Krzysztof Oledzki wrote:
> 
> On Fri, 25 Nov 2005, Harald Welte wrote:
> 
>> yes.  old libnetfilter_conntrack is broken, because it makes wrong size
>> assumptions.  But if we'd start introdcucing the "new scheme" I
>> proposed, we can guarantee not to run into any of these.
> 
> Only if new libnetfilter_conntrack will send different messages to 
> differnet kernels (u_int16_t for 2.6.14 and u_int8_t for 2.6.15+).

I also thinks its a large overkill to introduce this scheme just to
handle brokeness, lets just try not to make this kind of mistake again.
Also it doesn't handle the byteorder-problem of this particular case,
so I don't think we should go this way.

My favourite solutions:

- Pablo's suggestion
- your first patch, but keep dumping the old CTA_PROTO_NUM

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Krzysztof Oledzki wrote:
>>
>> On Fri, 25 Nov 2005, Harald Welte wrote:
>>
>>> yes.  old libnetfilter_conntrack is broken, because it makes wrong size
>>> assumptions.  But if we'd start introdcucing the "new scheme" I
>>> proposed, we can guarantee not to run into any of these.
>>
>>
>> Only if new libnetfilter_conntrack will send different messages to
>> differnet kernels (u_int16_t for 2.6.14 and u_int8_t for 2.6.15+).
> 
> I also thinks its a large overkill to introduce this scheme just to
> handle brokeness, lets just try not to make this kind of mistake again.
> Also it doesn't handle the byteorder-problem of this particular case,
> so I don't think we should go this way.

I agree. Sorry, I'm not willing to be repetitive but I still see a
possible workaround as something ugly that we'll have to live with
forever because of an early mistake in the development. If there will be
more 2.6.14.x release, fixing it there can be a choice. In spite of
everything, I understand that breaking backward compatibility isn't nice
but in this case ctnetlink just got pushed forward and it's barely one
month old.

-- 
Pablo

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1586 bytes --]



On Sat, 26 Nov 2005, Pablo Neira Ayuso wrote:

> Patrick McHardy wrote:
>> Krzysztof Oledzki wrote:
>>>
>>> On Fri, 25 Nov 2005, Harald Welte wrote:
>>>
>>>> yes.  old libnetfilter_conntrack is broken, because it makes wrong size
>>>> assumptions.  But if we'd start introdcucing the "new scheme" I
>>>> proposed, we can guarantee not to run into any of these.
>>>
>>>
>>> Only if new libnetfilter_conntrack will send different messages to
>>> differnet kernels (u_int16_t for 2.6.14 and u_int8_t for 2.6.15+).
>>
>> I also thinks its a large overkill to introduce this scheme just to
>> handle brokeness, lets just try not to make this kind of mistake again.
>> Also it doesn't handle the byteorder-problem of this particular case,
>> so I don't think we should go this way.
>
> I agree. Sorry, I'm not willing to be repetitive but I still see a
> possible workaround as something ugly that we'll have to live with
> forever because of an early mistake in the development. If there will be
> more 2.6.14.x release, fixing it there can be a choice. In spite of
> everything, I understand that breaking backward compatibility isn't nice
> but in this case ctnetlink just got pushed forward and it's barely one
> month old.

So, I think what we can do is to fix it in 2.6.15, leave it unchanged in 
2.6.14 and add kernel version checking to the libnetfilter_conntrack to 
send u_int16_t for 2.6.14 and u_int8_t for 2.6.15+ (one if/else). I don't 
know if (and when) there will be another 2.6.14-stable release.

Best regards,

 				Krzysztof Olędzki

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Harald Welte]

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

On Sun, Nov 27, 2005 at 11:28:47PM +0100, Krzysztof Oledzki wrote:
> 
> 
> On Sat, 26 Nov 2005, Pablo Neira Ayuso wrote:
> 
> >Patrick McHardy wrote:
> >>Krzysztof Oledzki wrote:
> >>>On Fri, 25 Nov 2005, Harald Welte wrote:
> >>>>yes.  old libnetfilter_conntrack is broken, because it makes wrong size
> >>>>assumptions.  But if we'd start introdcucing the "new scheme" I
> >>>>proposed, we can guarantee not to run into any of these.
> >>>Only if new libnetfilter_conntrack will send different messages to
> >>>differnet kernels (u_int16_t for 2.6.14 and u_int8_t for 2.6.15+).
> >>I also thinks its a large overkill to introduce this scheme just to
> >>handle brokeness, lets just try not to make this kind of mistake again.
> >>Also it doesn't handle the byteorder-problem of this particular case,
> >>so I don't think we should go this way.
> >I agree. Sorry, I'm not willing to be repetitive but I still see a
> >possible workaround as something ugly that we'll have to live with
> >forever because of an early mistake in the development. If there will be
> >more 2.6.14.x release, fixing it there can be a choice. In spite of
> >everything, I understand that breaking backward compatibility isn't nice
> >but in this case ctnetlink just got pushed forward and it's barely one
> >month old.
> 
> So, I think what we can do is to fix it in 2.6.15, leave it unchanged
> in 2.6.14 and add kernel version checking to the
> libnetfilter_conntrack to send u_int16_t for 2.6.14 and u_int8_t for
> 2.6.15+ (one if/else). I don't know if (and when) there will be
> another 2.6.14-stable release.

No, I oppose any kind of kernel version number checking.  I'd rather
break 2.6.14 with new versions of the userspace.

-- 
- 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: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Harald Welte wrote:
> No, I oppose any kind of kernel version number checking.  I'd rather
> break 2.6.14 with new versions of the userspace.
> 

The stable tree has a couple of patches pending, so I guess there
will be another release. I'll ask them if they would take a patch
to fix this issue.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Harald Welte wrote:
> 
>> No, I oppose any kind of kernel version number checking.  I'd rather
>> break 2.6.14 with new versions of the userspace.
>>
> The stable tree has a couple of patches pending, so I guess there
> will be another release. I'll ask them if they would take a patch
> to fix this issue.

Any update on this?

-- 
Pablo

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

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

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>>Harald Welte wrote:
>>
>>
>>>No, I oppose any kind of kernel version number checking.  I'd rather
>>>break 2.6.14 with new versions of the userspace.
>>>
>>
>>The stable tree has a couple of patches pending, so I guess there
>>will be another release. I'll ask them if they would take a patch
>>to fix this issue.
> 
> 
> Any update on this?

I've added this patch to my queue. It doesn't even affect compatiblity,
userspace can send both u_int8_t or u_int16_t, the binary representation
will look the same. Changing the library will however break
compatibility with old kernels, I'll try to push this patch to -stable
as well.


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

[NETFILTER]: Fix CTA_PROTO_NUM attribute size in ctnetlink

CTA_PROTO_NUM is a u_int8_t.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 0fa82f8caa129bb2377e1b593bf2986fc13c5391
tree 0a7479acf48c9de99cb6f9fd1fac3a01dec2e220
parent 01563e56ad1c48c85e1258e2eaabcc270385e1a5
author Patrick McHardy <kaber@trash.net> Sun, 04 Dec 2005 17:00:48 +0100
committer Patrick McHardy <kaber@trash.net> Sun, 04 Dec 2005 17:00:48 +0100

 net/ipv4/netfilter/ip_conntrack_netlink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/netfilter/ip_conntrack_netlink.c b/net/ipv4/netfilter/ip_conntrack_netlink.c
index 70402e0..d058ac4 100644
--- a/net/ipv4/netfilter/ip_conntrack_netlink.c
+++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
@@ -503,7 +503,7 @@ ctnetlink_parse_tuple_ip(struct nfattr *
 }
 
 static const size_t cta_min_proto[CTA_PROTO_MAX] = {
-	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
+	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
@@ -528,7 +528,7 @@ ctnetlink_parse_tuple_proto(struct nfatt
 
 	if (!tb[CTA_PROTO_NUM-1])
 		return -EINVAL;
-	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+	tuple->dst.protonum = *(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
 
 	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
 

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
> 
>> Any update on this?
> 
> 
> I've added this patch to my queue. It doesn't even affect compatiblity,
> userspace can send both u_int8_t or u_int16_t, the binary representation
> will look the same.

After wakeing up entirely, I noticed this is nonsense, it depends on the
byte-order of course. Anyway, I'm going to push this patch forward
tonight, it seems libconntrack currently has a number of issues anyway.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sun, 04 Dec 2005 17:05:06 +0100

> It doesn't even affect compatiblity, userspace can send both
> u_int8_t or u_int16_t, the binary representation will look the same.

On little-endian, yes.  But on big-endian you'll get a byte-swapped
value for the case where userspace sends in a u8.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t

[Patrick McHardy]

David S. Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Sun, 04 Dec 2005 17:05:06 +0100
> 
> 
>>It doesn't even affect compatiblity, userspace can send both
>>u_int8_t or u_int16_t, the binary representation will look the same.
> 
> 
> On little-endian, yes.  But on big-endian you'll get a byte-swapped
> value for the case where userspace sends in a u8.

Yes. Unfortunately we don't have much choice because kernel and
userspace use both u_int8_t and u_int16_t for CTA_PROTO_NUM. Some
ugly workarounds might be possible, but considering that ctnetlink
is new and how much problems it still has, it shouldn't be a big
problem to do this right and break compatibility.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sun, 04 Dec 2005 21:02:08 +0100

> Some ugly workarounds might be possible, but considering that
> ctnetlink is new and how much problems it still has, it shouldn't be
> a big problem to do this right and break compatibility.

Yes, this is how I see it too.

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Krzysztof Oledzki]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 756 bytes --]



On Wed, 30 Nov 2005, Patrick McHardy wrote:

> Harald Welte wrote:
>> No, I oppose any kind of kernel version number checking.  I'd rather
>> break 2.6.14 with new versions of the userspace.
>> 
>
> The stable tree has a couple of patches pending, so I guess there
> will be another release. I'll ask them if they would take a patch
> to fix this issue.
>

The review cycle for the 2.6.14.4 was started. I can't find this fix in 
listed patches and it seems 2.6.14 needs little different patch than 
2.6.15: s/size_t/int/.

Patch attached, not sure what to do with Signed-off-by lines so please 
feel free to correct it.

Any chances for submitting it into -stable for inclusion in 2.6.14.4?

Best regards,

 			Krzysztof Olędzki

[-- Attachment #2: Type: TEXT/PLAIN, Size: 995 bytes --]

[NETFILTER]: Fix CTA_PROTO_NUM attribute size in ctnetlink (2.6.14.x version)

CTA_PROTO_NUM is a u_int8_t.

Based on oryginal patch by Patrick McHardy <kaber@trash.net>

Signed-off-by: Krzysztof Piotr Oledzki <ole@ans.pl>

--- a/net/ipv4/netfilter/ip_conntrack_netlink.c
+++ b/net/ipv4/netfilter/ip_conntrack_netlink.c
@@ -503,7 +503,7 @@ ctnetlink_parse_tuple_ip(struct nfattr *
 }
 
 static const int cta_min_proto[CTA_PROTO_MAX] = {
-	[CTA_PROTO_NUM-1]	= sizeof(u_int16_t),
+	[CTA_PROTO_NUM-1]	= sizeof(u_int8_t),
 	[CTA_PROTO_SRC_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_DST_PORT-1]	= sizeof(u_int16_t),
 	[CTA_PROTO_ICMP_TYPE-1]	= sizeof(u_int8_t),
@@ -528,7 +528,7 @@ ctnetlink_parse_tuple_proto(struct nfatt
 
 	if (!tb[CTA_PROTO_NUM-1])
 		return -EINVAL;
-	tuple->dst.protonum = *(u_int16_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
+	tuple->dst.protonum = *(u_int8_t *)NFA_DATA(tb[CTA_PROTO_NUM-1]);
 
 	proto = ip_conntrack_proto_find_get(tuple->dst.protonum);
 

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Patrick McHardy]

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

Krzysztof Oledzki wrote:
> The review cycle for the 2.6.14.4 was started. I can't find this fix in 
> listed patches and it seems 2.6.14 needs little different patch than 
> 2.6.15: s/size_t/int/.
> 
> Patch attached, not sure what to do with Signed-off-by lines so please 
> feel free to correct it.
> 
> Any chances for submitting it into -stable for inclusion in 2.6.14.4?

Sorry, I wanted to wait until the patch hit Linus' tree and
forgot about it. Unfortunately ctnetlink has a number of other
issues in 2.6.14 that don't fulfil the -stable requirements,
so in the end its still pretty unusable.

Anyway, this patch fixes a deadlock when dumping the conntrack
table which has already hit a number of people. Please consider
for -stable.

The patch Krzysztof attached went into 2.6.15-rc and fixes an
attribute sizes that was used inconsistently. Without this patch
compatiblity will break once we fix up the userspace side. The
first released kernel with ctnetlink was 2.6.14, so far the only
user known to me is a tool in beta-stage that lives in netfilter
SVN. We would prefer to have no incompatiblities between at least
2.6.14.x and later kernels, so is something like this acceptable
for -stable? If yes I'll send a patch that applies cleanly to
2.6.14.3.

[-- Attachment #2: ctnl-01.diff --]
[-- Type: text/x-patch, Size: 1468 bytes --]

[NETFILTER]: Fix unbalanced read_unlock_bh in ctnetlink

NFA_NEST calls NFA_PUT which jumps to nfattr_failure if the skb has no
room left. We call read_unlock_bh at nfattr_failure for the NFA_PUT inside
the locked section, so move NFA_NEST inside the locked section too.

Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: David S. Miller <davem@davemloft.net>

---
commit 266c8543480e2202ab63d1d604a5ca049f350cd8
tree 77c754dce63f39e1f9dc2d1768ecd348c1d50c74
parent 6636568cf85ef5898a892e90fcc88b61cca9ca27
author Patrick McHardy <kaber@trash.net> Mon, 05 Dec 2005 13:37:33 -0800
committer David S. Miller <davem@davemloft.net> Mon, 05 Dec 2005 13:37:33 -0800

 net/ipv4/netfilter/ip_conntrack_proto_tcp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
index aeb7353..e7fa29e 100644
--- a/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
+++ b/net/ipv4/netfilter/ip_conntrack_proto_tcp.c
@@ -341,9 +341,10 @@ static int tcp_print_conntrack(struct se
 static int tcp_to_nfattr(struct sk_buff *skb, struct nfattr *nfa,
 			 const struct ip_conntrack *ct)
 {
-	struct nfattr *nest_parms = NFA_NEST(skb, CTA_PROTOINFO_TCP);
+	struct nfattr *nest_parms;
 	
 	read_lock_bh(&tcp_lock);
+	nest_parms = NFA_NEST(skb, CTA_PROTOINFO_TCP);
 	NFA_PUT(skb, CTA_PROTOINFO_TCP_STATE, sizeof(u_int8_t),
 		&ct->proto.tcp.state);
 	read_unlock_bh(&tcp_lock);

Re: [PATCH] CTA_PROTO_NUM is u_int8_t not u_int16_t (was Re: CTA_PROTO_NUM u_int8_t or u_int16_t)

[Pablo Neira Ayuso]

Hi Patrick,

Patrick McHardy wrote:
> Krzysztof Oledzki wrote:
> 
>> The review cycle for the 2.6.14.4 was started. I can't find this fix
>> in listed patches and it seems 2.6.14 needs little different patch
>> than 2.6.15: s/size_t/int/.
>>
>> Patch attached, not sure what to do with Signed-off-by lines so please
>> feel free to correct it.
>>
>> Any chances for submitting it into -stable for inclusion in 2.6.14.4?
> 
> 
> Sorry, I wanted to wait until the patch hit Linus' tree and
> forgot about it. Unfortunately ctnetlink has a number of other
> issues in 2.6.14 that don't fulfil the -stable requirements,
> so in the end its still pretty unusable.
> 
> Anyway, this patch fixes a deadlock when dumping the conntrack
> table which has already hit a number of people. Please consider
> for -stable.
> 
> The patch Krzysztof attached went into 2.6.15-rc and fixes an
> attribute sizes that was used inconsistently. Without this patch
> compatiblity will break once we fix up the userspace side. The
> first released kernel with ctnetlink was 2.6.14, so far the only
> user known to me is a tool in beta-stage that lives in netfilter
> SVN. We would prefer to have no incompatiblities between at least
> 2.6.14.x and later kernels, so is something like this acceptable
> for -stable? If yes I'll send a patch that applies cleanly to
> 2.6.14.3.

The userspace part (libnetfilter_conntrack) is GPL at the moment, I
don't known any other GPL tool using the library at the moment apart
from the conntrack-tool. So I wouldn't care so much about breaking third
party applications that are not fulfilling the licensing requirements.

-- 
Pablo