Re: [PATCH] Fix bus error when accessing MBR partition records

[NeilBrown <neilb@suse.com>]

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

On Mon, Oct 03 2016, James Clarke wrote:

> Hi Neil,
>
>> On 2 Oct 2016, at 23:32, NeilBrown <neilb@suse.com> wrote:
>> 
>>> On Thu, Sep 29 2016, James Clarke wrote:
>>> 
>>> Since the MBR layout only has partition records as 2-byte aligned, the 32-bit
>>> fields in them are not aligned. Thus, they cannot be accessed on some
>>> architectures (such as SPARC) by using a "struct MBR_part_record *" pointer,
>>> as the compiler can assume that the pointer is properly aligned. Instead, the
>>> records must be accessed by going through the MBR struct itself every time.
>> 
>> Weird....
>> 
>> Can you see if adding "__attribute__((packed))" to struct
>> MBR_part_record also fixes the problem?
>
> That also works. When I wrote the patch initially, I wasn’t sure if it was a
> "correct" fix, but having looked into it more I *believe* it is conformant. The
> alignment of a packed struct is 1-byte, so, while the compiler may know that the
> 32-bit fields are 8-byte aligned within the struct, the pointer to the struct
> need not be aligned, and so the correct conservative code is generated.
>
>> It seems strange that the compiler lets you take a pointer, but then
>> doesn't use it correctly.  Maybe it is an inconsistency in the types.
>
> Yes, the type doesn’t include the provenance of the pointer, so in general the
> compiler can’t know it came from a packed struct (although in this case not much
> static analysis would be needed). See [1] and [2].
>
>> I don't necessarily disagree with your fix, but I'd like to understand
>> why the current code is wrong.
>
> Hopefully the links make it clearer.
>
> Regards,
> James
>
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> [2] https://llvm.org/bugs/show_bug.cgi?id=22821

Thanks.

It looks as though the type change I suggested would work, but probably isn't
the best solution.
Your patch is probably safest, though adding the __attribute__((packed))
as well wouldn't hurt.

I'll leave it for Jes to decide what exactly to apply, but I can offer
a

  Reviewed-by: NeilBrown <neilb@suse.com>

for you patch.

BTW I tried compiling mdadm with clang to see if my clang was new enough
to give a warning (it isn't) but it found a few other things to give
errors about ... I should post patches.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]