[PATCH 0/1] Adjust nfnetlink structure size between kernel and user space

[PATCH 0/1] Adjust nfnetlink structure size between kernel and user space

[Lutz Jaenicke]

The attached patch adjust the size of struct nf_ct_tcp_flags in the kernel
to match the hardcoded size in the respective user space tools.
This issue has so far only been visible for an Intel IXP4xx network
processor (ARM big endian, gcc-4.2.4) while the same code and
toolchain works fine on powerpc (32bit) and x86_64.

Lutz Jaenicke (1):
  netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

 include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Lutz Jaenicke]

On some platforms (here: Intel IXP4xx ARM big endian)
sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
space code 2 bytes is hard coded.
Add "__attribute__ ((__packed__))" to enforce matching structure
sizes.

Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
---
 include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter/nf_conntrack_tcp.h b/include/linux/netfilter/nf_conntrack_tcp.h
index 6e135f9..c7ee4ec 100644
--- a/include/linux/netfilter/nf_conntrack_tcp.h
+++ b/include/linux/netfilter/nf_conntrack_tcp.h
@@ -39,7 +39,7 @@ enum tcp_conntrack {
 /* The field td_maxack has been set */
 #define IP_CT_TCP_FLAG_MAXACK_SET		0x20
 
-struct nf_ct_tcp_flags {
+struct __attribute__ ((packed)) nf_ct_tcp_flags {
 	__u8 flags;
 	__u8 mask;
 };
-- 
1.6.0.3

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Eric Dumazet]

Le mardi 09 mars 2010 à 17:48 +0100, Lutz Jaenicke a écrit :
> On some platforms (here: Intel IXP4xx ARM big endian)
> sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
> space code 2 bytes is hard coded.
> Add "__attribute__ ((__packed__))" to enforce matching structure
> sizes.
> 
> Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> ---
>  include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/netfilter/nf_conntrack_tcp.h b/include/linux/netfilter/nf_conntrack_tcp.h
> index 6e135f9..c7ee4ec 100644
> --- a/include/linux/netfilter/nf_conntrack_tcp.h
> +++ b/include/linux/netfilter/nf_conntrack_tcp.h
> @@ -39,7 +39,7 @@ enum tcp_conntrack {
>  /* The field td_maxack has been set */
>  #define IP_CT_TCP_FLAG_MAXACK_SET		0x20
>  
> -struct nf_ct_tcp_flags {
> +struct __attribute__ ((packed)) nf_ct_tcp_flags {
>  	__u8 flags;
>  	__u8 mask;
>  };

Preferred way is to use :

struct nf_ct_tcp_flags {
	...
	} __attribute__ ((packed));


This eases grep games a bit.

Thanks


--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Lutz Jaenicke]

On Tue, Mar 09, 2010 at 06:02:30PM +0100, Eric Dumazet wrote:
> Le mardi 09 mars 2010 à 17:48 +0100, Lutz Jaenicke a écrit :
> >  
> > -struct nf_ct_tcp_flags {
> > +struct __attribute__ ((packed)) nf_ct_tcp_flags {
> >  	__u8 flags;
> >  	__u8 mask;
> >  };
> 
> Preferred way is to use :
> 
> struct nf_ct_tcp_flags {
> 	...
> 	} __attribute__ ((packed));
> 
> 
> This eases grep games a bit.

Makes sense.
	Lutz
-- 
Dr.-Ing. Lutz Jänicke
CTO
Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.921028-200
fax: +49.30.921028-020
Rudower Chaussee 13
D-12489 Berlin, Germany
www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Volker Bibelhausen
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Jan Engelhardt]

On Tuesday 2010-03-09 17:48, Lutz Jaenicke wrote:

>On some platforms (here: Intel IXP4xx ARM big endian)
>sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
>space code 2 bytes is hard coded.

Why does it evaluate to 4 bytes? That seems to go against all good C 
practice.

>@@ -39,7 +39,7 @@ enum tcp_conntrack {
> /* The field td_maxack has been set */
> #define IP_CT_TCP_FLAG_MAXACK_SET		0x20
> 
>-struct nf_ct_tcp_flags {
>+struct __attribute__ ((packed)) nf_ct_tcp_flags {
> 	__u8 flags;
> 	__u8 mask;
> };

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Lutz Jaenicke]

On Tue, Mar 09, 2010 at 06:03:02PM +0100, Jan Engelhardt wrote:
> On Tuesday 2010-03-09 17:48, Lutz Jaenicke wrote:
> 
> >On some platforms (here: Intel IXP4xx ARM big endian)
> >sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
> >space code 2 bytes is hard coded.
> 
> Why does it evaluate to 4 bytes? That seems to go against all good C 
> practice.

I agree wrt the "against all good C practice" point of view :-)

Nevertheless it fails in the policy check via tcp_nla_policy[]:
  [CTA_PROTOINFO_TCP_FLAGS_ORIGINAL]  = { .len = sizeof(struct nf_ct_tcp_flags) },
  [CTA_PROTOINFO_TCP_FLAGS_REPLY]     = { .len =  sizeof(struct nf_ct_tcp_flags) },
The expected length in "validate_nla()" case "default:" is
	pt->len = 4
while from user space only 2 byte are sent.

If you have a good hint on how to answer the
  "Why does it evaluate to 4 bytes?"
question, you are welcome... I cannot explain, I just observed.

Best regards,
	Lutz
-- 
Dr.-Ing. Lutz Jänicke
CTO
Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.921028-200
fax: +49.30.921028-020
Rudower Chaussee 13
D-12489 Berlin, Germany
www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Volker Bibelhausen
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[David Miller]

From: Jan Engelhardt <jengelh@medozas.de>
Date: Tue, 9 Mar 2010 18:03:02 +0100 (CET)

> On Tuesday 2010-03-09 17:48, Lutz Jaenicke wrote:
> 
>>On some platforms (here: Intel IXP4xx ARM big endian)
>>sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
>>space code 2 bytes is hard coded.
> 
> Why does it evaluate to 4 bytes? That seems to go against all good C 
> practice.

It's just what the ARM ABI does.

We have to do something similar in the IPV6 stack for
the same reason.

struct nd_opt_hdr {
	__u8		nd_opt_type;
	__u8		nd_opt_len;
} __attribute__((__packed__));

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Lutz Jaenicke]

Against which tree do I have to re-submit this patch to get it merged?

Best regards,
	Lutz

On Tue, Mar 09, 2010 at 05:48:17PM +0100, Lutz Jaenicke wrote:
> On some platforms (here: Intel IXP4xx ARM big endian)
> sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
> space code 2 bytes is hard coded.
> Add "__attribute__ ((__packed__))" to enforce matching structure
> sizes.
> 
> Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> ---
>  include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/netfilter/nf_conntrack_tcp.h b/include/linux/netfilter/nf_conntrack_tcp.h
> index 6e135f9..c7ee4ec 100644
> --- a/include/linux/netfilter/nf_conntrack_tcp.h
> +++ b/include/linux/netfilter/nf_conntrack_tcp.h
> @@ -39,7 +39,7 @@ enum tcp_conntrack {
>  /* The field td_maxack has been set */
>  #define IP_CT_TCP_FLAG_MAXACK_SET		0x20
>  
> -struct nf_ct_tcp_flags {
> +struct __attribute__ ((packed)) nf_ct_tcp_flags {
>  	__u8 flags;
>  	__u8 mask;
>  };
> -- 
> 1.6.0.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Dr.-Ing. Lutz Jänicke
CTO
Innominate Security Technologies AG  /protecting industrial networks/
tel: +49.30.921028-200
fax: +49.30.921028-020
Rudower Chaussee 13
D-12489 Berlin, Germany
www.innominate.com

Register Court: AG Charlottenburg, HR B 81603
Management Board: Dirk Seewald
Chairman of the Supervisory Board: Christoph Leifer
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Pablo Neira Ayuso]

On Fri, Dec 30, 2011 at 12:43:39PM +0100, Lutz Jaenicke wrote:
> Against which tree do I have to re-submit this patch to get it merged?

You kidding me ;-), this was discussed more than one year ago.

http://markmail.org/message/afhn66qzyebyf7cs#query:+page:1+mid:s3ji4qj74vkovbxi+state:results

The fix was agreed to be done in user-space.

I have pushed the following patch for libnetfilter_conntrack:

http://git.netfilter.org/cgi-bin/gitweb.cgi?p=libnetfilter_conntrack.git;a=commitdiff;h=187c8099704d4616de9479104ba6560c03f90c39;hp=3c5b8711a83caad7d6e4525a823002cac0f593a4

Please, confirm that it fixes the problem.

Thank you.

[PATCH 0/1] Adjust nfnetlink structure size between kernel and user space

[Lutz Jaenicke]

Revision 2:
Move __attribute__ ((packed)) to the end of the structure definition as
used in other locations allowing for simpler grep etc.

The attached patch adjust the size of struct nf_ct_tcp_flags in the kernel
to match the hardcoded size in the respective user space tools.
This issue has so far only been visible for an Intel IXP4xx network
processor (ARM big endian, gcc-4.2.4) while the same code and
toolchain works fine on powerpc (32bit) and x86_64.

Lutz Jaenicke (1):
  netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

 include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

[PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Lutz Jaenicke]

On some platforms (here: Intel IXP4xx ARM big endian)
sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
space code 2 bytes is hard coded.
Add "__attribute__ ((__packed__))" to enforce matching structure
sizes.

Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
---
 include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/netfilter/nf_conntrack_tcp.h b/include/linux/netfilter/nf_conntrack_tcp.h
index 6e135f9..9d4cf26 100644
--- a/include/linux/netfilter/nf_conntrack_tcp.h
+++ b/include/linux/netfilter/nf_conntrack_tcp.h
@@ -42,7 +42,7 @@ enum tcp_conntrack {
 struct nf_ct_tcp_flags {
 	__u8 flags;
 	__u8 mask;
-};
+} __attribute__ ((packed));
 
 #ifdef __KERNEL__
 
-- 
1.6.0.3

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Patrick McHardy]

