compat_xt_counters is not compatible

compat_xt_counters is not compatible

[Andreas Schwab]

struct compat_xt_counters uses uint32_t[4], which has 4 byte alignment,
but userspace uses uint64_t[2], which has 8 byte alignment.  This breaks
iptables in 2.6.17-rc1.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: compat_xt_counters is not compatible

[Dmitry Mishin]

compat_xt_counters should be used only for 32bit processes, which has 4 byte 
alignment.

On Friday 14 April 2006 21:12, Andreas Schwab wrote:
> struct compat_xt_counters uses uint32_t[4], which has 4 byte alignment,
> but userspace uses uint64_t[2], which has 8 byte alignment.  This breaks
> iptables in 2.6.17-rc1.
>
> Andreas.

-- 
Thanks,
Dmitry.

Re: compat_xt_counters is not compatible

[Andreas Schwab]

Dmitry Mishin <dim@sw.ru> writes:

> compat_xt_counters should be used only for 32bit processes, which has 4 byte 
> alignment.

This is wrong.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: compat_xt_counters is not compatible

[Dmitry Mishin]

Could you give more details on what's wrong?

dim@dhcp0-24:~> cat test.c
#include <linux/types.h>

struct x
{
        __u64 ptr;
        __u32 ptr2;
};

int main(int argc, char *argv[])
{
        printf("%d\n", sizeof(struct x));
        return 0;
}
dim@dhcp0-24:~>  gcc -m32 -o test test.c
dim@dhcp0-24:~> ./test 
12
dim@dhcp0-24:~> gcc -o test test.c
dim@dhcp0-24:~> ./test 
16
dim@dhcp0-24:~> uname -p
x86_64

On Saturday 15 April 2006 18:16, Andreas Schwab wrote:
> Dmitry Mishin <dim@sw.ru> writes:
> > compat_xt_counters should be used only for 32bit processes, which has 4
> > byte alignment.
>
> This is wrong.
>
> Andreas.

-- 
Thanks,
Dmitry.

Re: compat_xt_counters is not compatible

[Andreas Schwab]

Dmitry Mishin <dim@sw.ru> writes:

> Could you give more details on what's wrong?

Your claim that long long has 4 byte alignment.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: compat_xt_counters is not compatible

[Dmitry Mishin]

I believes, that on 32 bit arch any alignment <= 4 bytes. Could you point out 
the cases, where it is not true?

On Saturday 15 April 2006 21:32, Andreas Schwab wrote:
> Dmitry Mishin <dim@sw.ru> writes:
> > Could you give more details on what's wrong?
>
> Your claim that long long has 4 byte alignment.
>
> Andreas.

-- 
Thanks,
Dmitry.

Re: compat_xt_counters is not compatible

[Andreas Schwab]

Dmitry Mishin <dim@sw.ru> writes:

> I believes, that on 32 bit arch any alignment <= 4 bytes.

Why do you think so?

> Could you point out the cases, where it is not true?

Almost every architecture has alignment = size for basic types.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: compat_xt_counters is not compatible

[Dmitry Mishin]

>
> Almost every architecture has alignment = size for basic types.
>
> Andreas.
I mean structures. I don't see a bug still. Can you point out wrong code?

-- 
Thanks,
Dmitry.

Re: compat_xt_counters is not compatible

[Andreas Schwab]

Dmitry Mishin <dim@sw.ru> writes:

>>
>> Almost every architecture has alignment = size for basic types.
>>
>> Andreas.
> I mean structures.

So do I.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: compat_xt_counters is not compatible

[David S. Miller]

From: Dmitry Mishin <dim@sw.ru>
Date: Sun, 16 Apr 2006 10:28:38 +0400

> >
> > Almost every architecture has alignment = size for basic types.
> >
> > Andreas.
> I mean structures. I don't see a bug still. Can you point out wrong code?

I can't believe such a conversation even gets this far. :(

LOOK!

struct {
	int x;
	unsigned long long y;
};

