[v1,1/2] gpiolib: Fix line event handling in compatible mode
diff mbox series

Message ID 20191204194229.64251-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • [v1,1/2] gpiolib: Fix line event handling in compatible mode
Related show

Commit Message

Andy Shevchenko Dec. 4, 2019, 7:42 p.m. UTC
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.

Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/gpio.h |  6 +++++
 2 files changed, 54 insertions(+), 3 deletions(-)

Comments

Bartosz Golaszewski Dec. 6, 2019, 10:57 a.m. UTC | #1
śr., 4 gru 2019 o 20:42 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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.
>
> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++++++++++++++---
>  include/uapi/linux/gpio.h |  6 +++++
>  2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7340e4d0e873..134985210619 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -825,17 +825,26 @@ static __poll_t lineevent_poll(struct file *filep,
>         return events;
>  }
>
> -
>  static ssize_t lineevent_read(struct file *filep,
>                               char __user *buf,
>                               size_t count,
>                               loff_t *f_ps)
>  {
>         struct lineevent_state *le = filep->private_data;
> +       struct gpioevent_data event, *e = &event;
> +       /* We need size of each member to avoid endianess issues below */
> +       size_t ts_sz = sizeof(e->timestamp), id_sz = sizeof(e->id), e_sz;
>         unsigned int copied;
>         int ret;
>
> -       if (count < sizeof(struct gpioevent_data))
> +       /*
> +        * In compatible mode, when kernel is 64-bit and user space is 32-bit,
> +        * we may not tell what user wanted here when count is bigger than size
> +        * of one event, so, we just assume that user asks for precisely one
> +        * event.
> +        */
> +       e_sz = in_compat_syscall() ? ts_sz + id_sz : sizeof(*e);
> +       if (count < e_sz)
>                 return -EINVAL;
>
>         do {
> @@ -851,7 +860,43 @@ static ssize_t lineevent_read(struct file *filep,
>
>                 if (mutex_lock_interruptible(&le->read_lock))
>                         return -ERESTARTSYS;
> -               ret = kfifo_to_user(&le->events, buf, count, &copied);
> +               if (in_compat_syscall()) {
> +                       /*
> +                        * First we peek the one event and, if there is
> +                        * no error during copying to user space, skip it
> +                        * later.
> +                        */
> +                       if (kfifo_peek(&le->events, e))
> +                               copied = e_sz;
> +                       else
> +                               copied = 0;
> +
> +                       /* Do not try to copy garbage to the user */
> +                       ret = copied ? 0 : -EFAULT;
> +
> +                       /*
> +                        * Due to endianess concerns we have to copy this
> +                        * member-by-member. Luckily there are no members
> +                        * less than 32-bit.
> +                        */
> +                       if (!ret)
> +                               ret = copy_to_user(buf, &e->timestamp, ts_sz);
> +                       if (!ret)
> +                               ret = copy_to_user(buf + ts_sz, &e->id, id_sz);
> +
> +                       if (ret) {
> +                               /*
> +                                * Either we have got nothing from the FIFO or
> +                                * one of copy_to_user() calls failed.
> +                                */
> +                               ret = -EFAULT;
> +                       } else {
> +                               /* Skip peeked event if no error happened */
> +                               kfifo_skip(&le->events);
> +                       }
> +               } else {
> +                       ret = kfifo_to_user(&le->events, buf, count, &copied);
> +               }
>                 mutex_unlock(&le->read_lock);
>
>                 if (ret)
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 799cf823d493..054756bf6991 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -168,6 +168,12 @@ struct gpioevent_request {
>   * struct gpioevent_data - The actual event being pushed to userspace
>   * @timestamp: best estimate of time of event occurrence, in nanoseconds
>   * @id: event identifier
> + *
> + * Warning! This structure has issues in the compatible mode, when
> + * kernel is 64-bit and user space is 32-bit, due to alignment
> + * differences.
> + *
> + * It's not recommended to retrieve more than one event at a time.
>   */
>  struct gpioevent_data {
>         __u64 timestamp;
> --
> 2.24.0
>

Linus,

please don't apply the patches from this series - I will incorporate
them into my LINEINFO_WATCH ioctl() series.

Best regards,
Bartosz Golaszewski
Bartosz Golaszewski Dec. 10, 2019, 9:06 a.m. UTC | #2
śr., 4 gru 2019 o 20:42 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> 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?

Bart

> Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c    | 51 ++++++++++++++++++++++++++++++++++++---
>  include/uapi/linux/gpio.h |  6 +++++
>  2 files changed, 54 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 7340e4d0e873..134985210619 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -825,17 +825,26 @@ static __poll_t lineevent_poll(struct file *filep,
>         return events;
>  }
>
> -
>  static ssize_t lineevent_read(struct file *filep,
>                               char __user *buf,
>                               size_t count,
>                               loff_t *f_ps)
>  {
>         struct lineevent_state *le = filep->private_data;
> +       struct gpioevent_data event, *e = &event;
> +       /* We need size of each member to avoid endianess issues below */
> +       size_t ts_sz = sizeof(e->timestamp), id_sz = sizeof(e->id), e_sz;
>         unsigned int copied;
>         int ret;
>
> -       if (count < sizeof(struct gpioevent_data))
> +       /*
> +        * In compatible mode, when kernel is 64-bit and user space is 32-bit,
> +        * we may not tell what user wanted here when count is bigger than size
> +        * of one event, so, we just assume that user asks for precisely one
> +        * event.
> +        */
> +       e_sz = in_compat_syscall() ? ts_sz + id_sz : sizeof(*e);
> +       if (count < e_sz)
>                 return -EINVAL;
>
>         do {
> @@ -851,7 +860,43 @@ static ssize_t lineevent_read(struct file *filep,
>
>                 if (mutex_lock_interruptible(&le->read_lock))
>                         return -ERESTARTSYS;
> -               ret = kfifo_to_user(&le->events, buf, count, &copied);
> +               if (in_compat_syscall()) {
> +                       /*
> +                        * First we peek the one event and, if there is
> +                        * no error during copying to user space, skip it
> +                        * later.
> +                        */
> +                       if (kfifo_peek(&le->events, e))
> +                               copied = e_sz;
> +                       else
> +                               copied = 0;
> +
> +                       /* Do not try to copy garbage to the user */
> +                       ret = copied ? 0 : -EFAULT;
> +
> +                       /*
> +                        * Due to endianess concerns we have to copy this
> +                        * member-by-member. Luckily there are no members
> +                        * less than 32-bit.
> +                        */
> +                       if (!ret)
> +                               ret = copy_to_user(buf, &e->timestamp, ts_sz);
> +                       if (!ret)
> +                               ret = copy_to_user(buf + ts_sz, &e->id, id_sz);
> +
> +                       if (ret) {
> +                               /*
> +                                * Either we have got nothing from the FIFO or
> +                                * one of copy_to_user() calls failed.
> +                                */
> +                               ret = -EFAULT;
> +                       } else {
> +                               /* Skip peeked event if no error happened */
> +                               kfifo_skip(&le->events);
> +                       }
> +               } else {
> +                       ret = kfifo_to_user(&le->events, buf, count, &copied);
> +               }
>                 mutex_unlock(&le->read_lock);
>
>                 if (ret)
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 799cf823d493..054756bf6991 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -168,6 +168,12 @@ struct gpioevent_request {
>   * struct gpioevent_data - The actual event being pushed to userspace
>   * @timestamp: best estimate of time of event occurrence, in nanoseconds
>   * @id: event identifier
> + *
> + * Warning! This structure has issues in the compatible mode, when
> + * kernel is 64-bit and user space is 32-bit, due to alignment
> + * differences.
> + *
> + * It's not recommended to retrieve more than one event at a time.
>   */
>  struct gpioevent_data {
>         __u64 timestamp;
> --
> 2.24.0
>
Andy Shevchenko Dec. 10, 2019, 1:44 p.m. UTC | #3
On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> śr., 4 gru 2019 o 20:42 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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?

On x86 the rest of the structures is naturally aligned by 8 bytes.

But you are right, the ABI is broken more widely than simple ->read().
Kent Gibson Dec. 10, 2019, 2:39 p.m. UTC | #4
On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> śr., 4 gru 2019 o 20:42 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> 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.
Andy Shevchenko Dec. 10, 2019, 4:55 p.m. UTC | #5
On Tue, Dec 10, 2019 at 10:39:02PM +0800, Kent Gibson wrote:
> On Tue, Dec 10, 2019 at 10:06:04AM +0100, Bartosz Golaszewski wrote:
> > śr., 4 gru 2019 o 20:42 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> 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.

Exactly.

> (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.

I spoke to colleague of mine and has been told that best option is to fill all
gaps explicitly to have all members in the struct + 8 bytes alignment at the
end (also with explicit member).

> Neither of those options are available for the
> gpioevent_data, as that would break the ABI.

ABI needs v2 actually.

So, it must be

struct gpioevent_data_v2 {
	__u64 timestamp;
	__u32 id;
	__u32 padding;
};

And so on...

> 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.

Right.

> 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.
Bartosz Golaszewski Dec. 11, 2019, 9:18 a.m. UTC | #6
wt., 10 gru 2019 o 17:55 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> > 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.
>
> Exactly.
>
> > (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.
>
> I spoke to colleague of mine and has been told that best option is to fill all
> gaps explicitly to have all members in the struct + 8 bytes alignment at the
> end (also with explicit member).
>
> > Neither of those options are available for the
> > gpioevent_data, as that would break the ABI.
>
> ABI needs v2 actually.
>

I finally sat down to integrate this with my series and figured that
this can't go on top of it. It's a bug-fix actually and maybe even
stable material.

On the other hand - if we have so few users of GPIO chardev with
32-bit user-space and 64-bit kernel - maybe we should just bite the
bullet, not fix this one, deprecate it and introduce a proper v2 of
the API?

Bart
Kent Gibson Dec. 11, 2019, 9:29 a.m. UTC | #7
On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> wt., 10 gru 2019 o 17:55 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > > 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.
> >
> > Exactly.
> >
> > > (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.
> >
> > I spoke to colleague of mine and has been told that best option is to fill all
> > gaps explicitly to have all members in the struct + 8 bytes alignment at the
> > end (also with explicit member).
> >
> > > Neither of those options are available for the
> > > gpioevent_data, as that would break the ABI.
> >
> > ABI needs v2 actually.
> >
> 
> I finally sat down to integrate this with my series and figured that
> this can't go on top of it. It's a bug-fix actually and maybe even
> stable material.
> 
> On the other hand - if we have so few users of GPIO chardev with
> 32-bit user-space and 64-bit kernel - maybe we should just bite the
> bullet, not fix this one, deprecate it and introduce a proper v2 of
> the API?
> 

Fixing it in API v2 makes the most sense to me.

Kent.
Andy Shevchenko Dec. 11, 2019, 10:47 a.m. UTC | #8
On Wed, Dec 11, 2019 at 05:29:21PM +0800, Kent Gibson wrote:
> On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> > wt., 10 gru 2019 o 17:55 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):

> > > > Neither of those options are available for the
> > > > gpioevent_data, as that would break the ABI.
> > >
> > > ABI needs v2 actually.
> > >
> > 
> > I finally sat down to integrate this with my series and figured that
> > this can't go on top of it. It's a bug-fix actually and maybe even
> > stable material.
> > 
> > On the other hand - if we have so few users of GPIO chardev with
> > 32-bit user-space and 64-bit kernel - maybe we should just bite the
> > bullet, not fix this one, deprecate it and introduce a proper v2 of
> > the API?
> > 
> 
> Fixing it in API v2 makes the most sense to me.

I agree. Please, use only second patch in my series (if needed I can resend
it separately) and drop this one.

P.S. Actually it's not a bugfix since it's never worked from the day 1.
Bartosz Golaszewski Dec. 11, 2019, 1:15 p.m. UTC | #9
śr., 11 gru 2019 o 11:47 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Wed, Dec 11, 2019 at 05:29:21PM +0800, Kent Gibson wrote:
> > On Wed, Dec 11, 2019 at 10:18:39AM +0100, Bartosz Golaszewski wrote:
> > > wt., 10 gru 2019 o 17:55 Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> napisał(a):
>
> > > > > Neither of those options are available for the
> > > > > gpioevent_data, as that would break the ABI.
> > > >
> > > > ABI needs v2 actually.
> > > >
> > >
> > > I finally sat down to integrate this with my series and figured that
> > > this can't go on top of it. It's a bug-fix actually and maybe even
> > > stable material.
> > >
> > > On the other hand - if we have so few users of GPIO chardev with
> > > 32-bit user-space and 64-bit kernel - maybe we should just bite the
> > > bullet, not fix this one, deprecate it and introduce a proper v2 of
> > > the API?
> > >
> >
> > Fixing it in API v2 makes the most sense to me.
>
> I agree. Please, use only second patch in my series (if needed I can resend
> it separately) and drop this one.
>
> P.S. Actually it's not a bugfix since it's never worked from the day 1.
>

Yeah I guess so. Anyway, in this case, I won't be adding the event
watch feature to v1 - I'll go ahead and just add it to v2 then.

Bart

Patch
diff mbox series

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7340e4d0e873..134985210619 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -825,17 +825,26 @@  static __poll_t lineevent_poll(struct file *filep,
 	return events;
 }
 
-
 static ssize_t lineevent_read(struct file *filep,
 			      char __user *buf,
 			      size_t count,
 			      loff_t *f_ps)
 {
 	struct lineevent_state *le = filep->private_data;
+	struct gpioevent_data event, *e = &event;
+	/* We need size of each member to avoid endianess issues below */
+	size_t ts_sz = sizeof(e->timestamp), id_sz = sizeof(e->id), e_sz;
 	unsigned int copied;
 	int ret;
 
-	if (count < sizeof(struct gpioevent_data))
+	/*
+	 * In compatible mode, when kernel is 64-bit and user space is 32-bit,
+	 * we may not tell what user wanted here when count is bigger than size
+	 * of one event, so, we just assume that user asks for precisely one
+	 * event.
+	 */
+	e_sz = in_compat_syscall() ? ts_sz + id_sz : sizeof(*e);
+	if (count < e_sz)
 		return -EINVAL;
 
 	do {
@@ -851,7 +860,43 @@  static ssize_t lineevent_read(struct file *filep,
 
 		if (mutex_lock_interruptible(&le->read_lock))
 			return -ERESTARTSYS;
-		ret = kfifo_to_user(&le->events, buf, count, &copied);
+		if (in_compat_syscall()) {
+			/*
+			 * First we peek the one event and, if there is
+			 * no error during copying to user space, skip it
+			 * later.
+			 */
+			if (kfifo_peek(&le->events, e))
+				copied = e_sz;
+			else
+				copied = 0;
+
+			/* Do not try to copy garbage to the user */
+			ret = copied ? 0 : -EFAULT;
+
+			/*
+			 * Due to endianess concerns we have to copy this
+			 * member-by-member. Luckily there are no members
+			 * less than 32-bit.
+			 */
+			if (!ret)
+				ret = copy_to_user(buf, &e->timestamp, ts_sz);
+			if (!ret)
+				ret = copy_to_user(buf + ts_sz, &e->id, id_sz);
+
+			if (ret) {
+				/*
+				 * Either we have got nothing from the FIFO or
+				 * one of copy_to_user() calls failed.
+				 */
+				ret = -EFAULT;
+			} else {
+				/* Skip peeked event if no error happened */
+				kfifo_skip(&le->events);
+			}
+		} else {
+			ret = kfifo_to_user(&le->events, buf, count, &copied);
+		}
 		mutex_unlock(&le->read_lock);
 
 		if (ret)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 799cf823d493..054756bf6991 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -168,6 +168,12 @@  struct gpioevent_request {
  * struct gpioevent_data - The actual event being pushed to userspace
  * @timestamp: best estimate of time of event occurrence, in nanoseconds
  * @id: event identifier
+ *
+ * Warning! This structure has issues in the compatible mode, when
+ * kernel is 64-bit and user space is 32-bit, due to alignment
+ * differences.
+ *
+ * It's not recommended to retrieve more than one event at a time.
  */
 struct gpioevent_data {
 	__u64 timestamp;