On Sat, Jun 16, 2012 at 03:23:31PM +0200, Bjørn Mork wrote:
> Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> > On Fri, Jun 15, 2012 at 11:02:55PM +0200, Geert Uytterhoeven wrote:
> >
> >> As "kernel_ulong_t driver_info" is no longer naturally aligned, the
> >> compiler will
> >> add implicit padding. But the padding depends on the architecture.
> >
> > Ah, so we were "lucky" before, nice.
>
> I don't really believe in luck :-) I think someone has been really
> smart here. Maybe too smart...
No, I think the previous structure was just "lucky" in that it just
happened to be the right alignment. I say this as I think I was the one
who created that structure years ago. Or maybe not, this was back in
the 2.3 kernel days, I can't remember what patches I wrote last week...
> >> It can be fixed by adding explicit padding. Probably it should be padded by
> >> 7 bytes (not 3), as kernel_ulong_t may require 8-byte alignment on some 64-bit
> >> platforms. Or by an explicit alignment attribute.
> >>
> >> See also
> >> * commit 8175fe2dda1c93a9c596921c8ed4a0b4baccdefe ("HID: fix
> >> hid_device_id for cross compiling")
> >> * commit 7492d4a416d68ab4bd254b36ffcc4e0138daa8ff ("sdio: fix module
> >> device table definition for m68k")
> >> * commit 9e2d3cd34a159948dc753a14573e16bffc04dba8 ("[PATCH]
> >> mod_devicetable.h fixes")
> >
> > So would the patch below fix this? It should force alignment of the
> > driver_data field, which is all you want here, right?
> >
> >> Still, there's a bug in file2alias (which is compiled by the host
> >> compiler), in that
> >> it may use different padding than the target platform when cross-compiling.
> >
> > That's not good, but outside of this specific issue, right? Have we
> > just been fortunate it hasn't really hit us yet?
> >
> > thanks,
> >
> > greg k-h
> >
> > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> > index 7771d45..6955045 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -122,7 +122,8 @@ struct usb_device_id {
> > __u8 bInterfaceNumber;
> >
> > /* not matched against */
> > - kernel_ulong_t driver_info;
> > + kernel_ulong_t driver_info
> > + __attribute__((aligned(sizeof(kernel_ulong_t))));
> > };
>
>
> This feels a lot like papering over the real problem. It will solve
> this instance, but the list of such previous "paper work" that Geert
> provided should be enough evidence that this will happen again the next
> time someone modifies a device id struct for some subsystem.
Hopefully not, if you add another field here, the alignment force will
keep things lined up properly, from what I can tell. Is that not true?
> And adding forced aligment here feels wrong since there is no good
> reason why the (target) compiler shouldn't know the proper alignment for
> this structure, is there? OK, "feels wrong" is not a good argument. But
> it would be better to solve this problem once and for all.
C doesn't require the structure to be aligned. Actually the spec says
it doesn't guarantee anything about this, we just "know" that gcc is
going to be semi-sane and try to do the best it can. Hopefully clang is
also semi-sane as well.
So because of that, we have to give it some guidance, hence the patch.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-next" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
