Mailing List Archive: iptables: Devel

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

1 2

View All

Index | Next | Previous | View Threaded

pablo at eurodev

Nov 22, 2005, 11:04 AM

Post #1 of 33 (1273 views)
Permalink

[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 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

Attachments:

u8proto_num.patch (1.07 KB)

olenf at ans

Nov 22, 2005, 12:29 PM

Post #2 of 33 (1230 views)
Permalink

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) [In reply to]

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

laforge at netfilter

Nov 22, 2005, 2:06 PM

Post #3 of 33 (1251 views)
Permalink

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) [In reply to]

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 [at] netfilter> 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

kaber at trash

Nov 22, 2005, 5:06 PM

Post #4 of 33 (1232 views)
Permalink

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) [In reply to]

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 :)

pablo at eurodev

Nov 22, 2005, 5:15 PM

Post #5 of 33 (1252 views)
Permalink

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) [In reply to]

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

kaber at trash

Nov 23, 2005, 1:47 AM

Post #6 of 33 (1246 views)
Permalink

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) [In reply to]

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.

olenf at ans

Nov 23, 2005, 2:31 AM

Post #7 of 33 (1229 views)
Permalink

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) [In reply to]

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

laforge at netfilter

Nov 24, 2005, 12:07 PM

Post #8 of 33 (1233 views)
Permalink

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) [In reply to]

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 [at] netfilter> 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

laforge at netfilter

Nov 24, 2005, 12:21 PM

Post #9 of 33 (1247 views)
Permalink

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) [In reply to]

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 [at] netfilter> 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

olenf at ans

Nov 24, 2005, 3:24 PM

Post #10 of 33 (1245 views)
Permalink

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) [In reply to]

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

kaber at trash

Nov 24, 2005, 3:33 PM

Post #11 of 33 (1239 views)
Permalink

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) [In reply to]

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.

olenf at ans

Nov 24, 2005, 3:54 PM

Post #12 of 33 (1225 views)
Permalink

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) [In reply to]

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

kaber at trash

Nov 24, 2005, 4:11 PM

Post #13 of 33 (1233 views)
Permalink

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) [In reply to]

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?

pablo at eurodev

Nov 24, 2005, 4:22 PM

Post #14 of 33 (1236 views)
Permalink

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) [In reply to]

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

olenf at ans

Nov 24, 2005, 4:26 PM

Post #15 of 33 (1224 views)
Permalink

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) [In reply to]

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

olenf at ans

Nov 24, 2005, 4:28 PM

Post #16 of 33 (1228 views)
Permalink

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) [In reply to]

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

laforge at netfilter

Nov 25, 2005, 12:44 AM

Post #17 of 33 (1227 views)
Permalink

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) [In reply to]

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 [at] netfilter> 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

olenf at ans

Nov 25, 2005, 1:23 AM

Post #18 of 33 (1233 views)
Permalink

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) [In reply to]

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

laforge at netfilter

Nov 25, 2005, 3:09 AM

Post #19 of 33 (1246 views)
Permalink

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) [In reply to]

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 [at] netfilter> 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

kaber at trash

Nov 25, 2005, 5:25 AM

Post #20 of 33 (1236 views)
Permalink

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) [In reply to]

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

pablo at eurodev

Nov 25, 2005, 4:16 PM

Post #21 of 33 (1235 views)
Permalink

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) [In reply to]

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

olenf at ans

Nov 27, 2005, 2:28 PM

Post #22 of 33 (1248 views)
Permalink

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) [In reply to]

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

laforge at netfilter

Nov 28, 2005, 8:09 PM

Post #23 of 33 (1231 views)
Permalink

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) [In reply to]

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 [at] netfilter> 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

kaber at trash

Nov 29, 2005, 3:07 PM

Post #24 of 33 (1215 views)
Permalink

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) [In reply to]

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.

pablo at eurodev

Dec 3, 2005, 7:31 PM

Post #25 of 33 (1221 views)
Permalink

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) [In reply to]

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

1 2

View All

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads