[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <179caa10-5f86-4707-8bb0-fe1b316326d6@samsung.com>
Date: Fri, 16 Feb 2024 11:31:03 +0100
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Bartosz Golaszewski <brgl@...ev.pl>, Linus Walleij
<linus.walleij@...aro.org>, Kent Gibson <warthog618@...il.com>, Alex Elder
<elder@...aro.org>, Geert Uytterhoeven <geert+renesas@...der.be>, "Paul E .
McKenney" <paulmck@...nel.org>, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com>, Wolfram Sang <wsa@...-dreams.de>, Mark
Brown <broonie@...nel.org>, Dan Carpenter <dan.carpenter@...aro.org>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, Bartosz
Golaszewski <bartosz.golaszewski@...aro.org>, kernel test robot
<oliver.sang@...el.com>
Subject: Re: [PATCH v2 2/4] gpio: cdev: use correct pointer accessors with
SRCU
On 14.02.2024 09:44, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
>
> We never dereference the chip pointer in character device code so we can
> use the lighter rcu_access_pointer() helper. This also makes lockep
> happier as it no longer complains about suspicious rcu_dereference()
> usage.
>
> Fixes: d83cee3d2bb1 ("gpio: protect the pointer to gpio_chip in gpio_device with SRCU")
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@intel.com
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> drivers/gpio/gpiolib-cdev.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 9323b357df43..85037fa4925e 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -206,7 +206,7 @@ static long linehandle_ioctl(struct file *file, unsigned int cmd,
>
> guard(srcu)(&lh->gdev->srcu);
>
> - if (!rcu_dereference(lh->gdev->chip))
> + if (!rcu_access_pointer(lh->gdev->chip))
> return -ENODEV;
>
> switch (cmd) {
> @@ -1521,7 +1521,7 @@ static long linereq_ioctl(struct file *file, unsigned int cmd,
>
> guard(srcu)(&lr->gdev->srcu);
>
> - if (!rcu_dereference(lr->gdev->chip))
> + if (!rcu_access_pointer(lr->gdev->chip))
> return -ENODEV;
>
> switch (cmd) {
> @@ -1552,7 +1552,7 @@ static __poll_t linereq_poll(struct file *file,
>
> guard(srcu)(&lr->gdev->srcu);
>
> - if (!rcu_dereference(lr->gdev->chip))
> + if (!rcu_access_pointer(lr->gdev->chip))
> return EPOLLHUP | EPOLLERR;
>
> poll_wait(file, &lr->wait, wait);
> @@ -1574,7 +1574,7 @@ static ssize_t linereq_read(struct file *file, char __user *buf,
>
> guard(srcu)(&lr->gdev->srcu);
>
> - if (!rcu_dereference(lr->gdev->chip))
> + if (!rcu_access_pointer(lr->gdev->chip))
> return -ENODEV;
>
> if (count < sizeof(le))
> @@ -1875,7 +1875,7 @@ static __poll_t lineevent_poll(struct file *file,
>
> guard(srcu)(&le->gdev->srcu);
>
> - if (!rcu_dereference(le->gdev->chip))
> + if (!rcu_access_pointer(le->gdev->chip))
> return EPOLLHUP | EPOLLERR;
>
> poll_wait(file, &le->wait, wait);
> @@ -1913,7 +1913,7 @@ static ssize_t lineevent_read(struct file *file, char __user *buf,
>
> guard(srcu)(&le->gdev->srcu);
>
> - if (!rcu_dereference(le->gdev->chip))
> + if (!rcu_access_pointer(le->gdev->chip))
> return -ENODEV;
>
> /*
> @@ -1996,7 +1996,7 @@ static long lineevent_ioctl(struct file *file, unsigned int cmd,
>
> guard(srcu)(&le->gdev->srcu);
>
> - if (!rcu_dereference(le->gdev->chip))
> + if (!rcu_access_pointer(le->gdev->chip))
> return -ENODEV;
>
> /*
> @@ -2510,7 +2510,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> guard(srcu)(&gdev->srcu);
>
> /* We fail any subsequent ioctl():s when the chip is gone */
> - if (!rcu_dereference(gdev->chip))
> + if (!rcu_access_pointer(gdev->chip))
> return -ENODEV;
>
> /* Fill in the struct and pass to userspace */
> @@ -2595,7 +2595,7 @@ static __poll_t lineinfo_watch_poll(struct file *file,
>
> guard(srcu)(&cdev->gdev->srcu);
>
> - if (!rcu_dereference(cdev->gdev->chip))
> + if (!rcu_access_pointer(cdev->gdev->chip))
> return EPOLLHUP | EPOLLERR;
>
> poll_wait(file, &cdev->wait, pollt);
> @@ -2618,7 +2618,7 @@ static ssize_t lineinfo_watch_read(struct file *file, char __user *buf,
>
> guard(srcu)(&cdev->gdev->srcu);
>
> - if (!rcu_dereference(cdev->gdev->chip))
> + if (!rcu_access_pointer(cdev->gdev->chip))
> return -ENODEV;
>
> #ifndef CONFIG_GPIO_CDEV_V1
> @@ -2696,7 +2696,7 @@ static int gpio_chrdev_open(struct inode *inode, struct file *file)
> guard(srcu)(&gdev->srcu);
>
> /* Fail on open if the backing gpiochip is gone */
> - if (!rcu_dereference(gdev->chip))
> + if (!rcu_access_pointer(gdev->chip))
> return -ENODEV;
>
> cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
> @@ -2796,8 +2796,7 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt)
>
> guard(srcu)(&gdev->srcu);
>
> - gc = rcu_dereference(gdev->chip);
> - if (!gc)
> + if (!rcu_access_pointer(gdev->chip))
> return -ENODEV;
>
> chip_dbg(gc, "added GPIO chardev (%d:%d)\n", MAJOR(devt), gdev->id);
Here 'gc' is left uninitialized and nukes if GPIO DEBUGs are enabled.
Here is an example (captured on today's linux-next):
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address
00000000 when read
[00000000] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc4-next-20240216 #8096
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at gpiolib_cdev_register+0xd4/0x170
LR is at chainhash_table+0x784/0x20000
pc : [<c05dbe54>] lr : [<c18bb74c>] psr: 20000013
..
Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 4000404a DAC: 00000051
Register r0 information: non-slab/vmalloc memory
Register r1 information: NULL pointer
Register r2 information: non-paged memory
Register r3 information: non-paged memory
Register r4 information: slab kmalloc-1k start c1e5f800 pointer offset 0
size 1024
Register r5 information: NULL pointer
Register r6 information: non-paged memory
Register r7 information: slab kmalloc-1k start c1e5f800 pointer offset
952 size 1024
Register r8 information: NULL pointer
Register r9 information: slab kmalloc-1k start c1e5f800 pointer offset
960 size 1024
Register r10 information: non-paged memory
Register r11 information: non-slab/vmalloc memory
Register r12 information: NULL pointer
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xf082db90 to 0xf082e000)
..
gpiolib_cdev_register from gpiochip_setup_dev+0x44/0xb0
gpiochip_setup_dev from gpiochip_add_data_with_key+0x9ac/0xaac
gpiochip_add_data_with_key from devm_gpiochip_add_data_with_key+0x20/0x5c
devm_gpiochip_add_data_with_key from samsung_pinctrl_probe+0x938/0xb18
samsung_pinctrl_probe from platform_probe+0x5c/0xb8
platform_probe from really_probe+0xe0/0x400
really_probe from __driver_probe_device+0x9c/0x1f4
__driver_probe_device from driver_probe_device+0x30/0xc0
driver_probe_device from __device_attach_driver+0xa8/0x120
__device_attach_driver from bus_for_each_drv+0x80/0xcc
bus_for_each_drv from __device_attach+0xac/0x1fc
__device_attach from bus_probe_device+0x8c/0x90
bus_probe_device from device_add+0x5d4/0x7fc
device_add from of_platform_device_create_pdata+0x94/0xc4
of_platform_device_create_pdata from of_platform_bus_create+0x1f8/0x4c0
of_platform_bus_create from of_platform_bus_create+0x268/0x4c0
of_platform_bus_create from of_platform_populate+0x80/0x110
of_platform_populate from of_platform_default_populate_init+0xd4/0xec
of_platform_default_populate_init from do_one_initcall+0x64/0x2fc
do_one_initcall from kernel_init_freeable+0x1c4/0x228
kernel_init_freeable from kernel_init+0x1c/0x12c
kernel_init from ret_from_fork+0x14/0x28
Exception stack(0xf082dfb0 to 0xf082dff8)
..
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
---[ end Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b ]---
Probably the easiest way to fix this issue is to replace chip_dbg with
the following dev_dbg() call:
dev_dbg(&gdev->dev, "(%s): added GPIO chardev (%d:%d)\n", gdev->label,
MAJOR(devt), gdev->id);
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Powered by blists - more mailing lists