Patchwork sparc32: unaligned memory access (MNA) trap handler bug

login
register
mail settings
Submitter Daniel Hellstrom
Date Feb. 1, 2011, 6:03 p.m.
Message ID <1296583423-5469-1-git-send-email-daniel@gaisler.com>
Download mbox | patch
Permalink /patch/81343/
State Accepted
Delegated to: David Miller
Headers show

Comments

Daniel Hellstrom - Feb. 1, 2011, 6:03 p.m.
Since the merge process of the sparc and sparc64 the sparc32
MNA trap handler does not emulate stores to unaligned addresses
correctly. MNA operation from both from kernel and user space
are affected.

A typical effect of this bug is nr_frags in skbs are overwritten
during buffer copying/checksum-calculation, or maximally 6 bytes
of data in the network buffer will be overwitten with garbage.

Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
---
 arch/sparc/kernel/una_asm_32.S |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Sam Ravnborg - Feb. 1, 2011, 7:59 p.m.
On Tue, Feb 01, 2011 at 07:03:43PM +0100, Daniel Hellstrom wrote:
> Since the merge process of the sparc and sparc64 the sparc32
> MNA trap handler does not emulate stores to unaligned addresses
> correctly. MNA operation from both from kernel and user space
> are affected.

Well spotted!

This bug was actually introduced by:
f0e98c387e61de00646be31fab4c2fa0224e1efb "[SPARC]: Fix link errors with gcc-4.3"

I wanted to check if there were similar bugs introduced, but
this looks like a solo incident.

I grepped for "%1" in the other *.S files, no hits.
[Likewise for "%2", "%3", "%4"]

Patch looks good to me.
Reluctant with an "Acked-by" only because I do not feel confident
with the code in question. The change looks obviously correct.

I noticed you copied Greg on this. I guess you did so because you
consider this -stable material.

A better way to do so is to add:

Cc: <stable@kernel.org>

Then the stable team(s) will all be notified when this patch is applied
by Linus to mainline.

This is also a strong hint for David to apply this to sparc-2.6.git and not
sparc-next-2.6.git
But being explicit is always preferred.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Feb. 1, 2011, 8:37 p.m.
From: Sam Ravnborg <sam@ravnborg.org>
Date: Tue, 1 Feb 2011 20:59:26 +0100

> On Tue, Feb 01, 2011 at 07:03:43PM +0100, Daniel Hellstrom wrote:
>> Since the merge process of the sparc and sparc64 the sparc32
>> MNA trap handler does not emulate stores to unaligned addresses
>> correctly. MNA operation from both from kernel and user space
>> are affected.
> 
> Well spotted!

Indeed.

> This bug was actually introduced by:
> f0e98c387e61de00646be31fab4c2fa0224e1efb "[SPARC]: Fix link errors with gcc-4.3"

I'll make a note of this in the commit message.

> A better way to do so is to add:
> 
> Cc: <stable@kernel.org>
> 
> Then the stable team(s) will all be notified when this patch is applied
> by Linus to mainline.

I'll take care of the -stable submission.

I wish binutils wouldn't accept %1 as a register, nobody sane uses that
format for register specifications and when it does happen (like here)
it's a typo and a bug.

Thanks guys, I'll apply this and queue it up for -stable!
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Hellstrom - Feb. 2, 2011, 9:34 a.m.
David Miller wrote:

>From: Sam Ravnborg <sam@ravnborg.org>
>Date: Tue, 1 Feb 2011 20:59:26 +0100
>
>  
>
>>On Tue, Feb 01, 2011 at 07:03:43PM +0100, Daniel Hellstrom wrote:
>>    
>>
>>>Since the merge process of the sparc and sparc64 the sparc32
>>>MNA trap handler does not emulate stores to unaligned addresses
>>>correctly. MNA operation from both from kernel and user space
>>>are affected.
>>>      
>>>
>>Well spotted!
>>    
>>
>
>Indeed.
>  
>
Thanks, we have seen this problem pop up now and then since nov/dec but 
unable to trigger it stable. Debugging such a problem often requires a 
quite long time and since I can not work fulltime with Linux it took 
some time before I could fully focus on it. Of course at first I thought 
if was a LEON problem, then a GRETH network driver problem, then SMP 
related... Last week I spent a lot time tracing, and thanks to a great 
HW team here at Gaisler that made me a system with better hardware 
debugging support and the excellent GRMON I was able to find it at last.

