| Message ID | 20200910101935.47140-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] gpiolib: Fix line event handling in syscall compatible mode | expand |
Hi Andy,
I love your patch! Perhaps something to improve:
[auto build test WARNING on gpio/for-next]
[also build test WARNING on v5.9-rc4 next-20200910]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Andy-Shevchenko/gpiolib-Fix-line-event-handling-in-syscall-compatible-mode/20200910-182240
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: ia64-randconfig-s032-20200909 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.2-191-g10164920-dirty
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/gpio/gpiolib-cdev.c:432:41: sparse: sparse: expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:432:41: sparse: sparse: Expected } at end of struct-union-enum-specifier
drivers/gpio/gpiolib-cdev.c:432:41: sparse: sparse: got timestamp
drivers/gpio/gpiolib-cdev.c:446:17: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:446:17: sparse: sparse: got &&
>> drivers/gpio/gpiolib-cdev.c:446:9: sparse: sparse: Trying to use reserved word 'if' as identifier
drivers/gpio/gpiolib-cdev.c:449:9: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:449:9: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:452:1: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:452:1: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:467:19: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:467:19: sparse: sparse: got <
drivers/gpio/gpiolib-cdev.c:467:9: sparse: sparse: Trying to use reserved word 'if' as identifier
>> drivers/gpio/gpiolib-cdev.c:470:9: sparse: sparse: Trying to use reserved word 'do' as identifier
drivers/gpio/gpiolib-cdev.c:470:12: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:470:12: sparse: sparse: got {
drivers/gpio/gpiolib-cdev.c:472:21: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:472:21: sparse: sparse: got (
drivers/gpio/gpiolib-cdev.c:472:17: sparse: sparse: Trying to use reserved word 'if' as identifier
drivers/gpio/gpiolib-cdev.c:472:21: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:472:21: sparse: sparse: got ->
drivers/gpio/gpiolib-cdev.c:472:21: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:472:21: sparse: sparse: got }
>> drivers/gpio/gpiolib-cdev.c:475:33: sparse: sparse: Trying to use reserved word 'return' as identifier
drivers/gpio/gpiolib-cdev.c:475:40: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:475:40: sparse: sparse: got bytes_read
drivers/gpio/gpiolib-cdev.c:476:25: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:476:25: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:480:33: sparse: sparse: Trying to use reserved word 'return' as identifier
drivers/gpio/gpiolib-cdev.c:480:40: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:480:40: sparse: sparse: got -
drivers/gpio/gpiolib-cdev.c:481:25: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:481:25: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got ->
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got 0
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'if' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'do' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got {
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'if' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got break
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got ->
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got &
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'do' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got {
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got (
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'if' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in nested declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got {
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got (
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'if' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got "drivers/gpio/gpiolib-cdev.c"
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'do' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got {
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got !
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at the end of type declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got }
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in nested declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got task_struct
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'struct' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ) in function declarator
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got 1037
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'switch' as identifier
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'break' as identifier
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'case' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got 1016
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'break' as identifier
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'case' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got 1019
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'break' as identifier
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'case' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: got 1037
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'break' as identifier
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Trying to use reserved word 'case' as identifier
drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: Expected ; at end of declaration
>> drivers/gpio/gpiolib-cdev.c:483:31: sparse: sparse: too many errors
# https://github.com/0day-ci/linux/commit/402a73d41d7b2b3a72dc346e62d2b4c4403a6277
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andy-Shevchenko/gpiolib-Fix-line-event-handling-in-syscall-compatible-mode/20200910-182240
git checkout 402a73d41d7b2b3a72dc346e62d2b4c4403a6277
vim +432 drivers/gpio/gpiolib-cdev.c
425
426 static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge)
427 {
428 #ifdef __x86_64__
429 /* i386 has no padding after 'id' */
430 if (in_ia32_syscall()) {
431 struct compat_ge {
> 432 compat_u64 timestamp __packed;
433 u32 id;
434 } cge;
435
436 if (buf && ge) {
437 cge = (struct compat_ge){ ge->timestamp, ge->id };
438 if (copy_to_user(buf, &cge, sizeof(cge)))
439 return -EFAULT;
440 }
441
442 return sizeof(cge);
443 }
444 #endif
445
> 446 if (buf && ge) {
447 if (copy_to_user(buf, ge, sizeof(*ge)))
448 return -EFAULT;
449 }
450
451 return sizeof(*ge);
452 }
453
454 static ssize_t lineevent_read(struct file *file,
455 char __user *buf,
456 size_t count,
457 loff_t *f_ps)
458 {
459 struct lineevent_state *le = file->private_data;
460 struct gpioevent_data ge;
461 ssize_t bytes_read = 0;
462 ssize_t ge_size;
463 int ret;
464
465 /* When argument is NULL it returns size of the structure in user space */
466 ge_size = lineevent_to_user(NULL, NULL);
467 if (count < ge_size)
468 return -EINVAL;
469
> 470 do {
471 spin_lock(&le->wait.lock);
472 if (kfifo_is_empty(&le->events)) {
473 if (bytes_read) {
474 spin_unlock(&le->wait.lock);
> 475 return bytes_read;
476 }
477
478 if (file->f_flags & O_NONBLOCK) {
479 spin_unlock(&le->wait.lock);
480 return -EAGAIN;
481 }
482
> 483 ret = wait_event_interruptible_locked(le->wait,
484 !kfifo_is_empty(&le->events));
485 if (ret) {
486 spin_unlock(&le->wait.lock);
487 return ret;
488 }
489 }
490
491 ret = kfifo_out(&le->events, &ge, 1);
492 spin_unlock(&le->wait.lock);
493 if (ret != 1) {
494 /*
495 * This should never happen - we were holding the lock
496 * from the moment we learned the fifo is no longer
497 * empty until now.
498 */
499 ret = -EIO;
500 break;
501 }
502
503 ret = lineevent_to_user(buf + bytes_read, &ge);
504 if (ret < 0)
505 return ret;
506 bytes_read += ret;
507 } while (count >= bytes_read + ge_size);
508
509 return bytes_read;
510 }
511
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: > The introduced line even handling ABI in the commit > s/even/event/ > 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 introduce lineeven_to_user() helper which returns actual > size of the structure and copies its content to user if asked. > And again here. > Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events") > Suggested-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib-cdev.c | 41 ++++++++++++++++++++++++++++++++----- > 1 file changed, 36 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index e6c9b78adfc2..a6a8384c8255 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -423,6 +423,33 @@ static __poll_t lineevent_poll(struct file *file, > return events; > } > > +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge) > +{ > +#ifdef __x86_64__ > + /* i386 has no padding after 'id' */ > + if (in_ia32_syscall()) { > + struct compat_ge { > + compat_u64 timestamp __packed; > + u32 id; > + } cge; > + > + if (buf && ge) { > + cge = (struct compat_ge){ ge->timestamp, ge->id }; > + if (copy_to_user(buf, &cge, sizeof(cge))) > + return -EFAULT; > + } > + > + return sizeof(cge); > + } > +#endif > + > + if (buf && ge) { > + if (copy_to_user(buf, ge, sizeof(*ge))) > + return -EFAULT; > + } > + > + return sizeof(*ge); > +} > The dual-purpose nature of this function makes it more complicated than it needs to be. I was going to suggest splitting it into separate functions, but... Given that struct compat_ge is a strict truncation of struct gpioevent_data, how about reducing this function to just determining the size of the event for user space, say lineevent_user_size(), and replacing sizeof(ge) with that calculcated size throughout lineevent_read()? > static ssize_t lineevent_read(struct file *file, > char __user *buf, > @@ -432,9 +459,12 @@ static ssize_t lineevent_read(struct file *file, > struct lineevent_state *le = file->private_data; > struct gpioevent_data ge; > ssize_t bytes_read = 0; > + ssize_t ge_size; > int ret; > > - if (count < sizeof(ge)) > + /* When argument is NULL it returns size of the structure in user space */ > + ge_size = lineevent_to_user(NULL, NULL); > + if (count < ge_size) > return -EINVAL; > > do { > @@ -470,10 +500,11 @@ static ssize_t lineevent_read(struct file *file, > break; > } > > - if (copy_to_user(buf + bytes_read, &ge, sizeof(ge))) > - return -EFAULT; > - bytes_read += sizeof(ge); > - } while (count >= bytes_read + sizeof(ge)); > + ret = lineevent_to_user(buf + bytes_read, &ge); > + if (ret < 0) > + return ret; > + bytes_read += ret; > + } while (count >= bytes_read + ge_size); > > return bytes_read; > } > -- > 2.28.0 > Is patch 2 in any way related to this patch? Cheers, Kent.
On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote: > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: > > The introduced line even handling ABI in the commit > > > > s/even/event/ > > > 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 introduce lineeven_to_user() helper which returns actual > > size of the structure and copies its content to user if asked. > > > > And again here. Thanks! ... > > +#ifdef __x86_64__ > > + /* i386 has no padding after 'id' */ > > + if (in_ia32_syscall()) { > > + struct compat_ge { > > + compat_u64 timestamp __packed; > > + u32 id; > > + } cge; > > + > > + if (buf && ge) { > > + cge = (struct compat_ge){ ge->timestamp, ge->id }; > > + if (copy_to_user(buf, &cge, sizeof(cge))) > > + return -EFAULT; > > + } > > + > > + return sizeof(cge); > > + } > > +#endif > > + > > + if (buf && ge) { > > + if (copy_to_user(buf, ge, sizeof(*ge))) > > + return -EFAULT; > > + } > > + > > + return sizeof(*ge); > > +} > > > > The dual-purpose nature of this function makes it more complicated than > it needs to be. > I was going to suggest splitting it into separate functions, but... > > Given that struct compat_ge is a strict truncation of struct > gpioevent_data, how about reducing this function to just determining the > size of the event for user space, say lineevent_user_size(), and > replacing sizeof(ge) with that calculcated size throughout > lineevent_read()? So you mean something like struct compat_gpioeevent_data { compat_u64 timestamp; u32 id; } __packed; #ifdef __x86_64__ /* i386 has no padding after 'id' */ size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data); #else size_t ge_size = sizeof(struct gpioevent_data) ; #endif ? ... > Is patch 2 in any way related to this patch? No. It can be applied separately.
On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote: > On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote: > > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: > > > The introduced line even handling ABI in the commit > > > > > > > s/even/event/ > > > > > 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 introduce lineeven_to_user() helper which returns actual > > > size of the structure and copies its content to user if asked. > > > > > > > And again here. > > Thanks! > > ... > > > > +#ifdef __x86_64__ > > > + /* i386 has no padding after 'id' */ > > > + if (in_ia32_syscall()) { > > > + struct compat_ge { > > > + compat_u64 timestamp __packed; > > > + u32 id; > > > + } cge; > > > + > > > + if (buf && ge) { > > > + cge = (struct compat_ge){ ge->timestamp, ge->id }; > > > + if (copy_to_user(buf, &cge, sizeof(cge))) > > > + return -EFAULT; > > > + } > > > + > > > + return sizeof(cge); > > > + } > > > +#endif > > > + > > > + if (buf && ge) { > > > + if (copy_to_user(buf, ge, sizeof(*ge))) > > > + return -EFAULT; > > > + } > > > + > > > + return sizeof(*ge); > > > +} > > > > > > > The dual-purpose nature of this function makes it more complicated than > > it needs to be. > > I was going to suggest splitting it into separate functions, but... > > > > Given that struct compat_ge is a strict truncation of struct > > gpioevent_data, how about reducing this function to just determining the > > size of the event for user space, say lineevent_user_size(), and > > replacing sizeof(ge) with that calculcated size throughout > > lineevent_read()? > > So you mean something like > > struct compat_gpioeevent_data { > compat_u64 timestamp; > u32 id; > } __packed; > > #ifdef __x86_64__ > /* i386 has no padding after 'id' */ > size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data); > #else > size_t ge_size = sizeof(struct gpioevent_data) ; > #endif > > ? > Pretty much, though I was suggesting keeping it in a helper function, say lineevent_user_size(), not in lineevent_read() itself, to isolate all the ugliness in one small place. So in lineevent_read() you would: size_t ge_size = lineevent_user_size(); and then use that to replace all the sizeof(ge) occurrences. Cheers, Kent.
On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote: > On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote: > > On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote: > > > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: ... > > > > +#ifdef __x86_64__ > > > > + /* i386 has no padding after 'id' */ > > > > + if (in_ia32_syscall()) { > > > > + struct compat_ge { > > > > + compat_u64 timestamp __packed; > > > > + u32 id; > > > > + } cge; > > > > + > > > > + if (buf && ge) { > > > > + cge = (struct compat_ge){ ge->timestamp, ge->id }; > > > > + if (copy_to_user(buf, &cge, sizeof(cge))) > > > > + return -EFAULT; > > > > + } > > > > + > > > > + return sizeof(cge); > > > > + } > > > > +#endif > > > > + > > > > + if (buf && ge) { > > > > + if (copy_to_user(buf, ge, sizeof(*ge))) > > > > + return -EFAULT; > > > > + } > > > > + > > > > + return sizeof(*ge); > > > > +} > > > > > > The dual-purpose nature of this function makes it more complicated than > > > it needs to be. > > > I was going to suggest splitting it into separate functions, but... > > > > > > Given that struct compat_ge is a strict truncation of struct > > > gpioevent_data, how about reducing this function to just determining the > > > size of the event for user space, say lineevent_user_size(), and > > > replacing sizeof(ge) with that calculcated size throughout > > > lineevent_read()? > > > > So you mean something like > > > > struct compat_gpioeevent_data { > > compat_u64 timestamp; > > u32 id; > > } __packed; > > > > #ifdef __x86_64__ > > /* i386 has no padding after 'id' */ > > size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data); > > #else > > size_t ge_size = sizeof(struct gpioevent_data) ; > > #endif > > > > ? > > > > Pretty much, though I was suggesting keeping it in a helper function, > say lineevent_user_size(), not in lineevent_read() itself, to isolate > all the ugliness in one small place. > > So in lineevent_read() you would: > > size_t ge_size = lineevent_user_size(); > > and then use that to replace all the sizeof(ge) occurrences. But in any case it makes code a bit hackish, no? We calculate the size of one structure and by the fact *partially* copy another one. And actually if you look closer to the types, the compat_u64 is not the same as u64. So, I'm not sure your solution would work in all cases.
On Fri, Sep 11, 2020 at 12:53:55PM +0300, Andy Shevchenko wrote: > On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote: > > On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote: > > > > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: > > ... > > > > > > +#ifdef __x86_64__ > > > > > + /* i386 has no padding after 'id' */ > > > > > + if (in_ia32_syscall()) { > > > > > + struct compat_ge { > > > > > + compat_u64 timestamp __packed; > > > > > + u32 id; > > > > > + } cge; > > > > > + > > > > > + if (buf && ge) { > > > > > + cge = (struct compat_ge){ ge->timestamp, ge->id }; > > > > > + if (copy_to_user(buf, &cge, sizeof(cge))) > > > > > + return -EFAULT; > > > > > + } > > > > > + > > > > > + return sizeof(cge); > > > > > + } > > > > > +#endif > > > > > + > > > > > + if (buf && ge) { > > > > > + if (copy_to_user(buf, ge, sizeof(*ge))) > > > > > + return -EFAULT; > > > > > + } > > > > > + > > > > > + return sizeof(*ge); > > > > > +} > > > > > > > > The dual-purpose nature of this function makes it more complicated than > > > > it needs to be. > > > > I was going to suggest splitting it into separate functions, but... > > > > > > > > Given that struct compat_ge is a strict truncation of struct > > > > gpioevent_data, how about reducing this function to just determining the > > > > size of the event for user space, say lineevent_user_size(), and > > > > replacing sizeof(ge) with that calculcated size throughout > > > > lineevent_read()? > > > > > > So you mean something like > > > > > > struct compat_gpioeevent_data { > > > compat_u64 timestamp; > > > u32 id; > > > } __packed; > > > > > > #ifdef __x86_64__ > > > /* i386 has no padding after 'id' */ > > > size_t ge_size = in_ia32_syscall() ? sizeof(struct compat_gpioevent_data) : sizeof(struct gpioevent_data); > > > #else > > > size_t ge_size = sizeof(struct gpioevent_data) ; > > > #endif > > > > > > ? > > > > > > > Pretty much, though I was suggesting keeping it in a helper function, > > say lineevent_user_size(), not in lineevent_read() itself, to isolate > > all the ugliness in one small place. > > > > So in lineevent_read() you would: > > > > size_t ge_size = lineevent_user_size(); > > > > and then use that to replace all the sizeof(ge) occurrences. > > But in any case it makes code a bit hackish, no? > Maybe - I'm not totally happy about copying a partial struct either, but one way or the other that is what you effectively need to do. > We calculate the size of one structure and by the fact *partially* copy > another one. > Perhaps it is a matter of perspective - as I see it you are calculating the length of the event that the userspace expects, and then copying only that amount. As you say in the comment "i386 has no padding after 'id'" - other than that it is bitwise identical - the only difference is the length. > And actually if you look closer to the types, the compat_u64 is not the same as u64. > So, I'm not sure your solution would work in all cases. > There is only one corner case here - 32-bit on x86_64, and it works for that - I've tested it. For all other cases it falls back to the full length. For x86 the compat_u64 is: typedef u64 __attribute__((aligned(4))) compat_u64; which is bitwise identical - only allowed to 32-bit align. And if compat_u64 wasn't bitwise identical in this case then your original code wouldn't work either ;-). Cheers, Kent.
> +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge) > +{ > +#ifdef __x86_64__ I would make this "#ifdef CONFIG_IA32_COMPAT" to clarify what this is actually checking for. In theory we could add support for CONFIG_OABI_COMPAT here as well, not sure if there is a point. I recently came across a couple of things that all need the same hacks for arm-oabi and x86-32 in principle. > + /* i386 has no padding after 'id' */ > + if (in_ia32_syscall()) { > + struct compat_ge { > + compat_u64 timestamp __packed; No need for marking this __packed, it already is. > + u32 id; > + } cge; > + > + if (buf && ge) { I think I'd leave out the 'if()' checks here, and require the function to be called with valid pointers. It seems odd otherwise to return sizeof(cge) from the read() function without having written data. Note also that user space may pass a NULL pointer and should get back -EFAULT instead of 12 or 16. > - if (count < sizeof(ge)) > + /* When argument is NULL it returns size of the structure in user space */ > + ge_size = lineevent_to_user(NULL, NULL); > + if (count < ge_size) > return -EINVAL; Right, I see this is how it's being used, and I'd tend to agree with Kent: if you just determine the size dynamically and add a good comment, then the rest of the code gets simpler and more logical. Arnd
On Fri, Sep 11, 2020 at 05:28:46PM +0300, Andy Shevchenko wrote: > On Fri, Sep 11, 2020 at 06:17:14PM +0800, Kent Gibson wrote: > > On Fri, Sep 11, 2020 at 12:53:55PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote: > > > > On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote: > > > > > On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote: > > > > > > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: > > > [snip] > > > > typedef u64 __attribute__((aligned(4))) compat_u64; > > > > which is bitwise identical - only allowed to 32-bit align. > > Yes. That's what I meant under "not the same". > > As far as I understand the alignment makes sense if this type is a part of > the uAPI definition. But here we have it completely local. copy_to_user() takes > a pointer to a memory without any specific alignment implied. > > So, what you proposing is basically something like > > ret = copy_to_user(buf, &ge, compat ? sizeof(compat) : sizeof(ge)); > > Correct? > That isn't how I would write the copy_to_user(). The size would be calculated once, using the linevent_user_size() helper, with appropriate documentation as to why this is necessary, and then used throughout lineevent_read(). The documentation would mainly be on the lineevent_user_size() function itself. > I don't like the difference between 2nd and 3rd argument. This what looks to me > hackish. Variant with explicit compat structure I like more. > Agreed - writing it that way does look pretty nasty. But my suggestion is actually this: ret = copy_to_user(buf, &ge, event_size); I suggested ge_size previously, but event_size might help highlight that it isn't always sizeof(ge). > But if you think it's okay, I will update your way. > I would defer to Bart or Linus, but I think just calculating the appropriate size is preferable for this case. Cheers, Kent.
On Sat, Sep 12, 2020 at 4:21 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Sep 11, 2020 at 05:28:46PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 11, 2020 at 06:17:14PM +0800, Kent Gibson wrote: > > > On Fri, Sep 11, 2020 at 12:53:55PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 11, 2020 at 05:12:49PM +0800, Kent Gibson wrote: > > > > > On Fri, Sep 11, 2020 at 11:31:09AM +0300, Andy Shevchenko wrote: > > > > > > On Fri, Sep 11, 2020 at 11:05:39AM +0800, Kent Gibson wrote: > > > > > > > On Thu, Sep 10, 2020 at 01:19:34PM +0300, Andy Shevchenko wrote: > > > > > > [snip] > > > > > > typedef u64 __attribute__((aligned(4))) compat_u64; > > > > > > which is bitwise identical - only allowed to 32-bit align. > > > > Yes. That's what I meant under "not the same". > > > > As far as I understand the alignment makes sense if this type is a part of > > the uAPI definition. But here we have it completely local. copy_to_user() takes > > a pointer to a memory without any specific alignment implied. > > > > So, what you proposing is basically something like > > > > ret = copy_to_user(buf, &ge, compat ? sizeof(compat) : sizeof(ge)); > > > > Correct? > > > > That isn't how I would write the copy_to_user(). The size would be > calculated once, using the linevent_user_size() helper, with > appropriate documentation as to why this is necessary, and then > used throughout lineevent_read(). > > The documentation would mainly be on the lineevent_user_size() function > itself. > > > I don't like the difference between 2nd and 3rd argument. This what looks to me > > hackish. Variant with explicit compat structure I like more. > > > > Agreed - writing it that way does look pretty nasty. > > But my suggestion is actually this: > > ret = copy_to_user(buf, &ge, event_size); > > I suggested ge_size previously, but event_size might help highlight that > it isn't always sizeof(ge). > > > But if you think it's okay, I will update your way. > > > > I would defer to Bart or Linus, but I think just calculating the > appropriate size is preferable for this case. > > Cheers, > Kent. Kent has been producing clean and elegant code so far so I trust him on code quality issues TBH. Personally in this case however I'd go with an explicit compat structure as Andy prefers. I don't have a strong opinion on this so I really am fine either way. Bartosz
On Mon, Sep 14, 2020 at 4:00 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > On Sat, Sep 12, 2020 at 4:21 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Sep 11, 2020 at 05:28:46PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 11, 2020 at 06:17:14PM +0800, Kent Gibson wrote: ... > > > I don't like the difference between 2nd and 3rd argument. This what looks to me > > > hackish. Variant with explicit compat structure I like more. > > > > > > > Agreed - writing it that way does look pretty nasty. > > > > But my suggestion is actually this: > > > > ret = copy_to_user(buf, &ge, event_size); > > > > I suggested ge_size previously, but event_size might help highlight that > > it isn't always sizeof(ge). > > > > > But if you think it's okay, I will update your way. > > > > > > > I would defer to Bart or Linus, but I think just calculating the > > appropriate size is preferable for this case. > > > > Cheers, > > Kent. > > Kent has been producing clean and elegant code so far so I trust him > on code quality issues TBH. Personally in this case however I'd go > with an explicit compat structure as Andy prefers. > > I don't have a strong opinion on this so I really am fine either way. Since the initial idea was Arnd's and he agreed on Kent's approach, I will re-do that way.
On Fri, Sep 11, 2020 at 06:20:49PM +0200, Arnd Bergmann wrote: > > +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge) > > +{ > > +#ifdef __x86_64__ > > I would make this "#ifdef CONFIG_IA32_COMPAT" to clarify what this > is actually checking for. There is no such option available right now, I prefer to leave as is to make backporting easier. > In theory we could add support for > CONFIG_OABI_COMPAT here as well, not sure if there is a point. > I recently came across a couple of things that all need the same > hacks for arm-oabi and x86-32 in principle. > > > + /* i386 has no padding after 'id' */ > > + if (in_ia32_syscall()) { > > + struct compat_ge { > > + compat_u64 timestamp __packed; > > No need for marking this __packed, it already is. Yeah, due to a special alignment for compat_u64. I blindly copied from your proposal. > > + u32 id; > > + } cge; > > + > > + if (buf && ge) { > > I think I'd leave out the 'if()' checks here, and require the function > to be called with valid pointers. It seems odd otherwise to return > sizeof(cge) from the read() function without having written data. > > Note also that user space may pass a NULL pointer and should > get back -EFAULT instead of 12 or 16. OK! > > - if (count < sizeof(ge)) > > + /* When argument is NULL it returns size of the structure in user space */ > > + ge_size = lineevent_to_user(NULL, NULL); > > + if (count < ge_size) > > return -EINVAL; > > Right, I see this is how it's being used, and I'd tend to agree with Kent: > if you just determine the size dynamically and add a good comment, > then the rest of the code gets simpler and more logical. Okay, I will re-do this.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index e6c9b78adfc2..a6a8384c8255 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -423,6 +423,33 @@ static __poll_t lineevent_poll(struct file *file, return events; } +static ssize_t lineevent_to_user(char __user *buf, struct gpioevent_data *ge) +{ +#ifdef __x86_64__ + /* i386 has no padding after 'id' */ + if (in_ia32_syscall()) { + struct compat_ge { + compat_u64 timestamp __packed; + u32 id; + } cge; + + if (buf && ge) { + cge = (struct compat_ge){ ge->timestamp, ge->id }; + if (copy_to_user(buf, &cge, sizeof(cge))) + return -EFAULT; + } + + return sizeof(cge); + } +#endif + + if (buf && ge) { + if (copy_to_user(buf, ge, sizeof(*ge))) + return -EFAULT; + } + + return sizeof(*ge); +} static ssize_t lineevent_read(struct file *file, char __user *buf, @@ -432,9 +459,12 @@ static ssize_t lineevent_read(struct file *file, struct lineevent_state *le = file->private_data; struct gpioevent_data ge; ssize_t bytes_read = 0; + ssize_t ge_size; int ret; - if (count < sizeof(ge)) + /* When argument is NULL it returns size of the structure in user space */ + ge_size = lineevent_to_user(NULL, NULL); + if (count < ge_size) return -EINVAL; do { @@ -470,10 +500,11 @@ static ssize_t lineevent_read(struct file *file, break; } - if (copy_to_user(buf + bytes_read, &ge, sizeof(ge))) - return -EFAULT; - bytes_read += sizeof(ge); - } while (count >= bytes_read + sizeof(ge)); + ret = lineevent_to_user(buf + bytes_read, &ge); + if (ret < 0) + return ret; + bytes_read += ret; + } while (count >= bytes_read + ge_size); return bytes_read; }
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 introduce lineeven_to_user() helper which returns actual size of the structure and copies its content to user if asked. Fixes: 61f922db7221 ("gpio: userspace ABI for reading GPIO line events") Suggested-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-cdev.c | 41 ++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-)