| Message ID | 20210518131127.1308550-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [-next] i3c: master: svc: drop free_irq of devm_request_irq allocated irq | expand |
Hi Yang, Yang Yingliang <yangyingliang@huawei.com> wrote on Tue, 18 May 2021 21:11:27 +0800: > irq allocated with devm_request_irq should not be freed using > free_irq, because doing so causes a dangling pointer, and a > subsequent double free. > > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/i3c/master/svc-i3c-master.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > index 1f6ba4221817..761c9c468357 100644 > --- a/drivers/i3c/master/svc-i3c-master.c > +++ b/drivers/i3c/master/svc-i3c-master.c > @@ -1448,7 +1448,7 @@ static int svc_i3c_master_remove(struct platform_device *pdev) > if (ret) > return ret; > > - free_irq(master->irq, master); > + devm_free_irq(&pdev->dev, master->irq, master); Wouldn't removing this call the right solution? If it's a device managed resource, it won't probably be needed to free it explicitly in the remove path. > clk_disable_unprepare(master->pclk); > clk_disable_unprepare(master->fclk); > clk_disable_unprepare(master->sclk); Thanks, Miquèl
Hi, On 2021/5/27 18:01, Miquel Raynal wrote: > Hi Yang, > > Yang Yingliang <yangyingliang@huawei.com> wrote on Tue, 18 May 2021 > 21:11:27 +0800: > >> irq allocated with devm_request_irq should not be freed using >> free_irq, because doing so causes a dangling pointer, and a >> subsequent double free. >> >> Reported-by: Hulk Robot <hulkci@huawei.com> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/i3c/master/svc-i3c-master.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c >> index 1f6ba4221817..761c9c468357 100644 >> --- a/drivers/i3c/master/svc-i3c-master.c >> +++ b/drivers/i3c/master/svc-i3c-master.c >> @@ -1448,7 +1448,7 @@ static int svc_i3c_master_remove(struct platform_device *pdev) >> if (ret) >> return ret; >> >> - free_irq(master->irq, master); >> + devm_free_irq(&pdev->dev, master->irq, master); > Wouldn't removing this call the right solution? If it's a device > managed resource, it won't probably be needed to free it explicitly in > the remove path. Some drivers would expect to free irq itself, I am not sure if it's ok to remove the free_irq() in i3c, I just keep the original logic here and avoid double free. Thanks, Yang > >> clk_disable_unprepare(master->pclk); >> clk_disable_unprepare(master->fclk); >> clk_disable_unprepare(master->sclk); > Thanks, > Miquèl > .
Hi Yang, Yang Yingliang <yangyingliang@huawei.com> wrote on Thu, 27 May 2021 21:49:53 +0800: > Hi, > > On 2021/5/27 18:01, Miquel Raynal wrote: > > Hi Yang, > > > > Yang Yingliang <yangyingliang@huawei.com> wrote on Tue, 18 May 2021 > > 21:11:27 +0800: > > > >> irq allocated with devm_request_irq should not be freed using > >> free_irq, because doing so causes a dangling pointer, and a > >> subsequent double free. > >> > >> Reported-by: Hulk Robot <hulkci@huawei.com> > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > >> --- > >> drivers/i3c/master/svc-i3c-master.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c > >> index 1f6ba4221817..761c9c468357 100644 > >> --- a/drivers/i3c/master/svc-i3c-master.c > >> +++ b/drivers/i3c/master/svc-i3c-master.c > >> @@ -1448,7 +1448,7 @@ static int svc_i3c_master_remove(struct platform_device *pdev) > >> if (ret) > >> return ret; > >> >> - free_irq(master->irq, master); > >> + devm_free_irq(&pdev->dev, master->irq, master); > > Wouldn't removing this call the right solution? If it's a device > > managed resource, it won't probably be needed to free it explicitly in > > the remove path. > Some drivers would expect to free irq itself, I don't get it. Drivers do not expect anything, they should just comply with the API. If robots complain because a device managed resource is being freed without the device managed helper, this does not mean that the resource should explicitly be freed, it just means that *if* it must be explicitly freed, the wrong helper is being used. > I am not sure if it's ok to remove the free_irq() in i3c, What is the link with I3C? Sorry I might be missing something but master->irq is a driver variable, I don't get the link with the I3C framework and why it would interfere. > I just keep the original logic here and avoid double free. I don't think it is sane. Calling devm_free_irq() maybe is the right solution - I don't feel like it is - but your certainly can't hide behind a 'I just want the robots to be happy' justification. Hiding bugs on purpose is not something that I personally appreciate much. Thanks, Miquèl
On 2021/5/27 22:40, Miquel Raynal wrote: > Hi Yang, > > Yang Yingliang <yangyingliang@huawei.com> wrote on Thu, 27 May 2021 > 21:49:53 +0800: > >> Hi, >> >> On 2021/5/27 18:01, Miquel Raynal wrote: >>> Hi Yang, >>> >>> Yang Yingliang <yangyingliang@huawei.com> wrote on Tue, 18 May 2021 >>> 21:11:27 +0800: >>> >>>> irq allocated with devm_request_irq should not be freed using >>>> free_irq, because doing so causes a dangling pointer, and a >>>> subsequent double free. >>>> >>>> Reported-by: Hulk Robot <hulkci@huawei.com> >>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>> --- >>>> drivers/i3c/master/svc-i3c-master.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c >>>> index 1f6ba4221817..761c9c468357 100644 >>>> --- a/drivers/i3c/master/svc-i3c-master.c >>>> +++ b/drivers/i3c/master/svc-i3c-master.c >>>> @@ -1448,7 +1448,7 @@ static int svc_i3c_master_remove(struct platform_device *pdev) >>>> if (ret) >>>> return ret; >>>> >> - free_irq(master->irq, master); >>>> + devm_free_irq(&pdev->dev, master->irq, master); >>> Wouldn't removing this call the right solution? If it's a device >>> managed resource, it won't probably be needed to free it explicitly in >>> the remove path. >> Some drivers would expect to free irq itself, > I don't get it. Drivers do not expect anything, they should just comply > with the API. If robots complain because a device managed resource is > being freed without the device managed helper, this does not mean that > the resource should explicitly be freed, it just means that *if* it > must be explicitly freed, the wrong helper is being used. > >> I am not sure if it's ok to remove the free_irq() in i3c, > What is the link with I3C? Sorry I might be missing something but > master->irq is a driver variable, I don't get the link with the I3C > framework and why it would interfere. > >> I just keep the original logic here and avoid double free. > I don't think it is sane. Calling devm_free_irq() maybe is the right > solution - I don't feel like it is - but your certainly can't hide > behind a 'I just want the robots to be happy' justification. Hiding > bugs on purpose is not something that I personally appreciate much. Freeing irq in ->remove() is earlier than in device manage framework, if just remove the free_irq() in svc_i3c_master_remove() and free the irq by device manage framework, I am not sure if it breaks the resource free sequence in Silvaco I3C master driver. If it's OK, I can resend a patch with removing the free_irq(). Thanks, Yang > > Thanks, > Miquèl > .
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 1f6ba4221817..761c9c468357 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -1448,7 +1448,7 @@ static int svc_i3c_master_remove(struct platform_device *pdev) if (ret) return ret; - free_irq(master->irq, master); + devm_free_irq(&pdev->dev, master->irq, master); clk_disable_unprepare(master->pclk); clk_disable_unprepare(master->fclk); clk_disable_unprepare(master->sclk);
irq allocated with devm_request_irq should not be freed using free_irq, because doing so causes a dangling pointer, and a subsequent double free. Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/i3c/master/svc-i3c-master.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)