On 07/09/2012 03:57 PM, Eric Sandeen wrote:
On 7/9/12 9:45 AM, Gordan Bobic wrote:
> On 07/09/2012 03:24 PM, Eric Sandeen wrote:
>> On 7/8/12 1:07 AM, Jon Masters wrote:
>>> On 07/07/2012 08:42 PM, Gordan Bobic wrote:
>>>> Can anyone confirm whether this has been fixed since the bug
>>>> was filed? Or whether there is still a risk of trashing the
>>>> file system on ARMv5/ARMv6?
>>>>
>>>>
https://bugzilla.redhat.com/show_bug.cgi?id=680090
>>>
>>> I believe Eric said this was now likely fixed upstream. Have you
>>> asked him to confirm? Let me copy him, and I'm sure he'll say
>>> it's all fine :)
>>
>> nope, sorry - well, not exactly fine. ;)
>>
>> We fixed on-disk structure alignment errors for XFS - this is a
>> different problem, with in-memory alignment...
>>
>> These memory access problem on arm may well still exist.
>>
>> It's "only" a warning about alignment though, right - I don't
think
>> this results in any corruption etc, does it (anyway, filefrag is
>> read-only).
>
> What concerns me is that the buffers are allocated as char[4096] all
> over the place in e2fsprogs code files. That is inherently unsafe on
> everything except x86 and ARMv7+. I wouldn't want to assume that
> filefrag is the only affected program from the e2fsprogs suite,
Running xfstests& e2fsprogs "make check" should expose any others.
Maybe. But if make check missed filefrag issues, so how far can it be
trusted?
> especially since the scope for trashing the file system is
pretty
> high.
Maybe I should know, but what happens on< ARMv7+ ? Memory corruption?
Essentially, yes.
You have an unaligned array being cast into a struct. You then
dereference an element in that struct. Because the array wasn't aligned,
the struct isn't aligned. Thus, the elements in the struct aren't
aligned. So when you dereference a pointer into the struct, it'll get
truncated to the word boundary, then dereferenced. So your data will be
the prefixed with unexpected garbage, and you won't get the tail of the
data.
This issue also affects SPARC and Itanium, IIRC, likely a lot of others,
too. It's only x86 and ARMv7+ that have transparent automatic alignment
fixup in hardware. Note that even on those there may be a performance
penalty, and there is always a performance penalty for straddling cache
lines (which is much more likely when you're not paying attention to
alignment - one of the reasons why ICC has an option to align all data
to a 16-byte boundary, just to make sure).
If so I'm surprised that extN doesn't fall down all over the
place today.
Me too. :)
But it is possible that fsck doesn't get run on older ARMs often.
>> And as the comments in this bug indicate, I'm really not
quite sure
>> what the accepted method of fixing is.
>
> The options I can see are:
>
> 1) Detect sizeof(int) and declare the arrays as a suitable multiple
> of that. int _should_ be word sized on most things AFAIK.
now I'll sound really dense, but isn't 4096 a multiple of sizeof(int)
on arm?
Sure, it is, but arrays of type foo are aligned to a size increment of
foo. So the beginning of an array of char will be byte aligned since
char is 1 byte.
I guess the weird char buf[4096] construct was to avoid malloc/free
or something.
I wouldn't know, I didn't write it. :)
> 2) Check the required alignment for the arch at build time, and
apply
> it with "__attribute__ aligned" in every relevant instance. There's a
> LOT of those, but that's what you get for using non-portable
> techniques. :(
Yeah, that's nasty. On xfs we did some #ifdef macros to add __attributes__
to a few on-disk structures for arm old ABI, but that was fairly well-
contained.
> 3) Add a new flag to gcc to align all arrays to a particular
> boundary. Unlikely to happen. FWIW, ICC has such an option, IIRC as
> it helps with performance, too, if things aren't straddling cache
> lines.
>
>> I imagine there are several applications that exhibit similar
>> behavior?
>
> Not many, if you are referring to other packages. I filed bugs for
> most of them, but I filed against the available ARM releases (since
> rawhide wasn't up to running on ARM), so they all got closed without
> resolution pretty quickly. I gave up on bothering to file bugs after
> that.
Sorry about that. Obviously it's a higher priority now.
Yeah, it's understandable, but it was highly demotivating. Eventually I
ended up causing confusion by filing against rawhide even though it
wasn't quite applicable to ARM (well, "rawhide" for ARM was a few
releases behind, at least :) just to avoid the bugzapper. There didn't
seem to be a less wrong solution at the time.
Gordan