I really hope that the sparc community can take benefit of the time we 
invested in this. Now that we are more synced with the official kernel 
and hopefully SMP will work better, I think it will be much easier for 
us to spot and fix generic problems as they appear in the future. Thanks!

>  
>
>>This bug was actually introduced by:
>>f0e98c387e61de00646be31fab4c2fa0224e1efb "[SPARC]: Fix link errors with gcc-4.3"
>>    
>>
>
>I'll make a note of this in the commit message.
>
>  
>
>>A better way to do so is to add:
>>
>>Cc: <stable@kernel.org>
>>
>>Then the stable team(s) will all be notified when this patch is applied
>>by Linus to mainline.
>>    
>>
>
>I'll take care of the -stable submission.
>  
>
Good, thanks, that was my intention but didn't know there was a 
procedure like this. Will stick to that in the future if more problems 
like this arise.

>I wish binutils wouldn't accept %1 as a register, nobody sane uses that
>format for register specifications and when it does happen (like here)
>it's a typo and a bug.
>  
>
I agree.

>Thanks guys, I'll apply this and queue it up for -stable!
>  
>
nice.

--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sam Ravnborg - Feb. 3, 2011, 4:58 p.m.
>
> I really hope that the sparc community can take benefit of the time we  
> invested in this.

Sometimes I have asked myself what is the size of the sparc32 community?
Personally I got more involved in sparc development because I wanted
to simplify the way we handled our headers and a natural step forward
was to get the remaining architectures merged where sparc & sparc64
was one of the outstanding architectures.
A responsive maintainer also helped a lot.

I have looked around for a decent distribution that claim up-to-date
support for sparc. But they all seem to be dropping sparc support.
And here sparc64 is in focus - not sparc32.

And I understand that sparc32 does not work well for many people.
I know that recent kernels only can run on SparcServer 1000 for
a few minutes before it locks hard. And I expect we see similar
problems on sun4m boxes with > 1 CPU.
The bug you have fixed (with great effort) in the unaligned
trap handler further confirm to me that that not many users
are running recent kernels on sparc32 hardware.
I also recall that I broke sparc32 during the unification,
which took months to e detected.

Personally I run sparc only to test my kernel contributions,
as the hw I have available is too noisy / too slow to
be used for anything else.

But IMHO the sparc boxes are quite nice and the sparc32 architecture
code does not deserve to bitrot.
So when Gaisler sponsored me a LEON4 evaluation board I decided to
concentrate my effort on sparc. I have had no benefit of the
evaluation board - yet. The current effort is to bring
the sparc32 stuff up snuff - and this will also benefit the
LEON support.
I expect one day that my contributions rasies above the
janitorial level...

It also looks better on the CV when I can say that I have refactored
the IRQ support on sparc compared to some random kbuild hacking :-)

So Gaislers contributions helps the sparc32 community.
And I expect that the combined effort will allow more people to
use a recent kernel on sparc32 and thus will increase the community.
But how big the community is - is an open question to me.

Note: If someone has - or knows about - a SS1000 that I can use
for Linux testing I am all ears. (I live in Scandinavia / Denmark)
Contact me in private as this in not relevant on the list.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
crn@pop3.netunix.com - Feb. 3, 2011, 5:27 p.m.
>
> Note: If someone has - or knows about - a SS1000 that I can use
> for Linux testing I am all ears. (I live in Scandinavia / Denmark)
> Contact me in private as this in not relevant on the list.
>

I have a SS1000E with 6 procs sitting in my garage in the SW UK.
Replying to the group because someone may be able to get it from
here (Weston Super Mare) to you.
They are heavy awkward beasties and I no longer have space to
work with it so free to a good home but shipping would be expensive.


--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sam Ravnborg - Feb. 3, 2011, 7:16 p.m.
On Thu, Feb 03, 2011 at 05:27:42PM -0000, crn@pop3.netunix.com wrote:
> >
> > Note: If someone has - or knows about - a SS1000 that I can use
> > for Linux testing I am all ears. (I live in Scandinavia / Denmark)
> > Contact me in private as this in not relevant on the list.
> >
> 
> I have a SS1000E with 6 procs sitting in my garage in the SW UK.
> Replying to the group because someone may be able to get it from
> here (Weston Super Mare) to you.
> They are heavy awkward beasties and I no longer have space to
> work with it so free to a good home but shipping would be expensive.

Hi Chris.

Thanks for the nice offer!
We will continue in private as this is off-topic for the list.

	Sam
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julian Calaby - Feb. 3, 2011, 10:28 p.m.
On Fri, Feb 4, 2011 at 03:58, Sam Ravnborg <sam@ravnborg.org> wrote:
>>
>> I really hope that the sparc community can take benefit of the time we
>> invested in this.
>
> Sometimes I have asked myself what is the size of the sparc32 community?

