iommu/amd: make kmemleak ignore the 'irq_remap_table' object

The kmemleak testing on 3.18.24 show:

unreferenced object 0xffff880233ff9010 (size 16):
comm "swapper/0", pid 1, jiffies 4294937440 (age 2010.490s) hex dump (first 16 bytes):

    0a 0a 00 00 20 00 00 00 00 44 fb 33 02 88 ff ff  .... ....D.3....
  backtrace:
    [<ffffffff8118192d>] create_object+0x10d/0x2d0
    [<ffffffff815c2d4b>] kmemleak_alloc+0x5b/0xc0
    [<ffffffff8116dd19>] kmem_cache_alloc_trace+0xb9/0x160
    [<ffffffff814ffe51>] get_irq_table+0x151/0x380

This is caused by the 'irq_lookup_table' was allocated with __get_free_pages() which won't create kmemleak object, thus it's pointers won't be count as referencing 'irq_remap_table' in kmemleak scan.

The 'irq_remap_table' won't be freed after initialized, doesn't make sense to check it's leaking.

This patch mark the 'irq_remap_table' object as 'gray' to stop the 'false positives' report.

Signed-off-by Michael Wang <[email protected]>

---
v2:
Use kmemleak_not_leak() instead of kmemleak_ignore() since the 'irq_remap_table' itself also contain pointer.

 drivers/iommu/amd_iommu.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iommu/amd_iommu.c +1 -0

index 8b2be1e..87a1a88 100644
@@ -3603,6 +3603,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	}
 
 	irq_lookup_table[devid] = table;
+	kmemleak_not_leak(table);
 	set_dte_irq_entry(devid, table);
 	iommu_flush_dte(iommu, devid);
 	if (devid != alias) {

The kmemleak testing on 3.18.24 show:

unreferenced object 0xffff880233ff9010 (size 16):
comm "swapper/0", pid 1, jiffies 4294937440 (age 2010.490s) hex dump (first 16 bytes):

    0a 0a 00 00 20 00 00 00 00 44 fb 33 02 88 ff ff  .... ....D.3....
  backtrace:
    [<ffffffff8118192d>] create_object+0x10d/0x2d0
    [<ffffffff815c2d4b>] kmemleak_alloc+0x5b/0xc0
    [<ffffffff8116dd19>] kmem_cache_alloc_trace+0xb9/0x160
    [<ffffffff814ffe51>] get_irq_table+0x151/0x380

This is caused by the 'irq_lookup_table' was allocated with __get_free_pages() which won't create kmemleak object, thus it's pointers won't be count as referencing 'irq_remap_table' in kmemleak scan.

The 'irq_remap_table' won't be freed after initialized, doesn't make sense to check it's leaking.

This patch mark the 'irq_remap_table' object as 'gray' to stop the 'false positives' report.

Signed-off-by Michael Wang <[email protected]>

---
v2:
Use kmemleak_not_leak() instead of kmemleak_ignore() since the 'irq_remap_table' itself also contain pointer.

 drivers/iommu/amd_iommu.c | 1 +
 1 file changed, 1 insertion(+)

drivers/iommu/amd_iommu.c +1 -0

index 8b2be1e..87a1a88 100644
@@ -3603,6 +3603,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	}
 
 	irq_lookup_table[devid] = table;
