Commit 085219f79cad broke Sparc-32 back in 2.6.28.

Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[Rob Landley]

On Saturday 20 February 2010 17:12:22 Rob Landley wrote:
> On Saturday 20 February 2010 15:59:31 Blue Swirl wrote:
> > > I've got 2.6.32 booting to a command prompt (albeit with serial console
> > > and intentionall restricted set of hardware).  But then it misbehaves.
> > >
> > >  I'll try getting 2.6.18 to build with a known .config, and then bisect
> > > forward if that seems to work...
> >
> > Good plan. Bisecting backwards could be interesting too, to find out
> > which releases are actually working out of the box.
>
> I started by iterating through the release versions.  It's working up
> through 2.6.28, then 2.6.29 has the out of memory error in my init script.
>
> Bisecting now...
>
> Rob

And the commit that broke it bisects to:

085219f79cad89291699bd2bfb21c9fdabafe65f is first bad commit
commit 085219f79cad89291699bd2bfb21c9fdabafe65f
Author: Sam Ravnborg <sam@ravnborg.org>
Date:   Fri Jan 2 18:47:34 2009 -0800

    sparc32: use proper types in struct stat
    
    Like sparc64 use proper types in struct stat
    
    Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

This commit breaks stat and makes sparc32 essentially unusable.  It changes 
the size of the various types in stat.h, and means that if you "mount -t tmpfs 
/tmp /tmp" and then try to ls /tmp, ls dies with a memory allocation error.

I've confirmed that reverting it fixes the problem.

Looking at the actual diff, here's the hunk that causes problems:

--- a/arch/sparc/include/asm/stat_32.h
+++ b/arch/sparc/include/asm/stat_32.h
        short           st_nlink;
-       unsigned short  st_uid;
-       unsigned short  st_gid;
+       uid_t           st_uid;
+       gid_t           st_gid;

The symptom (in my uClibc+busybox root filesystem) is:

/ # mount -t tmpfs /tmp /tmp
/ # ls -l /tmp
ls: can't open '/tmp': Cannot allocate memory
total 0

The problem is that both uid_t and gid_t are "int" instead of "short".  This 
patch changes the size of those types.  (I note that this is apparently a 
known issue, there's __compat_uid_t and friends in the sparc asm directory...)

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[David Miller]

From: Rob Landley <rob@landley.net>
Date: Sun, 21 Feb 2010 10:25:09 -0600

> 085219f79cad89291699bd2bfb21c9fdabafe65f is first bad commit
> commit 085219f79cad89291699bd2bfb21c9fdabafe65f
> Author: Sam Ravnborg <sam@ravnborg.org>
> Date:   Fri Jan 2 18:47:34 2009 -0800
> 
>     sparc32: use proper types in struct stat
>     
>     Like sparc64 use proper types in struct stat
>     
>     Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> This commit breaks stat and makes sparc32 essentially unusable.  It changes 
> the size of the various types in stat.h, and means that if you "mount -t tmpfs 
> /tmp /tmp" and then try to ls /tmp, ls dies with a memory allocation error.
> 
> I've confirmed that reverting it fixes the problem.

Thanks for tracking this down Rob, I'll work on a fix and
push it around.

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[Bartlomiej Zolnierkiewicz]

On Monday 22 February 2010 12:57:19 am David Miller wrote:
> From: Rob Landley <rob@landley.net>
> Date: Sun, 21 Feb 2010 10:25:09 -0600
> 
> > 085219f79cad89291699bd2bfb21c9fdabafe65f is first bad commit
> > commit 085219f79cad89291699bd2bfb21c9fdabafe65f
> > Author: Sam Ravnborg <sam@ravnborg.org>
> > Date:   Fri Jan 2 18:47:34 2009 -0800
> > 
> >     sparc32: use proper types in struct stat
> >     
> >     Like sparc64 use proper types in struct stat
> >     
> >     Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > This commit breaks stat and makes sparc32 essentially unusable.  It changes 
> > the size of the various types in stat.h, and means that if you "mount -t tmpfs 
> > /tmp /tmp" and then try to ls /tmp, ls dies with a memory allocation error.
> > 
> > I've confirmed that reverting it fixes the problem.
> 
> Thanks for tracking this down Rob, I'll work on a fix and
> push it around.

Looking at how whole sparc32 has been apparently broken for over a year now
because of a purely cleanup patch I wonder if it would be appropriate to
make sparc32 into 'legacy only' and provide 'a stability promise' for it?

Just an idea.. ;)