This structure requires 64-bit alignment on 32-bit Sparc and
32-bit PPC and 32-bit MIPS and 32-bit ....

It is x86, and only x86, that has only a 32-bit alignment requirement
for this structure.

Re: compat_xt_counters is not compatible

[Patrick McHardy]

Andreas Schwab wrote:
> struct compat_xt_counters uses uint32_t[4], which has 4 byte alignment,
> but userspace uses uint64_t[2], which has 8 byte alignment.  This breaks
> iptables in 2.6.17-rc1.

Did you actually see the breakage? gcc on x86 uses 8 byte alignment for
u_int64_t, but 4 byte alignment for structures with embedded u_int64_t
members. It appears to work just fine on my x86_64.

Re: compat_xt_counters is not compatible

[Andreas Schwab]

Patrick McHardy <kaber@trash.net> writes:

> Andreas Schwab wrote:
>> struct compat_xt_counters uses uint32_t[4], which has 4 byte alignment,
>> but userspace uses uint64_t[2], which has 8 byte alignment.  This breaks
>> iptables in 2.6.17-rc1.
>
> Did you actually see the breakage?

Of course.

> gcc on x86

Not all the worlds an x86.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: compat_xt_counters is not compatible

[Patrick McHardy]

Andreas Schwab wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
> 
>>Andreas Schwab wrote:
>>
>>>struct compat_xt_counters uses uint32_t[4], which has 4 byte alignment,
>>>but userspace uses uint64_t[2], which has 8 byte alignment.  This breaks
>>>iptables in 2.6.17-rc1.
>>
>>Did you actually see the breakage?
> 
> 
> Of course.
> 
> 
>>gcc on x86
> 
> 
> Not all the worlds an x86.

Well, then how about providing the necessary information, starting
with the architecture?

Re: compat_xt_counters is not compatible

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sat, 15 Apr 2006 19:52:01 +0200

> Well, then how about providing the necessary information, starting
> with the architecture?

A struct containing a u64 has to be 64-bit aligned on at least sparc64
and ppc64, both of which use the compat layer for 32-bit programs, so
I think it's quite clear this will have to be implemented differently.

Re: compat_xt_counters is not compatible

[Patrick McHardy]

David S. Miller wrote:
> A struct containing a u64 has to be 64-bit aligned on at least sparc64
> and ppc64, both of which use the compat layer for 32-bit programs, so
> I think it's quite clear this will have to be implemented differently.

Do these alignment requirements also hold for 32bit compiled programms?
This is what we need to match with compat_xt_counters. Or in other
words: we need a type that when compiled for 64bit has the same
alignment requirements as structures with embedded u_int64_t members
compiled for 32bit on all architectures supporting CONFIG_COMPAT.
I don't think such a type exists, so I guess we'll have to use different
types depending on the architecture.

I'll send a patch to do that if there are no better suggestions.

Re: compat_xt_counters is not compatible

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Sun, 16 Apr 2006 17:07:26 +0200

> David S. Miller wrote:
> > A struct containing a u64 has to be 64-bit aligned on at least sparc64
> > and ppc64, both of which use the compat layer for 32-bit programs, so
> > I think it's quite clear this will have to be implemented differently.
> 
> Do these alignment requirements also hold for 32bit compiled programms?

Yes, it does.

> This is what we need to match with compat_xt_counters.

This issue is exactly why the compat layer requirements are so wildly
different between "x86_64/ia64 --> ia32" and all other compat
situations.

ia32 doesn't have the 64-bit alignment requirement for structures
containing u64 objects, whereas every other 32-bit platform does.

[RFC][PATCH] compat_uint64_t type

[Dmitry Mishin]

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

This patch introduces compat_uint64_t type, which is required for proper 
handling different alignment of structures, containing u64 objects, on 
compatible with ia32 64bit platforms and others.

Signed-off-by: Dmitry Mishin <dim@openvz.org>
Acked-of-by: Kirill Korotaev <dev@openvz.org>
 