+	kmemleak_not_leak(table);
 	set_dte_irq_entry(devid, table);
 	iommu_flush_dte(iommu, devid);
 	if (devid != alias) {

The kmemleak testing on 3.18.24 show:

unreferenced object 0xffff880233ff9010 (size 16):
comm "swapper/0", pid 1, jiffies 4294937440 (age 2010.490s) hex dump (first 16 bytes):

    0a 0a 00 00 20 00 00 00 00 44 fb 33 02 88 ff ff  .... ....D.3....
  backtrace:
    [<ffffffff8118192d>] create_object+0x10d/0x2d0
    [<ffffffff815c2d4b>] kmemleak_alloc+0x5b/0xc0
    [<ffffffff8116dd19>] kmem_cache_alloc_trace+0xb9/0x160
    [<ffffffff814ffe51>] get_irq_table+0x151/0x380

This is caused by the 'irq_lookup_table' was allocated with __get_free_pages() which won't create kmemleak object, thus it's pointers won't be count as referencing 'irq_remap_table' in kmemleak scan.

Quoted 12 lines · Michael Wang

The kmemleak testing on 3.18.24 show:

unreferenced object 0xffff880233ff9010 (size 16):
comm "swapper/0", pid 1, jiffies 4294937440 (age 2010.490s) hex dump (first 16 bytes):

    0a 0a 00 00 20 00 00 00 00 44 fb 33 02 88 ff ff  .... ....D.3....
  backtrace:
    [<ffffffff8118192d>] create_object+0x10d/0x2d0
    [<ffffffff815c2d4b>] kmemleak_alloc+0x5b/0xc0
    [<ffffffff8116dd19>] kmem_cache_alloc_trace+0xb9/0x160
    [<ffffffff814ffe51>] get_irq_table+0x151/0x380

Quoted 4 lines

This is caused by the 'irq_lookup_table' was allocated with __get_free_pages() which won't create kmemleak object, thus it's pointers won't be count as referencing 'irq_remap_table' in kmemleak scan.

Isn't it better to allocate the kmemleak object manually instead of ignoring all irq-table pointers? With this patch we might not notice any real leak of irq-tables.

Joerg

On 11/25/2015 04:08 PM, Joerg Roedel wrote:

Quoted 5 lines

Quoted 4 lines

This is caused by the 'irq_lookup_table' was allocated with __get_free_pages() which won't create kmemleak object, thus it's pointers won't be count as referencing 'irq_remap_table' in kmemleak scan.

Quoted 3 lines

Isn't it better to allocate the kmemleak object manually instead of ignoring all irq-table pointers? With this patch we might not notice any real leak of irq-tables.

We've considered that too, but found that the irq-tables is not dynamically alloc/free, they won't be freed once initialized, so there is no leaking for such object :-)

Regards,
Michael Wang

Quoted 5 lines

Joerg

Quoted 1 line · Michael Wang

On 11/25/2015 04:08 PM, Joerg Roedel wrote:

[snip]

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

This is caused by the 'irq_lookup_table' was allocated with __get_free_pages() which won't create kmemleak object, thus it's pointers won't be count as referencing 'irq_remap_table' in kmemleak scan.

Isn't it better to allocate the kmemleak object manually instead of ignoring all irq-table pointers? With this patch we might not notice any real leak of irq-tables.

Quoted 3 lines

We've considered that too, but found that the irq-tables is not dynamically alloc/free, they won't be freed once initialized, so there is no leaking for such object :-)

Is there any more concern? actually we just want to get rid of this annoying report on obj won't leak, if you're going to create obj for 'irq_lookup_table' that's also fine for us, or will you pick this patch?

Quoted 4 lines

Is there any more concern? actually we just want to get rid of this annoying report on obj won't leak, if you're going to create obj for 'irq_lookup_table' that's also fine for us, or will you pick this patch?

My preference (from a kmemleak perspective) is to tell kmemleak about the irq_lookup_table. Untested:

drivers/iommu/amd_iommu_init.c +3 -0

index 013bdfff2d4d..c41609f71cbe 100644
@@ -27,6 +27,7 @@
 #include <linux/amd-iommu.h>
 #include <linux/export.h>
 #include <linux/iommu.h>
+#include <linux/kmemleak.h>
 #include <asm/pci-direct.h>
 #include <asm/iommu.h>
 #include <asm/gart.h>

