| Message ID | 20230815131558.33787-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Fix NULL pointer dereference in RZ/{G2L,V2M,A2} pinctrl driver | expand |
Hi Biju, On Tue, Aug 15, 2023 at 3:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > Fix the below random NULL pointer crash during boot by serializing > pinctrl group and function creation/remove calls in > rzg2l_dt_subnode_to_map() with mutex lock. > > Crash logs: > pc : __pi_strcmp+0x20/0x140 > lr : pinmux_func_name_to_selector+0x68/0xa4 > Call trace: > __pi_strcmp+0x20/0x140 > pinmux_generic_add_function+0x34/0xcc > rzg2l_dt_subnode_to_map+0x314/0x44c > rzg2l_dt_node_to_map+0x164/0x194 > pinctrl_dt_to_map+0x218/0x37c > create_pinctrl+0x70/0x3d8 > > While at it, add comments for bitmap_lock and lock. > > Fixes: c4c4637eb57f ("pinctrl: renesas: Add RZ/G2L pin and gpio controller driver") > Cc: stable@kernel.org > Tested-by: Chris Paterson <Chris.Paterson2@renesas.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Added comment for bitmap_lock and lock. > * Moved map allocation down to reduce lock section. > * Added locks for maps and pinctrl group and function creation/remove > calls > * Added unlock_and_done label for lock error path. Thanks for the update! Upon closer look, I noticed that I had missed that the map allocation is not global, but local to a specific DT node, so it does not need protection by a lock. If no one objects, I will back out that change myself, queue this patch in renesas-pinctrl-for-v6.6, and send a PR tomorrow. Thanks! Gr{oetje,eeting}s, Geert
On Thu, Aug 17, 2023 at 2:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Aug 15, 2023 at 3:16 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Fix the below random NULL pointer crash during boot by serializing > > pinctrl group and function creation/remove calls in > > rzg2l_dt_subnode_to_map() with mutex lock. > > > > Crash logs: > > pc : __pi_strcmp+0x20/0x140 > > lr : pinmux_func_name_to_selector+0x68/0xa4 > > Call trace: > > __pi_strcmp+0x20/0x140 > > pinmux_generic_add_function+0x34/0xcc > > rzg2l_dt_subnode_to_map+0x314/0x44c > > rzg2l_dt_node_to_map+0x164/0x194 > > pinctrl_dt_to_map+0x218/0x37c > > create_pinctrl+0x70/0x3d8 > > > > While at it, add comments for bitmap_lock and lock. > > > > Fixes: c4c4637eb57f ("pinctrl: renesas: Add RZ/G2L pin and gpio controller driver") > > Cc: stable@kernel.org > > Tested-by: Chris Paterson <Chris.Paterson2@renesas.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v2->v3: > > * Added comment for bitmap_lock and lock. > > * Moved map allocation down to reduce lock section. > > * Added locks for maps and pinctrl group and function creation/remove > > calls > > * Added unlock_and_done label for lock error path. > > Thanks for the update! > > Upon closer look, I noticed that I had missed that the map allocation > is not global, but local to a specific DT node, so it does not > need protection by a lock. If no one objects, I will back out that > change myself, queue this patch in renesas-pinctrl-for-v6.6, and send > a PR tomorrow. Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Thu, Aug 17, 2023 at 2:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > need protection by a lock. If no one objects, I will back out that > change myself, queue this patch in renesas-pinctrl-for-v6.6, and send > a PR tomorrow. Shouldn't this even go in for v6.5? Or is it non-urgent? (Maybe I already asked, I have teflon-memory.) Yours, Linus Walleij
Hi Linus, On Thu, Aug 17, 2023 at 3:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Aug 17, 2023 at 2:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > need protection by a lock. If no one objects, I will back out that > > change myself, queue this patch in renesas-pinctrl-for-v6.6, and send > > a PR tomorrow. > > Shouldn't this even go in for v6.5? > Or is it non-urgent? > > (Maybe I already asked, I have teflon-memory.) If you're still taking fixes for v6.5, I can do that. Else, it will have to wait for a stable backport after v6.6-rc1. Thanks! Gr{oetje,eeting}s, Geert
Hi Linus, On Thu, Aug 17, 2023 at 3:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 17, 2023 at 3:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Aug 17, 2023 at 2:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > need protection by a lock. If no one objects, I will back out that > > > change myself, queue this patch in renesas-pinctrl-for-v6.6, and send > > > a PR tomorrow. > > > > Shouldn't this even go in for v6.5? > > Or is it non-urgent? > > > > (Maybe I already asked, I have teflon-memory.) > > If you're still taking fixes for v6.5, I can do that. > Else, it will have to wait for a stable backport after v6.6-rc1. IOW, please let me know if I should move these 3 commits to a fixes branch. BTW, they conflict with commit 060f03e95454a0f4 ("pinctrl: Explicitly include correct DT includes") in pinctrl/for-next... https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=renesas-pinctrl Thanks! Gr{oetje,eeting}s, Geert
On Thu, Aug 17, 2023 at 5:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 17, 2023 at 3:54 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Thu, Aug 17, 2023 at 3:38 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > On Thu, Aug 17, 2023 at 2:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > need protection by a lock. If no one objects, I will back out that > > > > change myself, queue this patch in renesas-pinctrl-for-v6.6, and send > > > > a PR tomorrow. > > > > > > Shouldn't this even go in for v6.5? > > > Or is it non-urgent? > > > > > > (Maybe I already asked, I have teflon-memory.) > > > > If you're still taking fixes for v6.5, I can do that. > > Else, it will have to wait for a stable backport after v6.6-rc1. > > IOW, please let me know if I should move these 3 commits to a fixes > branch. I'd say that is up to the driver maintainer, hehe :D I can only determine if changes to the core or my own drivers are urgent. I am hopefully sending some fixes today but if more urgent stuff need to go in, such is life. > BTW, they conflict with commit 060f03e95454a0f4 ("pinctrl: > Explicitly include correct DT includes") in pinctrl/for-next... Yeah, we'll figure it out... I think. Worst case we toss some merge conflicts at Torvalds. Yours, Linus Walleij
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index 4f34f8f24bde..571a32d2f697 100644 --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c @@ -11,6 +11,7 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/of.h> #include <linux/of_irq.h> #include <linux/platform_device.h> @@ -149,10 +150,11 @@ struct rzg2l_pinctrl { struct gpio_chip gpio_chip; struct pinctrl_gpio_range gpio_range; DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT); - spinlock_t bitmap_lock; + spinlock_t bitmap_lock; /* protect tint_slot bitmap */ unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; - spinlock_t lock; + spinlock_t lock; /* lock read/write registers */ + struct mutex mutex; /* serialize adding groups and functions */ }; static const unsigned int iolh_groupa_mA[] = { 2, 4, 8, 12 }; @@ -309,28 +311,6 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, if (num_pins) nmaps += num_pins; - maps = krealloc_array(maps, nmaps, sizeof(*maps), GFP_KERNEL); - if (!maps) { - ret = -ENOMEM; - goto done; - } - - *map = maps; - *num_maps = nmaps; - if (num_pins) { - of_property_for_each_string(np, "pins", prop, pin) { - ret = rzg2l_map_add_config(&maps[idx], pin, - PIN_MAP_TYPE_CONFIGS_PIN, - configs, num_configs); - if (ret < 0) - goto done; - - idx++; - } - ret = 0; - goto done; - } - pins = devm_kcalloc(pctrl->dev, num_pinmux, sizeof(*pins), GFP_KERNEL); psel_val = devm_kcalloc(pctrl->dev, num_pinmux, sizeof(*psel_val), GFP_KERNEL); @@ -362,11 +342,34 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, name = np->name; } + mutex_lock(&pctrl->mutex); + maps = krealloc_array(maps, nmaps, sizeof(*maps), GFP_KERNEL); + if (!maps) { + ret = -ENOMEM; + goto unlock_and_done; + } + + *map = maps; + *num_maps = nmaps; + if (num_pins) { + of_property_for_each_string(np, "pins", prop, pin) { + ret = rzg2l_map_add_config(&maps[idx], pin, + PIN_MAP_TYPE_CONFIGS_PIN, + configs, num_configs); + if (ret < 0) + goto unlock_and_done; + + idx++; + } + ret = 0; + goto unlock_and_done; + } + /* Register a single pin group listing all the pins we read from DT */ gsel = pinctrl_generic_add_group(pctldev, name, pins, num_pinmux, NULL); if (gsel < 0) { ret = gsel; - goto done; + goto unlock_and_done; } /* @@ -380,6 +383,8 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, goto remove_group; } + mutex_unlock(&pctrl->mutex); + maps[idx].type = PIN_MAP_TYPE_MUX_GROUP; maps[idx].data.mux.group = name; maps[idx].data.mux.function = name; @@ -391,6 +396,8 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, remove_group: pinctrl_generic_remove_group(pctldev, gsel); +unlock_and_done: + mutex_unlock(&pctrl->mutex); done: *index = idx; kfree(configs); @@ -1503,6 +1510,7 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev) spin_lock_init(&pctrl->lock); spin_lock_init(&pctrl->bitmap_lock); + mutex_init(&pctrl->mutex); platform_set_drvdata(pdev, pctrl);
Fix the below random NULL pointer crash during boot by serializing pinctrl group and function creation/remove calls in rzg2l_dt_subnode_to_map() with mutex lock. Crash logs: pc : __pi_strcmp+0x20/0x140 lr : pinmux_func_name_to_selector+0x68/0xa4 Call trace: __pi_strcmp+0x20/0x140 pinmux_generic_add_function+0x34/0xcc rzg2l_dt_subnode_to_map+0x314/0x44c rzg2l_dt_node_to_map+0x164/0x194 pinctrl_dt_to_map+0x218/0x37c create_pinctrl+0x70/0x3d8 While at it, add comments for bitmap_lock and lock. Fixes: c4c4637eb57f ("pinctrl: renesas: Add RZ/G2L pin and gpio controller driver") Cc: stable@kernel.org Tested-by: Chris Paterson <Chris.Paterson2@renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v2->v3: * Added comment for bitmap_lock and lock. * Moved map allocation down to reduce lock section. * Added locks for maps and pinctrl group and function creation/remove calls * Added unlock_and_done label for lock error path. v1->v2: * Updated crash logs as per submitting patches doc. --- drivers/pinctrl/renesas/pinctrl-rzg2l.c | 58 ++++++++++++++----------- 1 file changed, 33 insertions(+), 25 deletions(-)