| Message ID | 20250210-gpio-sanitize-retvals-v1-8-12ea88506cb2@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: sanitize return values of callbacks | expand |
On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > As per the API contract, the get_direction() callback can only > return 0, 1 or a negative error number. Add a wrapper around the callback > calls that filters out anything else. ... > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + int ret; > + > + lockdep_assert_held(&gc->gpiodev->srcu); > + > + if (WARN_ON(!gc->get_direction)) > + return -EOPNOTSUPP; > + > + ret = gc->get_direction(gc, offset); > + if (ret > 1) Would it be better to use the respective GPIO*... macro instead of 1? > + ret = -EBADE; > + > + return ret; > +}
On Mon, Feb 24, 2025 at 5:33 PM Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > As per the API contract, the get_direction() callback can only > > return 0, 1 or a negative error number. Add a wrapper around the callback > > calls that filters out anything else. > > ... > > > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset) > > +{ > > + int ret; > > + > > + lockdep_assert_held(&gc->gpiodev->srcu); > > + > > + if (WARN_ON(!gc->get_direction)) > > + return -EOPNOTSUPP; > > + > > + ret = gc->get_direction(gc, offset); > > + if (ret > 1) > > Would it be better to use the respective GPIO*... macro instead of 1? > I did consider it but I don't like comparing against enums, it doesn't feel right as the value behind the name can change. I think I prefer it like this even if it's not the best solution either. Maybe we could be more explicit and say: if (!(ret == IN || ret == OUT || ret < 0) ? Bart
Mon, Feb 24, 2025 at 08:55:26PM +0100, Bartosz Golaszewski kirjoitti: > On Mon, Feb 24, 2025 at 5:33 PM Andy Shevchenko > <andriy.shevchenko@intel.com> wrote: > > On Mon, Feb 10, 2025 at 11:52:02AM +0100, Bartosz Golaszewski wrote: ... > > > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset) > > > +{ > > > + int ret; > > > + > > > + lockdep_assert_held(&gc->gpiodev->srcu); > > > + > > > + if (WARN_ON(!gc->get_direction)) > > > + return -EOPNOTSUPP; > > > + > > > + ret = gc->get_direction(gc, offset); > > > + if (ret > 1) > > > > Would it be better to use the respective GPIO*... macro instead of 1? > > > > I did consider it but I don't like comparing against enums, it doesn't > feel right as the value behind the name can change. I think I prefer > it like this even if it's not the best solution either. Maybe we could > be more explicit and say: > > if (!(ret == IN || ret == OUT || ret < 0) > > ? Yep, I like this.
On 10.02.2025 11:52, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > As per the API contract, the get_direction() callback can only > return 0, 1 or a negative error number. Add a wrapper around the callback > calls that filters out anything else. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> This patch landed in today's linux-next as commit e623c4303ed11 ("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It introduced a lockdep warning from the gpiochip_get_direction() function. IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu locking/annotation for the newly created gpio chip. Here is the log: gpio gpiochip1: Static allocation of GPIO base is deprecated, use dynamic allocation. ------------[ cut here ]------------ WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349 gpiochip_get_direction+0x48/0x66 Modules linked in: cdns_usb_common roles cdns3_starfive snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp jh7110_trng sfctemp dwmac_starfive stmmac_platform spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022 phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W 6.14.0-rc4-next-20250225 #1054 Tainted: [W]=WARN Hardware name: StarFive VisionFive 2 v1.2A (DT) Workqueue: events_unbound deferred_probe_work_func epc : gpiochip_get_direction+0x48/0x66 ra : gpiochip_get_direction+0x46/0x66 ... [<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66 [<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2 [<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a [<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa [<ffffffff80731116>] platform_probe+0x4e/0x92 [<ffffffff8000c366>] really_probe+0x10a/0x2de [<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0 [<ffffffff8072ee34>] driver_probe_device+0x78/0xc4 [<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6 [<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0 [<ffffffff8072f33e>] __device_attach+0x84/0x13c [<ffffffff8072f55e>] device_initial_probe+0xe/0x16 [<ffffffff8072e002>] bus_probe_device+0x88/0x8a [<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee [<ffffffff80047b7e>] process_one_work+0x1d0/0x57a [<ffffffff8004854e>] worker_thread+0x166/0x2cc [<ffffffff80051568>] kthread+0xdc/0x1b4 [<ffffffff80bcb942>] ret_from_fork+0xe/0x18 irq event stamp: 17857 hardirqs last enabled at (17857): [<ffffffff80bca986>] _raw_spin_unlock_irqrestore+0x4c/0x4e hardirqs last disabled at (17856): [<ffffffff80bca73c>] _raw_spin_lock_irqsave+0x5e/0x64 softirqs last enabled at (17322): [<ffffffff80adff1a>] inet6_fill_ifla6_attrs+0x3d0/0x420 softirqs last disabled at (17320): [<ffffffff80adfefe>] inet6_fill_ifla6_attrs+0x3b4/0x420 ---[ end trace 0000000000000000 ]--- > --- > drivers/gpio/gpiolib.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 683a03d237c0..7f2aca9f81a1 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -341,6 +341,22 @@ static int gpiochip_find_base_unlocked(u16 ngpio) > } > } > > +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + int ret; > + > + lockdep_assert_held(&gc->gpiodev->srcu); > + > + if (WARN_ON(!gc->get_direction)) > + return -EOPNOTSUPP; > + > + ret = gc->get_direction(gc, offset); > + if (ret > 1) > + ret = -EBADE; > + > + return ret; > +} > + > /** > * gpiod_get_direction - return the current direction of a GPIO > * @desc: GPIO to get the direction of > @@ -381,7 +397,7 @@ int gpiod_get_direction(struct gpio_desc *desc) > if (!guard.gc->get_direction) > return -ENOTSUPP; > > - ret = guard.gc->get_direction(guard.gc, offset); > + ret = gpiochip_get_direction(guard.gc, offset); > if (ret < 0) > return ret; > > @@ -1057,7 +1073,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > desc->gdev = gdev; > > if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) { > - ret = gc->get_direction(gc, desc_index); > + ret = gpiochip_get_direction(gc, desc_index); > if (ret < 0) > goto err_cleanup_desc_srcu; > > @@ -2770,8 +2786,7 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc) > ret = gpiochip_direction_input(guard.gc, > gpio_chip_hwgpio(desc)); > } else if (guard.gc->get_direction) { > - ret = guard.gc->get_direction(guard.gc, > - gpio_chip_hwgpio(desc)); > + ret = gpiochip_get_direction(guard.gc, gpio_chip_hwgpio(desc)); > if (ret < 0) > return ret; > > @@ -2818,8 +2833,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value) > } else { > /* Check that we are in output mode if we can */ > if (guard.gc->get_direction) { > - ret = guard.gc->get_direction(guard.gc, > - gpio_chip_hwgpio(desc)); > + ret = gpiochip_get_direction(guard.gc, > + gpio_chip_hwgpio(desc)); > if (ret < 0) > return ret; > > Best regards
On Tue, Feb 25, 2025 at 11:13 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 10.02.2025 11:52, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > As per the API contract, the get_direction() callback can only > > return 0, 1 or a negative error number. Add a wrapper around the callback > > calls that filters out anything else. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > This patch landed in today's linux-next as commit e623c4303ed11 > ("gpiolib: sanitize the return value of gpio_chip::get_direction()"). It > introduced a lockdep warning from the gpiochip_get_direction() function. > IMHO it looks that gpiochip_add_data_with_key() lacks proper srcu > locking/annotation for the newly created gpio chip. Here is the log: > > gpio gpiochip1: Static allocation of GPIO base is deprecated, use > dynamic allocation. > ------------[ cut here ]------------ > WARNING: CPU: 2 PID: 35 at drivers/gpio/gpiolib.c:349 > gpiochip_get_direction+0x48/0x66 > Modules linked in: cdns_usb_common roles cdns3_starfive > snd_soc_simple_card snd_soc_simple_card_utils phy_jh7110_dphy_rx > clk_starfive_jh7110_vout pcie_starfive(+) clk_starfive_jh7110_isp > jh7110_trng sfctemp dwmac_starfive stmmac_platform > spi_cadence_quadspi(+) clk_starfive_jh7110_stg stmmac > clk_starfive_jh7110_aon jh7110_pwmdac pcs_xpcs phy_jh7110_usb spi_pl022 > phy_jh7110_pcie snd_soc_spdif_tx i2c_dev drm > drm_panel_orientation_quirks backlight dm_mod configfs ip_tables x_tables > CPU: 2 UID: 0 PID: 35 Comm: kworker/u18:0 Tainted: G W > 6.14.0-rc4-next-20250225 #1054 > Tainted: [W]=WARN > Hardware name: StarFive VisionFive 2 v1.2A (DT) > Workqueue: events_unbound deferred_probe_work_func > epc : gpiochip_get_direction+0x48/0x66 > ra : gpiochip_get_direction+0x46/0x66 > ... > [<ffffffff805fc72c>] gpiochip_get_direction+0x48/0x66 > [<ffffffff80603a14>] gpiochip_add_data_with_key+0x74a/0xde2 > [<ffffffff806044e6>] devm_gpiochip_add_data_with_key+0x1e/0x5a > [<ffffffff805f8738>] jh7110_pinctrl_probe+0x298/0x3aa > [<ffffffff80731116>] platform_probe+0x4e/0x92 > [<ffffffff8000c366>] really_probe+0x10a/0x2de > [<ffffffff8000c5e4>] __driver_probe_device.part.0+0xaa/0xe0 > [<ffffffff8072ee34>] driver_probe_device+0x78/0xc4 > [<ffffffff8072eee6>] __device_attach_driver+0x66/0xc6 > [<ffffffff8072d0b0>] bus_for_each_drv+0x5c/0xb0 > [<ffffffff8072f33e>] __device_attach+0x84/0x13c > [<ffffffff8072f55e>] device_initial_probe+0xe/0x16 > [<ffffffff8072e002>] bus_probe_device+0x88/0x8a > [<ffffffff8072e516>] deferred_probe_work_func+0xd4/0xee > [<ffffffff80047b7e>] process_one_work+0x1d0/0x57a > [<ffffffff8004854e>] worker_thread+0x166/0x2cc > [<ffffffff80051568>] kthread+0xdc/0x1b4 > [<ffffffff80bcb942>] ret_from_fork+0xe/0x18 > irq event stamp: 17857 > hardirqs last enabled at (17857): [<ffffffff80bca986>] > _raw_spin_unlock_irqrestore+0x4c/0x4e > hardirqs last disabled at (17856): [<ffffffff80bca73c>] > _raw_spin_lock_irqsave+0x5e/0x64 > softirqs last enabled at (17322): [<ffffffff80adff1a>] > inet6_fill_ifla6_attrs+0x3d0/0x420 > softirqs last disabled at (17320): [<ffffffff80adfefe>] > inet6_fill_ifla6_attrs+0x3b4/0x420 > ---[ end trace 0000000000000000 ]--- > Thanks for the report. We don't need to hold the SRCU when registering the chip. I'm now thinking I should revert using gpiochip_get_direction() in gpiochip_add_data_with_key() to directly calling the get_direction() callback and not checking its return value. Bartosz
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 683a03d237c0..7f2aca9f81a1 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -341,6 +341,22 @@ static int gpiochip_find_base_unlocked(u16 ngpio) } } +static int gpiochip_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + int ret; + + lockdep_assert_held(&gc->gpiodev->srcu); + + if (WARN_ON(!gc->get_direction)) + return -EOPNOTSUPP; + + ret = gc->get_direction(gc, offset); + if (ret > 1) + ret = -EBADE; + + return ret; +} + /** * gpiod_get_direction - return the current direction of a GPIO * @desc: GPIO to get the direction of @@ -381,7 +397,7 @@ int gpiod_get_direction(struct gpio_desc *desc) if (!guard.gc->get_direction) return -ENOTSUPP; - ret = guard.gc->get_direction(guard.gc, offset); + ret = gpiochip_get_direction(guard.gc, offset); if (ret < 0) return ret; @@ -1057,7 +1073,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, desc->gdev = gdev; if (gc->get_direction && gpiochip_line_is_valid(gc, desc_index)) { - ret = gc->get_direction(gc, desc_index); + ret = gpiochip_get_direction(gc, desc_index); if (ret < 0) goto err_cleanup_desc_srcu; @@ -2770,8 +2786,7 @@ int gpiod_direction_input_nonotify(struct gpio_desc *desc) ret = gpiochip_direction_input(guard.gc, gpio_chip_hwgpio(desc)); } else if (guard.gc->get_direction) { - ret = guard.gc->get_direction(guard.gc, - gpio_chip_hwgpio(desc)); + ret = gpiochip_get_direction(guard.gc, gpio_chip_hwgpio(desc)); if (ret < 0) return ret; @@ -2818,8 +2833,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value) } else { /* Check that we are in output mode if we can */ if (guard.gc->get_direction) { - ret = guard.gc->get_direction(guard.gc, - gpio_chip_hwgpio(desc)); + ret = gpiochip_get_direction(guard.gc, + gpio_chip_hwgpio(desc)); if (ret < 0) return ret;