--
Bartlomiej Zolnierkiewicz

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[Rob Landley]

On Sunday 21 February 2010 18:28:20 Bartlomiej Zolnierkiewicz wrote:
> On Monday 22 February 2010 12:57:19 am David Miller wrote:
> > From: Rob Landley <rob@landley.net>
> > Date: Sun, 21 Feb 2010 10:25:09 -0600
> >
> > > 085219f79cad89291699bd2bfb21c9fdabafe65f is first bad commit
> > > commit 085219f79cad89291699bd2bfb21c9fdabafe65f
> > > Author: Sam Ravnborg <sam@ravnborg.org>
> > > Date:   Fri Jan 2 18:47:34 2009 -0800
> > >
> > >     sparc32: use proper types in struct stat
> > >
> > >     Like sparc64 use proper types in struct stat
> > >
> > >     Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > >
> > > This commit breaks stat and makes sparc32 essentially unusable.  It
> > > changes the size of the various types in stat.h, and means that if you
> > > "mount -t tmpfs /tmp /tmp" and then try to ls /tmp, ls dies with a
> > > memory allocation error.
> > >
> > > I've confirmed that reverting it fixes the problem.
> >
> > Thanks for tracking this down Rob, I'll work on a fix and
> > push it around.
>
> Looking at how whole sparc32 has been apparently broken for over a year now
> because of a purely cleanup patch I wonder if it would be appropriate to
> make sparc32 into 'legacy only' and provide 'a stability promise' for it?
>
> Just an idea.. ;)

Actually, the problem is that lots of people seem to expect current kernels to 
be broken on non-x86 targets, so they keep using old versions.  (In the case 
of the debian release everybody kept pointing me to on "but it works fine!" 
grounds, a 2.6.18 kernel.)  Lots of them only upgrade once idiots like me have 
gone across the minefield and made it safe. :)

"Current is always broken so nobody uses current" != "nobody uses this 
platform".  More "sparc people use distros rather than building their own 
systems from source, and tend not to be aggressive about upgrading".

Back in 2007 arm was broken for me for two or three releases (according to my 
blog it broke in 2.6.20 and the patch that fixed it ( 
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=4454/1 ) was 
not yet in 2.6.22-rc7.  That doesn't mean arm isn't widely used, just that 
nobody with that hardware was seriously trying to use the current version of 
the kernel.

My Firmware LInux project is working on implementing automated regression 
testing under QEMU.  Once I've got a platform working (which sparc wasn't 
until now) I can provide much more prompt breakage reports in future, at least 
for the basic stuff like this...

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[David Miller]

Here's the fix I'll use, thanks for the report Rob:

sparc32: Fix struct stat uid/gid types.

Commit 085219f79cad89291699bd2bfb21c9fdabafe65f
("sparc32: use proper types in struct stat")

Accidently changed the struct stat uid/gid members
to uid_t and gid_t, but those get set to
__kernel_uid32_t and __kernel_gid32_t respectively.
Those are of type 'int' but the structure is meant
to have 'short'.  So use uid16_t and gid16_t to
correct this.

