From: Lu Baolu <baolu.lu@linux.intel.com> To: dinghao.liu@zju.edu.cn Cc: baolu.lu@linux.intel.com, kjlu@umn.edu, David Woodhouse <dwmw2@infradead.org>, Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>, Jiang Liu <jiang.liu@linux.intel.com>, Thomas Gleixner <tglx@linutronix.de>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc Date: Tue, 5 Jan 2021 09:51:31 +0800 [thread overview] Message-ID: <dda6e03a-147a-a482-4f31-f3dcb8aa47bd@linux.intel.com> (raw) In-Reply-To: <3b0b2129.17762.176c6e9114d.Coremail.dinghao.liu@zju.edu.cn> On 1/3/21 2:22 PM, dinghao.liu@zju.edu.cn wrote: >> On 2021/1/3 12:08, dinghao.liu@zju.edu.cn wrote: >>>> Hi, >>>> >>>> On 2021/1/2 17:50, Dinghao Liu wrote: >>>>> When irq_domain_get_irq_data() or irqd_cfg() fails >>>>> meanwhile i == 0, data allocated by kzalloc() has not >>>>> been freed before returning, which leads to memleak. >>>>> >>>>> Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to support hierarchical irqdomains") >>>>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> >>>>> --- >>>>> drivers/iommu/intel/irq_remapping.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c >>>>> index aeffda92b10b..cdaeed36750f 100644 >>>>> --- a/drivers/iommu/intel/irq_remapping.c >>>>> +++ b/drivers/iommu/intel/irq_remapping.c >>>>> @@ -1354,6 +1354,8 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain, >>>>> irq_cfg = irqd_cfg(irq_data); >>>>> if (!irq_data || !irq_cfg) { >>>>> ret = -EINVAL; >>>>> + kfree(data); >>>>> + data = NULL; >>>> >>>> Do you need to check (i == 0) here? @data will not be used anymore as it >>>> goes to out branch, why setting it to NULL here? >>>> >>> >>> data will be passed to ire_data->chip_data when i == 0 and >>> intel_free_irq_resources() will free it on failure. Thus I >> >> Isn't it going to "goto out_free_data"? If "i == 0", the allocated @data >> won't be freed by intel_free_irq_resources(), hence memory leaking. Does >> this patch aim to fix this? >> >> Best regards, >> baolu >> > > Correct, this is what I mean. When i > 0, data has been passed to > irq_data->chip_data, which will be freed in intel_free_irq_resources() > on failure. So there is no memleak in this case. The memleak only occurs > on failure when i == 0 (data has not been passed to irq_data->chip_data). So how about diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c index aeffda92b10b..685200a5cff0 100644 --- a/drivers/iommu/intel/irq_remapping.c +++ b/drivers/iommu/intel/irq_remapping.c @@ -1353,6 +1353,8 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain, irq_data = irq_domain_get_irq_data(domain, virq + i); irq_cfg = irqd_cfg(irq_data); if (!irq_data || !irq_cfg) { + if (!i) + kfree(data); ret = -EINVAL; goto out_free_data; } > I set data to NULL after kfree() in this patch to prevent double-free > when the failure occurs at i > 0. if i>0, @data has been passed and will be freed by intel_free_irq_resources() on the failure path. No need to free or clear, right? Best regards, baolu > > Regards, > Dinghao > >>> set it to NULL to prevent double-free. However, if we add >>> a check (i == 0) here, we will not need to set it to NULL. >>> If this is better, I will resend a new patch soon. >>> >>> Regards, >>> Dinghao >>>
WARNING: multiple messages have this Message-ID (diff)
From: Lu Baolu <baolu.lu@linux.intel.com> To: dinghao.liu@zju.edu.cn Cc: Will Deacon <will@kernel.org>, kjlu@umn.edu, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, Thomas Gleixner <tglx@linutronix.de>, David Woodhouse <dwmw2@infradead.org>, Jiang Liu <jiang.liu@linux.intel.com> Subject: Re: [PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc Date: Tue, 5 Jan 2021 09:51:31 +0800 [thread overview] Message-ID: <dda6e03a-147a-a482-4f31-f3dcb8aa47bd@linux.intel.com> (raw) In-Reply-To: <3b0b2129.17762.176c6e9114d.Coremail.dinghao.liu@zju.edu.cn> On 1/3/21 2:22 PM, dinghao.liu@zju.edu.cn wrote: >> On 2021/1/3 12:08, dinghao.liu@zju.edu.cn wrote: >>>> Hi, >>>> >>>> On 2021/1/2 17:50, Dinghao Liu wrote: >>>>> When irq_domain_get_irq_data() or irqd_cfg() fails >>>>> meanwhile i == 0, data allocated by kzalloc() has not >>>>> been freed before returning, which leads to memleak. >>>>> >>>>> Fixes: b106ee63abccb ("irq_remapping/vt-d: Enhance Intel IR driver to support hierarchical irqdomains") >>>>> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn> >>>>> --- >>>>> drivers/iommu/intel/irq_remapping.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c >>>>> index aeffda92b10b..cdaeed36750f 100644 >>>>> --- a/drivers/iommu/intel/irq_remapping.c >>>>> +++ b/drivers/iommu/intel/irq_remapping.c >>>>> @@ -1354,6 +1354,8 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain, >>>>> irq_cfg = irqd_cfg(irq_data); >>>>> if (!irq_data || !irq_cfg) { >>>>> ret = -EINVAL; >>>>> + kfree(data); >>>>> + data = NULL; >>>> >>>> Do you need to check (i == 0) here? @data will not be used anymore as it >>>> goes to out branch, why setting it to NULL here? >>>> >>> >>> data will be passed to ire_data->chip_data when i == 0 and >>> intel_free_irq_resources() will free it on failure. Thus I >> >> Isn't it going to "goto out_free_data"? If "i == 0", the allocated @data >> won't be freed by intel_free_irq_resources(), hence memory leaking. Does >> this patch aim to fix this? >> >> Best regards, >> baolu >> > > Correct, this is what I mean. When i > 0, data has been passed to > irq_data->chip_data, which will be freed in intel_free_irq_resources() > on failure. So there is no memleak in this case. The memleak only occurs > on failure when i == 0 (data has not been passed to irq_data->chip_data). So how about diff --git a/drivers/iommu/intel/irq_remapping.c b/drivers/iommu/intel/irq_remapping.c index aeffda92b10b..685200a5cff0 100644 --- a/drivers/iommu/intel/irq_remapping.c +++ b/drivers/iommu/intel/irq_remapping.c @@ -1353,6 +1353,8 @@ static int intel_irq_remapping_alloc(struct irq_domain *domain, irq_data = irq_domain_get_irq_data(domain, virq + i); irq_cfg = irqd_cfg(irq_data); if (!irq_data || !irq_cfg) { + if (!i) + kfree(data); ret = -EINVAL; goto out_free_data; } > I set data to NULL after kfree() in this patch to prevent double-free > when the failure occurs at i > 0. if i>0, @data has been passed and will be freed by intel_free_irq_resources() on the failure path. No need to free or clear, right? Best regards, baolu > > Regards, > Dinghao > >>> set it to NULL to prevent double-free. However, if we add >>> a check (i == 0) here, we will not need to set it to NULL. >>> If this is better, I will resend a new patch soon. >>> >>> Regards, >>> Dinghao >>> _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2021-01-05 2:01 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-01-02 9:50 [PATCH] iommu/intel: Fix memleak in intel_irq_remapping_alloc Dinghao Liu 2021-01-02 9:50 ` Dinghao Liu 2021-01-03 2:40 ` Lu Baolu 2021-01-03 2:40 ` Lu Baolu 2021-01-03 4:08 ` dinghao.liu 2021-01-03 4:08 ` dinghao.liu 2021-01-03 5:49 ` Lu Baolu 2021-01-03 5:49 ` Lu Baolu 2021-01-03 6:22 ` dinghao.liu 2021-01-03 6:22 ` dinghao.liu 2021-01-05 1:51 ` Lu Baolu [this message] 2021-01-05 1:51 ` Lu Baolu 2021-01-05 2:48 ` dinghao.liu 2021-01-05 2:48 ` dinghao.liu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=dda6e03a-147a-a482-4f31-f3dcb8aa47bd@linux.intel.com \ --to=baolu.lu@linux.intel.com \ --cc=dinghao.liu@zju.edu.cn \ --cc=dwmw2@infradead.org \ --cc=iommu@lists.linux-foundation.org \ --cc=jiang.liu@linux.intel.com \ --cc=joro@8bytes.org \ --cc=kjlu@umn.edu \ --cc=linux-kernel@vger.kernel.org \ --cc=tglx@linutronix.de \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.