@@ -1692,6 +1693,7 @@ static struct syscore_ops amd_iommu_syscore_ops = {
 
 static void __init free_on_init_error(void)
 {
+ kmemleak_free(irq_lookup_table);
  free_pages((unsigned long)irq_lookup_table,
    get_order(rlookup_table_size));
 

@@ -1906,6 +1908,7 @@ static int __init early_amd_iommu_init(void)
  irq_lookup_table = (void *)__get_free_pages(
  GFP_KERNEL | __GFP_ZERO,
  get_order(rlookup_table_size));
+ kmemleak_alloc(irq_lookup_table, rlookup_table_size, 1, GFP_KERNEL);
  if (!irq_lookup_table)
  goto out;
  }

drivers/iommu/amd_iommu_init.c +2 -0

index 013bdfff2d4d..c41609f71cbe 100644
@@ -27,6 +27,7 @@
 #include <linux/amd-iommu.h>
 #include <linux/export.h>
 #include <linux/iommu.h>
+#include <linux/kmemleak.h>
 #include <asm/pci-direct.h>
 #include <asm/iommu.h>
 #include <asm/gart.h>

@@ -1692,6 +1693,7 @@ static struct syscore_ops amd_iommu_syscore_ops = {
 
 static void __init free_on_init_error(void)
 {
+ kmemleak_free(irq_lookup_table);
  free_pages((unsigned long)irq_lookup_table,
    get_order(rlookup_table_size));
 

Quoted 6 lines · Catalin Marinas

Quoted 3 lines

Is there any more concern? actually we just want to get rid of this annoying report on obj won't leak, if you're going to create obj for 'irq_lookup_table' that's also fine for us, or will you pick this patch?

My preference (from a kmemleak perspective) is to tell kmemleak about the irq_lookup_table. Untested:

I'm fine with both solution, will leave the decision to maintainer :-)

BTW, could you please send a formal patch with descriptions?

On 2 December 2015 at 10:56, Michael Wang <[email protected]> wrote:

I could copy your description but I don't currently have a way (nor time) to test the patch. If you plan to test it anyway, please feel free to include my diff (which I guess was badly re-formatted by gmail), I don't really mind which author it is (I found it easier to show a diff than explain in plain English ;)).

Joerg, this is really a tiny fix, would you mind to merge it into some of your cleanup patch and testing them together? we are not in hurry, just want to make sure the issue will get solved.

Quoted 3 lines · Michael Wang

Joerg, this is really a tiny fix, would you mind to merge it into some of your cleanup patch and testing them together? we are not in hurry, just want to make sure the issue will get solved.

I am not doing your work. You sent a patch, received feedback, and now you can send a new patch based on it. Thats the process. If it addresses the feedback I will merge it, but I will not scissor your patch together.

Joerg

It's not my work or your work... it's a defect in the module and maintainer should take responsibility on fixing it, correct?

It's not my work or your work... it's a defect in the module and maintainer should take responsibility on fixing it, correct?

We're very willing to help, but as I mentioned we are out of resource for testing at this moment, but we can send you a new patch without testing, will that works for you?

Quoted 2 lines · Michael Wang

It's not my work or your work... it's a defect in the module and maintainer should take responsibility on fixing it, correct?

Well, you said "actually we just want to get rid of this annoying report on obj won't leak..."

It sounds to me like you want to have something fixed. So you do the patch properly, add to the commit message why exactly you're doing it and test it. Like everyone else.

Quoted 2 lines · Michael Wang

It's not my work or your work... it's a defect in the module and maintainer should take responsibility on fixing it, correct?

No, its a false positive from an in-kernel checking tool, the iommu driver is correct. You just sent a patch to silence the false positive report.

Quoted 4 lines

We're very willing to help, but as I mentioned we are out of resource for testing at this moment, but we can send you a new patch without testing, will that works for you?

This should be testable on any AMD IOMMU system with working interrupt remapping. I will probably have no time to test this, if you really can't test yourself, try to get a Tested-by from someone else.

Joerg

Yeah.. it's a little complicated since we have our own kernel tree and this won't be a problem for us, but we really prefer to help fix it in mainline too, as long as this is really a defect, so others could save time on research in future.

Quoted 4 lines · Michael Wang

Yeah.. it's a little complicated since we have our own kernel tree and this won't be a problem for us, but we really prefer to help fix it in mainline too, as long as this is really a defect, so others could save time on research in future.

Well, to keep it realistic and if it were me, I wouldn't even take such a fix as it is apparently kmemleak's problem.

So you could fix your testing instead to ignore that error message now that you know it is a false-positive. That should be easiest.

Do you mean this could be a real kmemleak? Could you please provide more details?

Yeah, but it would be better to solve it, otherwise whoever saw this report will need to go into the amd-iommu, make sure it's not a real leak, then change their testing script...

On Wed, Dec 02, 2015 at 02:18:44PM +0100, Michael Wang wrote:

Quoted 4 lines

Yeah, but it would be better to solve it, otherwise whoever saw this report will need to go into the amd-iommu, make sure it's not a real leak, then change their testing script...

No, you don't need to go into the iommu - you need to fix kmemleak.

And frankly, I'm getting sick and tired of all those tools needing special handling and us adding code just so that they're happy. If the tools can't figure out something, they shouldn't warn just in case but shut up instead.

I'm not sure why amd-iommu use get_page not kmalloc to initialize the pointer table, but if it's necessary then the conflict will be there, it's not the fault of driver or kmemleak, but the design require them to cooperate with each other.

Quoted 4 lines · Michael Wang

I'm not sure why amd-iommu use get_page not kmalloc to initialize the pointer table, but if it's necessary then the conflict will be there, it's not the fault of driver or kmemleak, but the design require them to cooperate with each other.

So, according to you, we should go and "fix" all callers of __get_free_pages() to make kmemleak happy. Then when the next new tool comes along, we should "fix" another kernel API just so that the tools are happy.

Bzzt. Wrong!

The tools should work without sprinkling their code everywhere. Driver etc developers don't need to care about what tool they make happy or not. Tools' hooks should be hidden in macro magic so that developers don't care.

This tool will help improve the kernel, AFAIK it's already made it's best, if you got any idea on how to make it even better that would be great, but at this moment, it still need few of care :-P

Quoted 3 lines · Michael Wang

This tool will help improve the kernel, AFAIK it's already made it's best, if you got any idea on how to make it even better that would be great, but at this moment, it still need few of care :-P

I think you're replying to my emails without even reading what I said. So I'm going to do the same and stop wasting my time.

Quoted 4 lines · Michael Wang

I'm not sure why amd-iommu use get_page not kmalloc to initialize the pointer table, but if it's necessary then the conflict will be there, it's not the fault of driver or kmemleak, but the design require them to cooperate with each other.

So, according to you, we should go and "fix" all callers of __get_free_pages() to make kmemleak happy. Then when the next new tool comes along, we should "fix" another kernel API just so that the tools are happy.

Defending kmemleak here ;).

Tracking page allocations in kmemleak by intercepting __get_free_pages() has significant implications on false negatives for two main reasons:

1. The sl?b allocators themselves use page allocations, so kmemleak could end up detecting the same pointer twice, hiding a potential leak

2. Most page allocations do not contain data/pointers relevant to kmemleak (e.g. page cache pages), however the randomness of such data greatly diminishes kmemleak's ability to detect real leaks

Arguably, kmemleak could be made to detect both cases above by a combination of page flags, additional annotations or specific page alloc API. However, this has its own drawbacks in terms of code complexity (potentially outside mm/kmemleak.c) and overhead.

Regarding a kmemleak_alloc() annotation like in the patch I suggested, that's the second one I've seen needed outside alloc APIs (the first one is commit f75782e4e067 - "block: kmemleak: Track the page allocations for struct request"). If the number of such explicit annotations stays small, it's better to keep it this way.

There are other explicit annotations like kmemleak_not_leak() or kmemleak_ignore() but these are for objects kmemleak knows about and incorrectly reports them as leaks. Most of the time is because the pointers to such objects are stored in a different form (e.g. physical address).

Anyway, kmemleak is not the only tool requiring annotations (take spin_lock_nested() for example).

If needed, we could do with an additional page alloc/free API which informs kmemleak in the process but I don't think it's worth it.

On 2 December 2015 at 13:59, Borislav Petkov <[email protected]> wrote:

1. The sl?b allocators themselves use page allocations, so kmemleak could end up detecting the same pointer twice, hiding a potential leak

2. Most page allocations do not contain data/pointers relevant to kmemleak (e.g. page cache pages), however the randomness of such data greatly diminishes kmemleak's ability to detect real leaks

Arguably, kmemleak could be made to detect both cases above by a combination of page flags, additional annotations or specific page alloc API. However, this has its own drawbacks in terms of code complexity (potentially outside mm/kmemleak.c) and overhead.

Regarding a kmemleak_alloc() annotation like in the patch I suggested, that's the second one I've seen needed outside alloc APIs (the first one is commit f75782e4e067 - "block: kmemleak: Track the page allocations for struct request"). If the number of such explicit annotations stays small, it's better to keep it this way.

There are other explicit annotations like kmemleak_not_leak() or kmemleak_ignore() but these are for objects kmemleak knows about and incorrectly reports them as leaks. Most of the time is because the pointers to such objects are stored in a different form (e.g. physical address).

Anyway, kmemleak is not the only tool requiring annotations (take spin_lock_nested() for example). If needed, we could do with an additional page alloc/free API which informs kmemleak in the process but I don't think it's worth it.