| 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 | expand |
ś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
ś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 >
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().
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.
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.
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
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.
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.
ś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
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;
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(-)