CTA_PROTO_NUM u_int8_t or u_int16_t

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?

Quoted 11 lines · Krzysztof Oledzki

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]);

Quoted 1 line

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.

Quoted 17 lines · Pablo Neira

Quoted 13 lines · Krzysztof Oledzki

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?

Quoted 4 lines

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.

Quoted 24 lines · Patrick McHardy

Quoted 21 lines · Pablo Neira

Quoted 5 lines · Krzysztof Oledzki

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;

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 3 lines · Patrick McHardy

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.

Quoted 8 lines · Pablo Neira

Quoted 3 lines · Patrick McHardy

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.

Quoted 11 lines · Patrick McHardy

Quoted 8 lines · Pablo Neira

Quoted 3 lines · Patrick McHardy

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.

Quoted 7 lines

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.

Quoted 15 lines · Pablo Neira

Quoted 14 lines · Patrick McHardy

Quoted 6 lines · Pablo Neira

Quoted 3 lines · Patrick McHardy

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.

Quoted 2 lines

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

Quoted 24 lines · Krzysztof Oledzki

Quoted 21 lines · Pablo Neira

Quoted 18 lines · Patrick McHardy

Quoted 8 lines · Pablo Neira

Quoted 3 lines · Patrick McHardy

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 2 lines · Harald Welte

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

Quoted 6 lines · Pablo Neira

Quoted 2 lines · Harald Welte

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

Quoted 4 lines

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.

Quoted 4 lines · Patrick McHardy

Quoted 3 lines · Pablo Neira

Quoted 2 lines · Harald Welte

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

Quoted 5 lines

Quoted 4 lines

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.

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:

=====

net/ipv4/netfilter/ip_conntrack_netlink.c +0 -0

@@ -502,12 +502,13 @@ ctnetlink_parse_tuple_ip(struct nfattr *
 }
 

static inline int

net/ipv4/netfilter/ip_conntrack_netlink.c +2 -1

@@ -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]))

net/ipv4/netfilter/ip_conntrack_netlink.c +0 -0

 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.

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:

=====

net/ipv4/netfilter/ip_conntrack_netlink.c +14 -2

@@ -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?

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

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.

Quoted 4 lines · Krzysztof Oledzki

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.

Quoted 7 lines · Patrick McHardy

Quoted 4 lines · Krzysztof Oledzki

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.

Quoted 3 lines

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.

Quoted 1 line · Krzysztof Oledzki

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.

On Fri, 25 Nov 2005, Patrick McHardy wrote:
<CUT>

Quoted 2 lines

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:

Quoted 4 lines · Krzysztof Oledzki

On Fri, 25 Nov 2005, Patrick McHardy wrote:
<CUT>

Quoted 4 lines

Quoted 2 lines

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.

Quoted 3 lines · Harald Welte

Quoted 2 lines · Krzysztof Oledzki

On Fri, 25 Nov 2005, Patrick McHardy wrote:
<CUT>

Quoted 6 lines

Quoted 3 lines

Quoted 2 lines

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+).

Quoted 3 lines · Harald Welte

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+).

Quoted 8 lines · Krzysztof Oledzki

Quoted 3 lines · Harald Welte

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.

Quoted 2 lines

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.

Quoted 9 lines · Patrick McHardy

Quoted 8 lines · Krzysztof Oledzki

Quoted 3 lines · Harald Welte

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 12 lines · Pablo Neira Ayuso

Quoted 11 lines · Patrick McHardy

Quoted 6 lines · Krzysztof Oledzki

Quoted 3 lines · Harald Welte

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.

Quoted 7 lines

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.

Quoted 3 lines · Harald Welte

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.

Quoted 7 lines · Patrick McHardy

Quoted 3 lines · Harald Welte

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

Quoted 3 lines

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?

Quoted 1 line · Pablo Neira Ayuso

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.

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

From: Patrick McHardy <[email protected]>
Date: Sun, 04 Dec 2005 17:05:06 +0100

Quoted 2 lines

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.

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.

Quoted 3 lines · Harald Welte

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?

Quoted 8 lines · Krzysztof Oledzki

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.