Re: ARM unaligned MMIO access with attribute((packed))
On Wed, Feb 02, 2011 at 05:00:20PM +0100, Arnd Bergmann wrote: > I would suggest fixing this by: > > 1. auditing all uses of __attribute__((packed)) in the Linux USB code > and other drivers, removing the ones that are potentially harmful. > > 2. Changing the ARM MMIO functions to use inline assembly instead of > direct pointer dereference. > > 3. Documenting the gcc behavior as undefined. We used to use inline assembly at one point, but that got chucked out. The problem is that using asm() for this causes GCC to generate horrid code. 1. there's no way to tell GCC that the inline assembly is a load instruction and therefore it needs to schedule the following instructions appropriately. 2. GCC will needlessly reload pointers from structures and other such behaviour because it can't be told clearly what the inline assembly is doing, so the inline asm needs to have a "memory" clobber. 3. It seems to misses out using the pre-index addressing, prefering to create add/sub instructions prior to each inline assembly load/store. 4. There are no (documented) constraints in GCC to allow you to represent the offset format for the half-word instructions. Overall, it means greater register pressure, more instructions, larger functions, greater instruction cache pressure, etc.
Re: ARM unaligned MMIO access with attribute((packed))
On Wed, Feb 02, 2011 at 05:51:27PM +0100, Richard Guenther wrote:
> > I would suggest fixing this by:
> >
> > 1. auditing all uses of __attribute__((packed)) in the Linux USB code
> > and other drivers, removing the ones that are potentially harmful.
> >
> > 2. Changing the ARM MMIO functions to use inline assembly instead of
> > direct pointer dereference.
> >
> > 3. Documenting the gcc behavior as undefined.
>
> The pointer conversions already invoke undefined behavior as specified by the
> C standard (6.3.2.3/7).
Just to be clear: you are not saying that the ARM implementation is
undefined.
What you're saying is that converting from a pointer with less strict
alignment requirements to a pointer with more strict alignment
requirements is undefined.
IOW:
unsigned long *blah(unsigned char *c)
{
return (unsigned long *)c;
}
would be undefined, but:
unsigned char *blah(unsigned long *c)
{
return (unsigned char *)c;
}
would not be.
If you're saying something else, please explain with reference to the
point in the C standard you quote above.
Re: ARM unaligned MMIO access with attribute((packed))
On Wed, Feb 02, 2011 at 01:38:31PM -0800, David Miller wrote: > From: Russell King - ARM Linux > Date: Wed, 2 Feb 2011 16:37:02 + > > > 1. there's no way to tell GCC that the inline assembly is a load > >instruction and therefore it needs to schedule the following > >instructions appropriately. > > Just add a dummy '"m" (pointer)' asm input argument to the inline asm > statement. Just make sure "typeof(pointer)" has a size matching the > size of the load your are performing. That involves this problematical cast from a packed struct pointer to an unsigned long pointer, which according to the C standard and GCC folk is undefined. > > 2. GCC will needlessly reload pointers from structures and other such > >behaviour because it can't be told clearly what the inline assembly > >is doing, so the inline asm needs to have a "memory" clobber. > > This behavior is correct, and in fact needed. Writing to chip registers > can trigger changes to arbitrary main memory locations. That is really not an argument which stands up to analysis. When does main memory locations change as a result of a write to a chip register? The answer is: when DMA is performed - which could be many microseconds or even milliseconds after you've written the register, which would be long after you've exited the function doing the writing. Not only that, but we have the DMA API to deal with the implications of that. On ARM, that's a function call, and GCC can't make any assumptions about memory contents across function calls where it doesn't know what the function does. Practice over the last 15 years on ARM has also shown that this is not necessary.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 06:17:11PM +0100, Richard Guenther wrote:
> On Thu, Dec 17, 2009 at 6:09 PM, David Daney
> wrote:
> > Jamie Lokier wrote:
> >>
> >> Uwe Kleine-König wrote:
> >>>
> >>> Use the new unreachable() macro instead of for(;;);
> >>> *(int *)0 = 0;
> >>> /* Avoid "noreturn function does return" */
> >>> - for (;;);
> >>> + unreachable();
> >>
> >> Will GCC-4.5 remove ("optimise away") the *(int *)0 = 0 because it
> >> knows the branch of the code leading to unreachable can never be reached?
> >>
> >
> > I don't know the definitive answer, so I am sending to g...@...
> >
> > FYI: #define unreachable() __builtin_unreachable()
>
> It shouldn't as *(int *)0 = 0; might trap. But if you want to be sure
> use
>__builtin_trap ();
> instead for the whole sequence (the unreachable is implied then).
> GCC choses a size-optimal trap representation for your target then.
How is "size-optimal trap" defined? The point of "*(int *)0 = 0;" is
to cause a NULL pointer dereference which is trapped by the kernel to
produce a full post mortem and backtrace which is easily recognised
as a result of this code.
Having gcc decide on, maybe, an undefined instruction instead would be
confusing.
Let me put it another way: I want this function to terminate with an
explicit NULL pointer dereference in every case.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > Besides, didn't I see a whole bunch of kernel security patches related > to null pointer dereferences lately? If page 0 can be mapped, you > suddenly won't get your trap. Page 0 can not be mapped on ARM kernels since the late 1990s, and this protection is independent of the generic kernel. Milage may vary on other architectures, but that's not a concern here.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 11:14:01AM -0800, Joe Buck wrote: > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > Besides, didn't I see a whole bunch of kernel security patches related > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > suddenly won't get your trap. > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > protection is independent of the generic kernel. > > > > Milage may vary on other architectures, but that's not a concern here. > > I don't understand, though, why you would want to implement a generally > useful facility (make the kernel trap so you can do a post-mortem > analysis) in a way that's only safe for the ARM port. The discussion at hand is about code contained in the ARM supporting files (arch/arm/kernel/traps.c), rather than the generic kernel. As such, it is not relevant to any architecture other than ARM.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 07:38:26PM +, Jamie Lokier wrote: > Joe Buck wrote: > > On Thu, Dec 17, 2009 at 11:06:13AM -0800, Russell King - ARM Linux wrote: > > > On Thu, Dec 17, 2009 at 10:35:17AM -0800, Joe Buck wrote: > > > > Besides, didn't I see a whole bunch of kernel security patches related > > > > to null pointer dereferences lately? If page 0 can be mapped, you > > > > suddenly won't get your trap. > > > > > > Page 0 can not be mapped on ARM kernels since the late 1990s, and this > > > protection is independent of the generic kernel. > > > > > > Milage may vary on other architectures, but that's not a concern here. > > It does not trap on at least one ARM-nommu kernel... I was going to say the following in a different reply but discarded it because it wasn't relevant to the GCC list. I regard ARM nommu as highly experimental, especially as the maintainer vanished half way through merging it into mainline. I know that there are some parts of ARM nommu that are highly buggy - such as ARM940 support invalidating the entire data cache on any DMA transaction... say goodbye stacked return addresses. As such, I would not be surprised if the ARM nommu kernel has _lots_ of weird and wonderful bugs. I am not surprised that NULL pointer dereferences don't work - its actually something I'd expect given that they have a protection unit which the kernel doesn't apparently touch. Maybe the protection unit code never got merged? I've no idea. As I say, uclinux support got as far as being half merged and that's roughly the state it's remained in ever since. We don't even have any no-MMU configurations which the kernel builder automatically tests for us. Given the lack of progress/bug reporting on ARM uclinux, the lack of platform support and the lack of configurations, my view is that there is no one actually using it. I know that I don't particularly take any care with respect to uclinux when making changes to the MMU based kernels. Why bother if apparantly no one's using it?
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Thu, Dec 17, 2009 at 07:48:37PM +, Russell King - ARM Linux wrote: > Given the lack of progress/bug reporting on ARM uclinux, the lack of > platform support and the lack of configurations, my view is that there > is no one actually using it. I know that I don't particularly take any > care with respect to uclinux when making changes to the MMU based kernels. > Why bother if apparantly no one's using it? Jamie, As you seem to be a user of uclinux on ARM, could you help ensure that the support there is bug fixed and we at least have a configuration file which kautobuild can use to provide feedback on the build status of uclinux on ARM please?
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: > On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: >> How is "size-optimal trap" defined? > > E.g. Sparc and MIPS have "tcc" instructions that trap based on the > condition codes, and so we eliminate the branch. That's the only > optimization we apply with __builtin_trap. > >> Let me put it another way: I want this function to terminate with an >> explicit NULL pointer dereference in every case. > > Then just use that. That's precisely what we have been using for many years.
Re: [PATCH] ARM: Convert BUG() to use unreachable()
On Tue, Dec 22, 2009 at 02:09:02PM +, Dave Korn wrote: > Russell King - ARM Linux wrote: > > On Mon, Dec 21, 2009 at 11:30:43AM -0800, Richard Henderson wrote: > >> On 12/17/2009 10:17 AM, Russell King - ARM Linux wrote: > >>> How is "size-optimal trap" defined? > >> E.g. Sparc and MIPS have "tcc" instructions that trap based on the > >> condition codes, and so we eliminate the branch. That's the only > >> optimization we apply with __builtin_trap. > >> > >>> Let me put it another way: I want this function to terminate with an > >>> explicit NULL pointer dereference in every case. > >> Then just use that. > > > > That's precisely what we have been using for many years. > > I don't understand. It should still work just fine; the original version > posted appears to simply lack 'volatile' on the (int *) cast. Neither do I - AFAIK the existing code works fine. I think this is just a noisy thread about people wanting to use the latest and greated compiler "features" whether they make sense to or not, and this thread should probably die until some problem has actually been identified. If it ain't broke, don't fix.
