Re: [PATCH v2 2/4] gpio: cdev: use correct pointer accessors with SRCU

[Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>]

On 14.02.2024 09:44, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-lkp/202402122234.d85cca9b-lkp@xxxxxxxxx
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>   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