* [PATCH -next] scsi: hisi_sas: drop free_irq of devm_request_irq allocated irq
@ 2021-05-18 13:09 Yang Yingliang
2021-05-18 15:34 ` John Garry
0 siblings, 1 reply; 4+ messages in thread
From: Yang Yingliang @ 2021-05-18 13:09 UTC (permalink / raw)
To: linux-kernel, linux-scsi; +Cc: john.garry, jejb, martin.petersen
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/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 499c770d405c..684f762bcfb3 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -4811,9 +4811,9 @@ hisi_sas_v3_destroy_irqs(struct pci_dev *pdev, struct hisi_hba *hisi_hba)
{
int i;
- free_irq(pci_irq_vector(pdev, 1), hisi_hba);
- free_irq(pci_irq_vector(pdev, 2), hisi_hba);
- free_irq(pci_irq_vector(pdev, 11), hisi_hba);
+ devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 1), hisi_hba);
+ devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 2), hisi_hba);
+ devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 11), hisi_hba);
for (i = 0; i < hisi_hba->cq_nvecs; i++) {
struct hisi_sas_cq *cq = &hisi_hba->cq[i];
int nr = hisi_sas_intr_conv ? 16 : 16 + i;
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH -next] scsi: hisi_sas: drop free_irq of devm_request_irq allocated irq
2021-05-18 13:09 [PATCH -next] scsi: hisi_sas: drop free_irq of devm_request_irq allocated irq Yang Yingliang
@ 2021-05-18 15:34 ` John Garry
2021-05-19 3:36 ` Yang Yingliang
0 siblings, 1 reply; 4+ messages in thread
From: John Garry @ 2021-05-18 15:34 UTC (permalink / raw)
To: Yang Yingliang, linux-kernel, linux-scsi
Cc: jejb, martin.petersen, chenxiang, luojiaxing
On 18/05/2021 14:09, Yang Yingliang wrote:
> 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/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> index 499c770d405c..684f762bcfb3 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
> @@ -4811,9 +4811,9 @@ hisi_sas_v3_destroy_irqs(struct pci_dev *pdev, struct hisi_hba *hisi_hba)
> {
> int i;
>
> - free_irq(pci_irq_vector(pdev, 1), hisi_hba);
> - free_irq(pci_irq_vector(pdev, 2), hisi_hba);
> - free_irq(pci_irq_vector(pdev, 11), hisi_hba);
> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 1), hisi_hba);
> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 2), hisi_hba);
> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 11), hisi_hba);
> for (i = 0; i < hisi_hba->cq_nvecs; i++) {
> struct hisi_sas_cq *cq = &hisi_hba->cq[i];
> int nr = hisi_sas_intr_conv ? 16 : 16 + i;
>
Does the free_irq(pci_irq_vector(pdev, nr, cq)) call also need to change
(not shown)?
Having said that, why have these at all if we use devm_request_irq()?
devm_irq_release() calls free_irq().
Thanks,
John
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -next] scsi: hisi_sas: drop free_irq of devm_request_irq allocated irq
2021-05-18 15:34 ` John Garry
@ 2021-05-19 3:36 ` Yang Yingliang
2021-05-19 11:19 ` John Garry
0 siblings, 1 reply; 4+ messages in thread
From: Yang Yingliang @ 2021-05-19 3:36 UTC (permalink / raw)
To: John Garry, linux-kernel, linux-scsi
Cc: jejb, martin.petersen, chenxiang, luojiaxing
On 2021/5/18 23:34, John Garry wrote:
> On 18/05/2021 14:09, Yang Yingliang wrote:
>> 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/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> index 499c770d405c..684f762bcfb3 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>> @@ -4811,9 +4811,9 @@ hisi_sas_v3_destroy_irqs(struct pci_dev *pdev,
>> struct hisi_hba *hisi_hba)
>> {
>> int i;
>> - free_irq(pci_irq_vector(pdev, 1), hisi_hba);
>> - free_irq(pci_irq_vector(pdev, 2), hisi_hba);
>> - free_irq(pci_irq_vector(pdev, 11), hisi_hba);
>> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 1), hisi_hba);
>> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 2), hisi_hba);
>> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 11), hisi_hba);
>> for (i = 0; i < hisi_hba->cq_nvecs; i++) {
>> struct hisi_sas_cq *cq = &hisi_hba->cq[i];
>> int nr = hisi_sas_intr_conv ? 16 : 16 + i;
>>
>
> Does the free_irq(pci_irq_vector(pdev, nr, cq)) call also need to
> change (not shown)?
Yes, I missed that, it should be changed too.
>
> Having said that, why have these at all if we use devm_request_irq()?
> devm_irq_release() calls free_irq().
I keep the original logic here, only avoid double free.
>
> Thanks,
> John
>
> .
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH -next] scsi: hisi_sas: drop free_irq of devm_request_irq allocated irq
2021-05-19 3:36 ` Yang Yingliang
@ 2021-05-19 11:19 ` John Garry
0 siblings, 0 replies; 4+ messages in thread
From: John Garry @ 2021-05-19 11:19 UTC (permalink / raw)
To: Yang Yingliang, linux-kernel, linux-scsi
Cc: jejb, martin.petersen, chenxiang, luojiaxing
On 19/05/2021 04:36, Yang Yingliang wrote:
>
> On 2021/5/18 23:34, John Garry wrote:
>> On 18/05/2021 14:09, Yang Yingliang wrote:
>>> 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/scsi/hisi_sas/hisi_sas_v3_hw.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>>> b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>>> index 499c770d405c..684f762bcfb3 100644
>>> --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>>> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
>>> @@ -4811,9 +4811,9 @@ hisi_sas_v3_destroy_irqs(struct pci_dev *pdev,
>>> struct hisi_hba *hisi_hba)
>>> {
>>> int i;
>>> - free_irq(pci_irq_vector(pdev, 1), hisi_hba);
>>> - free_irq(pci_irq_vector(pdev, 2), hisi_hba);
>>> - free_irq(pci_irq_vector(pdev, 11), hisi_hba);
>>> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 1), hisi_hba);
>>> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 2), hisi_hba);
>>> + devm_free_irq(&pdev->dev, pci_irq_vector(pdev, 11), hisi_hba);
>>> for (i = 0; i < hisi_hba->cq_nvecs; i++) {
>>> struct hisi_sas_cq *cq = &hisi_hba->cq[i];
>>> int nr = hisi_sas_intr_conv ? 16 : 16 + i;
>>>
>>
>> Does the free_irq(pci_irq_vector(pdev, nr, cq)) call also need to
>> change (not shown)?
> Yes, I missed that, it should be changed too.
So I think that we need this addition:
devm_free_irq(&pdev->dev, pci_irq_vector(pdev, nr), cq);
>>
>> Having said that, why have these at all if we use devm_request_irq()?
>> devm_irq_release() calls free_irq().
> I keep the original logic here, only avoid double free.
Kasan doesn't complain. Anyway, I think we can't rely on device-managed
method (for calling free_irq()) as it conflicts with pci free vectors
call. I thought that someone was developed a device-managed version of
that (pci_alloc_irq_vectors()).
Anyway, please proceed with your change, but with the suggested addition.
Thanks
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-19 11:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 13:09 [PATCH -next] scsi: hisi_sas: drop free_irq of devm_request_irq allocated irq Yang Yingliang
2021-05-18 15:34 ` John Garry
2021-05-19 3:36 ` Yang Yingliang
2021-05-19 11:19 ` John Garry
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.