On 14.02.2024 09:44, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> Closes: https://lore.kernel.org/oe-lkp/[email protected]
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> ---
> 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