On Monday 17 April 2006 09:24, David S. Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Sun, 16 Apr 2006 17:07:26 +0200
>
> > David S. Miller wrote:
> > > A struct containing a u64 has to be 64-bit aligned on at least sparc64
> > > and ppc64, both of which use the compat layer for 32-bit programs, so
> > > I think it's quite clear this will have to be implemented differently.
> >
> > Do these alignment requirements also hold for 32bit compiled programms?
>
> Yes, it does.
>
> > This is what we need to match with compat_xt_counters.
>
> This issue is exactly why the compat layer requirements are so wildly
> different between "x86_64/ia64 --> ia32" and all other compat
> situations.
>
> ia32 doesn't have the 64-bit alignment requirement for structures
> containing u64 objects, whereas every other 32-bit platform does.

-- 
Thanks,
Dmitry.

[-- Attachment #2: diff-ms-net-netfilter-compat-20060417 --]
[-- Type: text/plain, Size: 3028 bytes --]

diff --git a/include/asm-ia64/compat.h b/include/asm-ia64/compat.h
index 40d01d8..3e6417b 100644
--- a/include/asm-ia64/compat.h
+++ b/include/asm-ia64/compat.h
@@ -34,6 +34,8 @@ typedef s32		compat_long_t;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
 
+typedef u64 __attribute__((aligned(4)))	compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t	tv_sec;
 	s32		tv_nsec;
diff --git a/include/asm-mips/compat.h b/include/asm-mips/compat.h
index 986511d..f95640f 100644
--- a/include/asm-mips/compat.h
+++ b/include/asm-mips/compat.h
@@ -38,6 +38,8 @@ typedef s32		compat_long_t;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
 
+typedef u64		compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t	tv_sec;
 	s32		tv_nsec;
diff --git a/include/asm-parisc/compat.h b/include/asm-parisc/compat.h
index 289624d..312829d 100644
--- a/include/asm-parisc/compat.h
+++ b/include/asm-parisc/compat.h
@@ -33,6 +33,8 @@ typedef s32	compat_long_t;
 typedef u32	compat_uint_t;
 typedef u32	compat_ulong_t;
 
+typedef u64	compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t		tv_sec;
 	s32			tv_nsec;
diff --git a/include/asm-powerpc/compat.h b/include/asm-powerpc/compat.h
index aacaabd..e90fa22 100644
--- a/include/asm-powerpc/compat.h
+++ b/include/asm-powerpc/compat.h
@@ -36,6 +36,8 @@ typedef s32		compat_long_t;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
 
+typedef u64		compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t	tv_sec;
 	s32		tv_nsec;
diff --git a/include/asm-s390/compat.h b/include/asm-s390/compat.h
index 356a0b1..1b8c7f7 100644
--- a/include/asm-s390/compat.h
+++ b/include/asm-s390/compat.h
@@ -35,6 +35,8 @@ typedef s32		compat_long_t;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
 
+typedef u64		compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t	tv_sec;
 	s32		tv_nsec;
diff --git a/include/asm-sparc64/compat.h b/include/asm-sparc64/compat.h
index c73935d..6adc10c 100644
--- a/include/asm-sparc64/compat.h
+++ b/include/asm-sparc64/compat.h
@@ -34,6 +34,8 @@ typedef s32		compat_long_t;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
 
+typedef u64		compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t	tv_sec;
 	s32		tv_nsec;
diff --git a/include/asm-x86_64/compat.h b/include/asm-x86_64/compat.h
index b37ab82..4d7eb9c 100644
--- a/include/asm-x86_64/compat.h
+++ b/include/asm-x86_64/compat.h
@@ -36,6 +36,8 @@ typedef s32		compat_long_t;
 typedef u32		compat_uint_t;
 typedef u32		compat_ulong_t;
 
+typedef u64 __attribute__((aligned(4)))	compat_uint64_t;
+
 struct compat_timespec {
 	compat_time_t	tv_sec;
 	s32		tv_nsec;
diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index f6bdef8..558125c 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -361,7 +361,7 @@ struct compat_xt_entry_target
 
 struct compat_xt_counters
 {
-	u_int32_t cnt[4];
+	compat_uint64_t pcnt, bcnt;
 };
 
 struct compat_xt_counters_info

Re: [RFC][PATCH] compat_uint64_t type

[Andreas Schwab]

Dmitry Mishin <dim@openvz.org> writes:

> diff --git a/include/asm-ia64/compat.h b/include/asm-ia64/compat.h
> index 40d01d8..3e6417b 100644
> --- a/include/asm-ia64/compat.h
> +++ b/include/asm-ia64/compat.h
> @@ -34,6 +34,8 @@ typedef s32		compat_long_t;
>  typedef u32		compat_uint_t;
>  typedef u32		compat_ulong_t;
>  
> +typedef u64 __attribute__((aligned(4)))	compat_uint64_t;

The aligned attribute cannot decrease alignment.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC][PATCH] compat_uint64_t type

[Dmitry Mishin]

On Monday 17 April 2006 15:51, Andreas Schwab wrote:
> Dmitry Mishin <dim@openvz.org> writes:
> > diff --git a/include/asm-ia64/compat.h b/include/asm-ia64/compat.h
> > index 40d01d8..3e6417b 100644
> > --- a/include/asm-ia64/compat.h
> > +++ b/include/asm-ia64/compat.h
> > @@ -34,6 +34,8 @@ typedef s32		compat_long_t;
> >  typedef u32		compat_uint_t;
> >  typedef u32		compat_ulong_t;
> >
> > +typedef u64 __attribute__((aligned(4)))	compat_uint64_t;
>
> The aligned attribute cannot decrease alignment.
With my gcc (3.3.5) it can. Which solution do you propose?
__attribute__((packed, aligned(4)))?
Or 
#ifdef  ARCH_HAS_FUNNY_ALIGNMENT
struct compat_xt_counters {
	uint32_t cnt[4];
}
#else
#define compat_xt_counters xt_counters
#endif
?

>
> Andreas.

-- 
Thanks,
Dmitry.

Re: [RFC][PATCH] compat_uint64_t type

[Andreas Schwab]

Dmitry Mishin <dim@openvz.org> writes:

> On Monday 17 April 2006 15:51, Andreas Schwab wrote:
>> Dmitry Mishin <dim@openvz.org> writes:
>> > diff --git a/include/asm-ia64/compat.h b/include/asm-ia64/compat.h
>> > index 40d01d8..3e6417b 100644
>> > --- a/include/asm-ia64/compat.h
>> > +++ b/include/asm-ia64/compat.h
>> > @@ -34,6 +34,8 @@ typedef s32		compat_long_t;
>> >  typedef u32		compat_uint_t;
>> >  typedef u32		compat_ulong_t;
>> >
>> > +typedef u64 __attribute__((aligned(4)))	compat_uint64_t;
>>
>> The aligned attribute cannot decrease alignment.
> With my gcc (3.3.5) it can.

You're right, this is a special case (in general it can't without the
packed attribute).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC][PATCH] compat_uint64_t type

