Re: [PATCH v1 1/2] gpiolib: Fix line event handling in compatible mode

[Kent Gibson <warthog618@xxxxxxxxx>]

On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> śr., 4 gru 2019 o 20:42 Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx> napisał(a):
> >
> > The introduced line even handling ABI in the commit
> >
> >   61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> >
> > missed the fact that 64-bit kernel may serve for 32-bit applications.
> > In such case the very first check in the lineevent_read() will fail
> > due to alignment differences.
> >
> > To workaround this we do several things here:
> > - put warning comment to UAPI header near to the structure description
> > - derive the size of the structure in the compatible mode from its members
> > - check for the size of this structure in the ->read() callback
> > - return only one event in the compatible mode at a time
> >
> > Above mitigation will work at least with libgpiod which does one event
> > at a time.
> >
> > Since the bug hasn't been reported earlier we assume that there is close
> > to zero actual users of the compatible mode to monitor GPIO events and thus
> > we might consider to rework this ABI in the future.
> >
> 
> How come this only affects the read operation but not the structures
> passed as arguments to ioctl() calls?
> 

For Go the structs are aligned based on the size of their components so
that arrays of struct are naturally aligned.  The struct is given a
hidden trailing pad so that a subsequent struct will be correctly aligned.
The sizeof the struct includes this hidden pad.
I'm pretty sure the same is true for gcc.

The gpioevent_data contains a __u64 which causes the whole struct to be
64 bit aligned on 64 bit, so it actually looks like this internally:

struct gpioevent_data {
	__u64 timestamp;
	__u32 id;
    __u32 pad; // hidden
};

so 16 bytes.

On 32 bit the struct is 32 bit aligned and the trailing pad is missing,
so 12 bytes. This causes grief for the read due to the size mismatch.
(I'm sorry to say I had to add the pad to my Go gpiod library to get it
to read event data - but forgot to go back later and work out why -
until now :-()

Your new info change struct has the same problem, as it also contains a
__u64 and ends up with an odd number of __u32s, so gets a trailing pad
on 64 bit.  Using __packed seems to inhibit the trailing pad.
Or you could explicitly add the pad so the struct will be 64bit aligned
even on 32bit.  Neither of those options are available for the
gpioevent_data, as that would break the ABI.

The ioctl structs only contain __u32s (or smaller) and so get aligned to
32 bit boundaries on both 32 and 64 bit. So just lucky.

It is also lucky that the event_data happens to have the __u64 at the
beginning of the struct or there could be padding inserted between
fields, not just at the end.  Similarly the byte array lengths in the
ioctl structs are all multiples of 4, so all the components happen to 
align to 32 bit boundaries.

Kent.