| Message ID | 20240601113502.2709597-1-yangyingliang@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: core: fix possible memory leak in error path in pinctrl_enable() | expand |
On Sat, Jun 01, 2024 at 07:35:02PM +0800, Yang Yingliang wrote: > In devm_pinctrl_register(), if pinctrl_enable() fails in pinctrl_register(), > the "pctldev" has not been added to dev resources, so devm_pinctrl_dev_release() > can not be called, it leads memory leak. > > And some driver calls pinctrl_register_and_init() which is not devm_ managed, > it also leads memory leak if pinctrl_enable() fails. > > To fix this, add a flag devm_allocated to struct pinctrl_dev, free the memory > by checking this flag. > > Fixes: 5038a66dad01 ("pinctrl: core: delete incorrect free in pinctrl_enable()") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/pinctrl/core.c | 9 +++++++++ > drivers/pinctrl/core.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index cffeb869130d..374c36f5c759 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -2125,6 +2125,13 @@ int pinctrl_enable(struct pinctrl_dev *pctldev) > error = pinctrl_claim_hogs(pctldev); > if (error) { > dev_err(pctldev->dev, "could not claim hogs: %i\n", error); > + if (!pctldev->devm_allocated) { > + pinctrl_free_pindescs(pctldev, pctldev->desc->pins, > + pctldev->desc->npins); > + mutex_destroy(&pctldev->mutex); > + kfree(pctldev); > + } Why not do this free in pinctrl_register()? It's a layering violation to do it here. It's not where you expect the free to be. It will just lead to double free bugs. regards, dan carpenter
On Sat, Jun 01, 2024 at 07:35:02PM +0800, Yang Yingliang wrote: > In devm_pinctrl_register(), if pinctrl_enable() fails in pinctrl_register(), > the "pctldev" has not been added to dev resources, so devm_pinctrl_dev_release() > can not be called, it leads memory leak. > > And some driver calls pinctrl_register_and_init() which is not devm_ managed, > it also leads memory leak if pinctrl_enable() fails. > > To fix this, add a flag devm_allocated to struct pinctrl_dev, free the memory > by checking this flag. > > Fixes: 5038a66dad01 ("pinctrl: core: delete incorrect free in pinctrl_enable()") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/pinctrl/core.c | 9 +++++++++ > drivers/pinctrl/core.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index cffeb869130d..374c36f5c759 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -2125,6 +2125,13 @@ int pinctrl_enable(struct pinctrl_dev *pctldev) > error = pinctrl_claim_hogs(pctldev); > if (error) { > dev_err(pctldev->dev, "could not claim hogs: %i\n", error); > + if (!pctldev->devm_allocated) { > + pinctrl_free_pindescs(pctldev, pctldev->desc->pins, > + pctldev->desc->npins); > + mutex_destroy(&pctldev->mutex); > + kfree(pctldev); > + } The other thing is, this should be done in a function. ti_iodelay_probe() would have to call uninit as well. The error handling in ti_iodelay_probe() was already messed up. They probably assumed that pinctrl_enable() would do the cleanup so they forgot to call of_node_put(np). regards, dan carpenter diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index 9fcb9d913556..704541385397 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -2090,6 +2090,13 @@ pinctrl_init_controller(struct pinctrl_desc *pctldesc, struct device *dev, return ERR_PTR(ret); } +void pinctrl_uninit_controller(struct pinctrl_dev *pctldev) +{ + pinctrl_free_pindescs(pctldev, pctldev->desc->pins, pctldev->desc->npins); + mutex_destroy(&pctldev->mutex); + kfree(pctldev); +} + static int pinctrl_claim_hogs(struct pinctrl_dev *pctldev) { pctldev->p = create_pinctrl(pctldev->dev, pctldev);
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index cffeb869130d..374c36f5c759 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -2125,6 +2125,13 @@ int pinctrl_enable(struct pinctrl_dev *pctldev) error = pinctrl_claim_hogs(pctldev); if (error) { dev_err(pctldev->dev, "could not claim hogs: %i\n", error); + if (!pctldev->devm_allocated) { + pinctrl_free_pindescs(pctldev, pctldev->desc->pins, + pctldev->desc->npins); + mutex_destroy(&pctldev->mutex); + kfree(pctldev); + } + return error; } @@ -2283,6 +2290,7 @@ struct pinctrl_dev *devm_pinctrl_register(struct device *dev, return pctldev; } + pctldev->devm_allocated = true; *ptr = pctldev; devres_add(dev, ptr); @@ -2319,6 +2327,7 @@ int devm_pinctrl_register_and_init(struct device *dev, return error; } + (*pctldev)->devm_allocated = true; *ptr = *pctldev; devres_add(dev, ptr); diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h index 837fd5bd903d..bf57ce4fe927 100644 --- a/drivers/pinctrl/core.h +++ b/drivers/pinctrl/core.h @@ -72,6 +72,7 @@ struct pinctrl_dev { #ifdef CONFIG_DEBUG_FS struct dentry *device_root; #endif + bool devm_allocated; }; /**
In devm_pinctrl_register(), if pinctrl_enable() fails in pinctrl_register(), the "pctldev" has not been added to dev resources, so devm_pinctrl_dev_release() can not be called, it leads memory leak. And some driver calls pinctrl_register_and_init() which is not devm_ managed, it also leads memory leak if pinctrl_enable() fails. To fix this, add a flag devm_allocated to struct pinctrl_dev, free the memory by checking this flag. Fixes: 5038a66dad01 ("pinctrl: core: delete incorrect free in pinctrl_enable()") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/pinctrl/core.c | 9 +++++++++ drivers/pinctrl/core.h | 1 + 2 files changed, 10 insertions(+)