Raises hand =)

> Personally I got more involved in sparc development because I wanted
> to simplify the way we handled our headers and a natural step forward
> was to get the remaining architectures merged where sparc & sparc64
> was one of the outstanding architectures.

My motivation was that Debian dropped SPARC32 support and subsequently
broke my first SS10. (Linux doesn't run well when libc6 won't load)
Reading up on the lists indicated that their decision was due to there
being no active kernel contributors. My immediate thought was "That
sounds interesting, I could help with that", leading to the small
amount of work I've done so far.

> I have looked around for a decent distribution that claim up-to-date
> support for sparc. But they all seem to be dropping sparc support.
> And here sparc64 is in focus - not sparc32.

At the moment, the only way I've found to get sparc32 support with
modern packages involves installing Gentoo 2006.1 then upgrading it.
Annoyingly, I'm yet to complete an installation as I ran out of disk
space on my SSLX and both disks in my first SS10 died. I'm currently
trying to set up some form of NFS root environment for installing that
on them, but I'm running into issues involving shiny new hardware and
glacial ARM development. (Rant: people have patches to add full
support for the GoFlex Net, why haven't they been waved under the
noses of the relevant maintainers?)

I've never investigated sparc64 support as my Ultra 1e runs fine on
bleeding edge Debian.

> And I understand that sparc32 does not work well for many people.
> I know that recent kernels only can run on SparcServer 1000 for
> a few minutes before it locks hard. And I expect we see similar
> problems on sun4m boxes with > 1 CPU.

Both of my SS10s have > 1 CPU, so my focus, once I get them working,
will be to thoroughly test them. I'm thinking of using the Phoronix
Test Suite as a general hardware stresser, but I'll have to see if it
will actually run on them.

> The bug you have fixed (with great effort) in the unaligned
> trap handler further confirm to me that that not many users
> are running recent kernels on sparc32 hardware.
> I also recall that I broke sparc32 during the unification,
> which took months to e detected.
>
> Personally I run sparc only to test my kernel contributions,
> as the hw I have available is too noisy / too slow to
> be used for anything else.

I'm in the exact same boat. There was a time when I'd leave my Ultra1E
running overnight doing sparc32 / sparc64 randconfig builds (and I
found a couple of minor bugs with that) but my goal at the moment is
to come up with some way to do automated testing on my machines.

> It also looks better on the CV when I can say that I have refactored
> the IRQ support on sparc compared to some random kbuild hacking :-)

I put that I am active within the open source community, and I can
always boast that I've (indirectly^3) helped the ESA =)

> So Gaislers contributions helps the sparc32 community.
> And I expect that the combined effort will allow more people to
> use a recent kernel on sparc32 and thus will increase the community.
> But how big the community is - is an open question to me.

My estimates, based upon mailing list contributions and the general
attitude of distros is that the Linux sparc32 community that's vocally
interested in modern software consists of 5-10 people. That said,
Gaisler is changing things considerably. I was surprised to download
the source of u-boot the other day and see that there was a sparc32
directory in there with their code in it. I was more surprised when I
saw that it was listed as supported in their architecture list.

Leon seems to be really re-igniting the sparc32 kernel effort, and I
can only praise them for that.

> Note: If someone has - or knows about - a SS1000 that I can use
> for Linux testing I am all ears. (I live in Scandinavia / Denmark)
> Contact me in private as this in not relevant on the list.

If anyone has one in south-eastern Australia, I would be happy to
provide it with power, accommodation and occasional kernel builds.
Again, contact me in private as this is not relevant to the list.

Sam, if I do end up with one, I'm happy to be your hands / give you
whatever access to it that you want.

Thanks,

Patch

diff --git a/arch/sparc/kernel/una_asm_32.S b/arch/sparc/kernel/una_asm_32.S
index 8cc0345..8f096e8 100644
--- a/arch/sparc/kernel/una_asm_32.S
+++ b/arch/sparc/kernel/una_asm_32.S
@@ -24,9 +24,9 @@  retl_efault:
 	.globl	__do_int_store
 __do_int_store:
 	ld	[%o2], %g1
-	cmp	%1, 2
+	cmp	%o1, 2
 	be	2f
-	 cmp	%1, 4
+	 cmp	%o1, 4
 	be	1f
 	 srl	%g1, 24, %g2
 	srl	%g1, 16, %g7