[Patrick McHardy]

Dmitry Mishin wrote:
> This patch introduces compat_uint64_t type, which is required for proper 
> handling different alignment of structures, containing u64 objects, on 
> compatible with ia32 64bit platforms and others.
> 

> diff --git a/include/asm-ia64/compat.h b/include/asm-ia64/compat.h
> index 40d01d8..3e6417b 100644
> --- a/include/asm-ia64/compat.h
> +++ b/include/asm-ia64/compat.h
> @@ -34,6 +34,8 @@ typedef s32		compat_long_t;
>  typedef u32		compat_uint_t;
>  typedef u32		compat_ulong_t;
>  
> +typedef u64 __attribute__((aligned(4)))	compat_uint64_t;
> +
>  struct compat_timespec {
>  	compat_time_t	tv_sec;
>  	s32		tv_nsec;

I'm not sure if we should really introduce this new type, it is
misleading for at least x86. IIRC the situation there is:

- gcc < 4.0 always aligns u64 types to 4byte
- gcc >= 4.0 aligns basic u64 types to 8byte, u64's embedded in
  structures to 4 byte

So there is no one compat_uint64_t type, but it depends on the
situation.

Re: [RFC][PATCH] compat_uint64_t type

[Patrick McHardy]

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

Patrick McHardy wrote:
> Dmitry Mishin wrote:
> 
>>This patch introduces compat_uint64_t type, which is required for proper 
>>handling different alignment of structures, containing u64 objects, on 
>>compatible with ia32 64bit platforms and others.

This is my proposed alternative. It assumes all architectures besides
x86 use native alignment for basic types.


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

[NETFILTER]: Fix compat_xt_counters alignment for non-x86

Some (?) non-x86 architectures require 8byte alignment for u_int64_t
even when compiled for 32bit, using u_int32_t in compat_xt_counters
breaks on these architectures, use u_int64_t for everything but x86.

Reported by Andreas Schwab <schwab@suse.de>.

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

---
commit 2666afe70fb68b9b884f6d3fa7e4236183818028
tree 65eac95933d59a455853245f7442cbb38dc9f19c
parent 2ceefa038e908d5da21aefedae02da4eab1b2787
author Patrick McHardy <kaber@trash.net> Mon, 17 Apr 2006 18:04:37 +0200
committer Patrick McHardy <kaber@trash.net> Mon, 17 Apr 2006 18:04:37 +0200

 include/linux/netfilter/x_tables.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index f6bdef8..3870145 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -361,7 +361,11 @@ struct compat_xt_entry_target
 
 struct compat_xt_counters
 {
+#if defined(CONFIG_X86_64) || defined(CONFIG_IA64)
 	u_int32_t cnt[4];
+#else
+	u_int64_t cnt[2];
+#endif
 };
 
 struct compat_xt_counters_info

Re: [RFC][PATCH] compat_uint64_t type

[David S. Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 17 Apr 2006 18:03:39 +0200

> Patrick McHardy wrote:
> > Dmitry Mishin wrote:
> > 
> >>This patch introduces compat_uint64_t type, which is required for proper 
> >>handling different alignment of structures, containing u64 objects, on 
> >>compatible with ia32 64bit platforms and others.
> 
> This is my proposed alternative. It assumes all architectures besides
> x86 use native alignment for basic types.

Ok.  Perhaps we can eventually define a CONFIG_* object for
this.

Re: [RFC][PATCH] compat_uint64_t type

[Patrick McHardy]

David S. Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon, 17 Apr 2006 18:03:39 +0200
> 
>>This is my proposed alternative. It assumes all architectures besides
>>x86 use native alignment for basic types.
> 
> 
> Ok.  Perhaps we can eventually define a CONFIG_* object for
> this.

Since this appears to be a pure x86 32 bit emulation oddity
so far, I think CONFIG_IA32_EMULATION would express that
best. Unfortunately IA64 uses a different config option,
so I left it as is.

I'll send the patch with a batch of other fixes in a few hours.

Re: [RFC][PATCH] compat_uint64_t type

[Patrick McHardy]

Andreas Schwab wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>>I'm not sure if we should really introduce this new type, it is
>>misleading for at least x86. IIRC the situation there is:
>>
>>- gcc < 4.0 always aligns u64 types to 4byte
> 
> 
> Are you sure about that? I can't reproduce that with gcc 3.3.3.

I mixed up the versions, the change was between 2.x and 3.x:

gcc-2.95 -O2:
basic type:     align 4
embedded type:  align 4
gcc-3.3 -O2:
basic type:     align 8
embedded type:  align 4

It only shows with optimization, with -O0 gcc-2.95 also uses 8 for
the basic type.

>>So there is no one compat_uint64_t type, but it depends on the
>>situation.
> 
> 
> The alignment of standalone objects is uninteresting, what is important is
> getting compatible layout of aggregates.

I couldn't come up with a realistic example where people would make
mistakes because of this, so I guess I have to agree, although my
feeling still disagrees :)