* [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
@ 2020-09-10 10:19 Andy Shevchenko
2020-09-10 10:19 ` [PATCH v1 2/2] gpiolib: convert to use DEFINE_SEQ_ATTRIBUTE macro Andy Shevchenko
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-10 10:19 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Kent Gibson
Cc: Andy Shevchenko, Arnd Bergmann
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(-)
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;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 2/2] gpiolib: convert to use DEFINE_SEQ_ATTRIBUTE macro
2020-09-10 10:19 [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode Andy Shevchenko
@ 2020-09-10 10:19 ` Andy Shevchenko
2020-09-11 15:45 ` Bartosz Golaszewski
2020-09-11 1:37 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-10 10:19 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, linux-gpio, Kent Gibson
Cc: Andy Shevchenko
Use DEFINE_SEQ_ATTRIBUTE macro to simplify the code.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib.c | 19 +++----------------
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 80137c1b3cdc..1300650dc308 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4402,31 +4402,18 @@ static int gpiolib_seq_show(struct seq_file *s, void *v)
return 0;
}
-static const struct seq_operations gpiolib_seq_ops = {
+static const struct seq_operations gpiolib_sops = {
.start = gpiolib_seq_start,
.next = gpiolib_seq_next,
.stop = gpiolib_seq_stop,
.show = gpiolib_seq_show,
};
-
-static int gpiolib_open(struct inode *inode, struct file *file)
-{
- return seq_open(file, &gpiolib_seq_ops);
-}
-
-static const struct file_operations gpiolib_operations = {
- .owner = THIS_MODULE,
- .open = gpiolib_open,
- .read = seq_read,
- .llseek = seq_lseek,
- .release = seq_release,
-};
+DEFINE_SEQ_ATTRIBUTE(gpiolib);
static int __init gpiolib_debugfs_init(void)
{
/* /sys/kernel/debug/gpio */
- debugfs_create_file("gpio", S_IFREG | S_IRUGO, NULL, NULL,
- &gpiolib_operations);
+ debugfs_create_file("gpio", 0444, NULL, NULL, &gpiolib_fops);
return 0;
}
subsys_initcall(gpiolib_debugfs_init);
--
2.28.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-10 10:19 [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode Andy Shevchenko
@ 2020-09-11 1:37 ` kernel test robot
2020-09-11 1:37 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-09-11 1:37 UTC (permalink / raw)
To: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, linux-gpio,
Kent Gibson
Cc: kbuild-all, Andy Shevchenko, Arnd Bergmann
[-- Attachment #1: Type: text/plain, Size: 12603 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28892 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
@ 2020-09-11 1:37 ` kernel test robot
0 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-09-11 1:37 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 12833 bytes --]
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(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28892 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-10 10:19 [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode Andy Shevchenko
2020-09-10 10:19 ` [PATCH v1 2/2] gpiolib: convert to use DEFINE_SEQ_ATTRIBUTE macro Andy Shevchenko
2020-09-11 1:37 ` kernel test robot
@ 2020-09-11 3:05 ` Kent Gibson
2020-09-11 8:31 ` Andy Shevchenko
2020-09-11 16:20 ` Arnd Bergmann
3 siblings, 1 reply; 15+ messages in thread
From: Kent Gibson @ 2020-09-11 3:05 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Arnd Bergmann
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-11 3:05 ` Kent Gibson
@ 2020-09-11 8:31 ` Andy Shevchenko
2020-09-11 9:12 ` Kent Gibson
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-11 8:31 UTC (permalink / raw)
To: Kent Gibson; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Arnd Bergmann
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-11 8:31 ` Andy Shevchenko
@ 2020-09-11 9:12 ` Kent Gibson
2020-09-11 9:53 ` Andy Shevchenko
0 siblings, 1 reply; 15+ messages in thread
From: Kent Gibson @ 2020-09-11 9:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Arnd Bergmann
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-11 9:12 ` Kent Gibson
@ 2020-09-11 9:53 ` Andy Shevchenko
2020-09-11 10:17 ` Kent Gibson
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-11 9:53 UTC (permalink / raw)
To: Kent Gibson; +Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Arnd Bergmann
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-11 9:53 ` Andy Shevchenko
@ 2020-09-11 10:17 ` Kent Gibson
[not found] ` <20200911142846.GM1891694@smile.fi.intel.com>
0 siblings, 1 reply; 15+ messages in thread
From: Kent Gibson @ 2020-09-11 10:17 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Arnd Bergmann
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/2] gpiolib: convert to use DEFINE_SEQ_ATTRIBUTE macro
2020-09-10 10:19 ` [PATCH v1 2/2] gpiolib: convert to use DEFINE_SEQ_ATTRIBUTE macro Andy Shevchenko
@ 2020-09-11 15:45 ` Bartosz Golaszewski
0 siblings, 0 replies; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-09-11 15:45 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, linux-gpio, Kent Gibson
On Thu, Sep 10, 2020 at 12:19 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Use DEFINE_SEQ_ATTRIBUTE macro to simplify the code.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Queued for next, thanks!
Bartosz
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-10 10:19 [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode Andy Shevchenko
` (2 preceding siblings ...)
2020-09-11 3:05 ` Kent Gibson
@ 2020-09-11 16:20 ` Arnd Bergmann
2020-09-14 14:26 ` Andy Shevchenko
3 siblings, 1 reply; 15+ messages in thread
From: Arnd Bergmann @ 2020-09-11 16:20 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
Kent Gibson
> +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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
[not found] ` <20200911142846.GM1891694@smile.fi.intel.com>
@ 2020-09-12 2:21 ` Kent Gibson
2020-09-14 12:44 ` Bartosz Golaszewski
0 siblings, 1 reply; 15+ messages in thread
From: Kent Gibson @ 2020-09-12 2:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, Arnd Bergmann
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-12 2:21 ` Kent Gibson
@ 2020-09-14 12:44 ` Bartosz Golaszewski
2020-09-14 13:04 ` Andy Shevchenko
0 siblings, 1 reply; 15+ messages in thread
From: Bartosz Golaszewski @ 2020-09-14 12:44 UTC (permalink / raw)
To: Kent Gibson; +Cc: Andy Shevchenko, Linus Walleij, linux-gpio, Arnd Bergmann
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-14 12:44 ` Bartosz Golaszewski
@ 2020-09-14 13:04 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-14 13:04 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Kent Gibson, Andy Shevchenko, Linus Walleij, linux-gpio, Arnd Bergmann
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode
2020-09-11 16:20 ` Arnd Bergmann
@ 2020-09-14 14:26 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-09-14 14:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Walleij, Bartosz Golaszewski, open list:GPIO SUBSYSTEM,
Kent Gibson
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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-09-14 14:26 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 10:19 [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode Andy Shevchenko
2020-09-10 10:19 ` [PATCH v1 2/2] gpiolib: convert to use DEFINE_SEQ_ATTRIBUTE macro Andy Shevchenko
2020-09-11 15:45 ` Bartosz Golaszewski
2020-09-11 1:37 ` [PATCH v1 1/2] gpiolib: Fix line event handling in syscall compatible mode kernel test robot
2020-09-11 1:37 ` kernel test robot
2020-09-11 3:05 ` Kent Gibson
2020-09-11 8:31 ` Andy Shevchenko
2020-09-11 9:12 ` Kent Gibson
2020-09-11 9:53 ` Andy Shevchenko
2020-09-11 10:17 ` Kent Gibson
[not found] ` <20200911142846.GM1891694@smile.fi.intel.com>
2020-09-12 2:21 ` Kent Gibson
2020-09-14 12:44 ` Bartosz Golaszewski
2020-09-14 13:04 ` Andy Shevchenko
2020-09-11 16:20 ` Arnd Bergmann
2020-09-14 14:26 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.