Reported-by: Rob Landley <rob@landley.net>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/include/asm/stat.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/sparc/include/asm/stat.h b/arch/sparc/include/asm/stat.h
index 55db5ec..39327d6 100644
--- a/arch/sparc/include/asm/stat.h
+++ b/arch/sparc/include/asm/stat.h
@@ -53,8 +53,8 @@ struct stat {
 	ino_t		st_ino;
 	mode_t		st_mode;
 	short		st_nlink;
-	uid_t		st_uid;
-	gid_t		st_gid;
+	uid16_t		st_uid;
+	gid16_t		st_gid;
 	unsigned short	st_rdev;
 	off_t		st_size;
 	time_t		st_atime;
-- 
1.6.6.1

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[Rob Landley]

On Sunday 21 February 2010 20:06:58 David Miller wrote:
> Here's the fix I'll use, thanks for the report Rob:
>
> sparc32: Fix struct stat uid/gid types.
>
> Commit 085219f79cad89291699bd2bfb21c9fdabafe65f
> ("sparc32: use proper types in struct stat")

Unfortunately, while this fix makes sparc buidl and run again, the exported 
kernel headers are horked and can't build strace natively.

gcc -DHAVE_CONFIG_H -I. -Ilinux/sparc -I./linux/sparc -Ilinux -I./linux   -
Wall --static -MT file.o -MD -MP -MF .deps/file.Tpo -c -o file.o file.c
In file included from file.c:88:
/usr/bin/../include/asm/stat.h:56: error: expected specifier-qualifier-list 
before 'uid16_t'
file.c: In function 'realprintstat':
file.c:951: warning: format '%lu' expects type 'long unsigned int', but 
argument 2 has type 'unsigned int'
make[1]: *** [file.o] Error 1
make[1]: Leaving directory `/home/strace-4.5.19'
make: *** [all] Error 2

The problem is that uid16_t is a kernel internal type that gets cleaned out of 
the headers when they're exported, thus there's no definition for userspace to 
pick up if that structure is ever used from a userspace build.

What exactly was the problem with just saying "unsigned short" when you mean 
an unsigned short?  The way x86 does, and arm?  (If these ever change, it 
breaks binary compatability.  Not sure what these changes were trying to 
accomplish...)

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[David Miller]

From: Rob Landley <rob@landley.net>
Date: Fri, 26 Mar 2010 22:35:47 -0500

> What exactly was the problem with just saying "unsigned short" when you mean 
> an unsigned short?  The way x86 does, and arm?  (If these ever change, it 
> breaks binary compatability.  Not sure what these changes were trying to 
> accomplish...)

I was trying to use well defined types that described the
usage and the origin of the definition.

I'm happy to use "unsigned short" or whatever works better.
Please send a patch.

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[Rob Landley]

On Friday 26 March 2010 22:37:45 David Miller wrote:
> From: Rob Landley <rob@landley.net>
> Date: Fri, 26 Mar 2010 22:35:47 -0500
>
> > What exactly was the problem with just saying "unsigned short" when you
> > mean an unsigned short?  The way x86 does, and arm?  (If these ever
> > change, it breaks binary compatability.  Not sure what these changes were
> > trying to accomplish...)
>
> I was trying to use well defined types that described the
> usage and the origin of the definition.
>
> I'm happy to use "unsigned short" or whatever works better.
> Please send a patch.

Sure thing:

--- a/arch/sparc/include/asm/stat.h
+++ b/arch/sparc/include/asm/stat.h
@@ -53,8 +53,8 @@ struct stat {
 	ino_t		st_ino;
 	mode_t		st_mode;
 	short		st_nlink;
-	uid16_t		st_uid;
-	gid16_t		st_gid;
+	unsigned short	st_uid;
+	unsigned short	st_gid;
 	unsigned short	st_rdev;
 	off_t		st_size;
 	time_t		st_atime;

Signed-off-by: Rob Landley <rob@landley.net>

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds

Re: Commit 085219f79cad broke Sparc-32 back in 2.6.28.

[David Miller]

From: Rob Landley <rob@landley.net>
Date: Sat, 27 Mar 2010 02:44:16 -0500

> On Friday 26 March 2010 22:37:45 David Miller wrote:
>> From: Rob Landley <rob@landley.net>
>> Date: Fri, 26 Mar 2010 22:35:47 -0500
>>
>> > What exactly was the problem with just saying "unsigned short" when you
>> > mean an unsigned short?  The way x86 does, and arm?  (If these ever
>> > change, it breaks binary compatability.  Not sure what these changes were
>> > trying to accomplish...)
>>
>> I was trying to use well defined types that described the
>> usage and the origin of the definition.
>>
>> I'm happy to use "unsigned short" or whatever works better.
>> Please send a patch.
> 
> Sure thing:

Thanks a lot Rob, applied.

Please supply a complete commit log message and place your signoff
right before (instead of after) the patch next time, thanks!