Lutz Jaenicke wrote:
> On some platforms (here: Intel IXP4xx ARM big endian)
> sizeof(struct nf_ct_tcp_flags) evaluates to 4 bytes while in the user
> space code 2 bytes is hard coded.
> Add "__attribute__ ((__packed__))" to enforce matching structure
> sizes.
> 
> Signed-off-by: Lutz Jaenicke <ljaenicke@innominate.com>
> ---
>  include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/netfilter/nf_conntrack_tcp.h b/include/linux/netfilter/nf_conntrack_tcp.h
> index 6e135f9..9d4cf26 100644
> --- a/include/linux/netfilter/nf_conntrack_tcp.h
> +++ b/include/linux/netfilter/nf_conntrack_tcp.h
> @@ -42,7 +42,7 @@ enum tcp_conntrack {
>  struct nf_ct_tcp_flags {
>  	__u8 flags;
>  	__u8 mask;
> -};
> +} __attribute__ ((packed));

I would prefer to fix userspace to use sizeof(...) instead of
using packed structs in the kernel. AFAICT, that should also
work on all architectures.

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[Jan Engelhardt]

On Monday 2010-03-15 17:57, Patrick McHardy wrote:
>Lutz Jaenicke wrote:
>> ---
>>  include/linux/netfilter/nf_conntrack_tcp.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/include/linux/netfilter/nf_conntrack_tcp.h b/include/linux/netfilter/nf_conntrack_tcp.h
>> index 6e135f9..9d4cf26 100644
>> --- a/include/linux/netfilter/nf_conntrack_tcp.h
>> +++ b/include/linux/netfilter/nf_conntrack_tcp.h
>> @@ -42,7 +42,7 @@ enum tcp_conntrack {
>>  struct nf_ct_tcp_flags {
>>  	__u8 flags;
>>  	__u8 mask;
>> -};
>> +} __attribute__ ((packed));
>
>I would prefer to fix userspace to use sizeof(...) instead of
>using packed structs in the kernel. AFAICT, that should also
>work on all architectures.

Yeah that's what I would have preferred too.

Re: [PATCH 1/1] netfilter: nf_ct_tcp: pack nf_ct_tcp_flags to match user space

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 15 Mar 2010 17:57:32 +0100

> I would prefer to fix userspace to use sizeof(...) instead of
> using packed structs in the kernel. AFAICT, that should also
> work on all architectures.

Agreed.