| Message ID | 20230815075602.10473-2-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 9:56 AM 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 > > 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> > --- > v1->v2: > * Updated crash logs as per submitting patches doc. Thanks for your patch! > --- 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> > @@ -153,6 +154,7 @@ struct rzg2l_pinctrl { > unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; > > spinlock_t lock; > + struct mutex mutex; /* serialize adding groups and functions */ I guess it's not too late to add comments to the two other locks (bitmap_lock and lock)? > }; > > static const unsigned int iolh_groupa_mA[] = { 2, 4, 8, 12 }; > @@ -362,10 +364,12 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, > name = np->name; > } > > + mutex_lock(&pctrl->mutex); I think this is too late, as krealloc_array() above has already released the old map. > /* 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; > + mutex_unlock(&pctrl->mutex); Please add a new label below, to avoid adding a call to mutex_unlock() here. > goto done; > } > > @@ -380,6 +384,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 +397,7 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, > > remove_group: > pinctrl_generic_remove_group(pctldev, gsel); ^^ new label here. > + mutex_unlock(&pctrl->mutex); > done: > *index = idx; > kfree(configs); Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v2 1/3] pinctrl: renesas: rzg2l: Fix NULL pointer > dereference in rzg2l_dt_subnode_to_map() > > Hi Biju, > > On Tue, Aug 15, 2023 at 9:56 AM 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 > > > > 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> > > --- > > v1->v2: > > * Updated crash logs as per submitting patches doc. > > Thanks for your patch! > > > --- 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> > > @@ -153,6 +154,7 @@ struct rzg2l_pinctrl { > > unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; > > > > spinlock_t lock; > > + struct mutex mutex; /* serialize adding > groups and functions */ > > I guess it's not too late to add comments to the two other locks > (bitmap_lock and lock)? OK will add. > > > }; > > > > static const unsigned int iolh_groupa_mA[] = { 2, 4, 8, 12 }; @@ > > -362,10 +364,12 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev > *pctldev, > > name = np->name; > > } > > > > + mutex_lock(&pctrl->mutex); > > I think this is too late, as krealloc_array() above has already released > the old map. OK will move krealloc_array() to reduce lock section and add lock. > > > /* 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; > > + mutex_unlock(&pctrl->mutex); > > Please add a new label below, to avoid adding a call to mutex_unlock() > here. OK. > > > goto done; > > } > > > > @@ -380,6 +384,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 +397,7 @@ static > > int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, > > > > remove_group: > > pinctrl_generic_remove_group(pctldev, gsel); > > ^^ new label here. OK. Cheers, Biju > > > + mutex_unlock(&pctrl->mutex); > > done: > > *index = idx; > > kfree(configs); > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c index 4f34f8f24bde..8a14cbb56449 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> @@ -153,6 +154,7 @@ struct rzg2l_pinctrl { unsigned int hwirq[RZG2L_TINT_MAX_INTERRUPT]; spinlock_t lock; + struct mutex mutex; /* serialize adding groups and functions */ }; static const unsigned int iolh_groupa_mA[] = { 2, 4, 8, 12 }; @@ -362,10 +364,12 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, name = np->name; } + mutex_lock(&pctrl->mutex); /* 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; + mutex_unlock(&pctrl->mutex); goto done; } @@ -380,6 +384,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 +397,7 @@ static int rzg2l_dt_subnode_to_map(struct pinctrl_dev *pctldev, remove_group: pinctrl_generic_remove_group(pctldev, gsel); + 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 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> --- v1->v2: * Updated crash logs as per submitting patches doc. --- drivers/pinctrl/renesas/pinctrl-rzg2l.c | 8 ++++++++ 1 file changed, 8 insertions(+)