[RFC PATCH] iommu/amd: make kmemleak ignore the 'irq_remap_table' object

[RFC PATCH] iommu/amd: make kmemleak ignore the 'irq_remap_table' object

[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

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 in kmemleak scanning.

The 'irq_remap_table' allocated won't be freed after initialized,
doesn't make sense to let kmemleak scan it.

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

Signed-off-by: Michael Wang <yun.wang@profitbricks.com>
---
 drivers/iommu/amd_iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 8b2be1e..87a1a88 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3603,6 +3603,7 @@ static struct irq_remap_table *get_irq_table(u16 devid, bool ioapic)
 	}
 
 	irq_lookup_table[devid] = table;
+	kmemleak_ignore(table);
 	set_dte_irq_entry(devid, table);
 	iommu_flush_dte(iommu, devid);
 	if (devid != alias) {
-- 
2.1.4

[RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[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

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 <yun.wang@profitbricks.com>
---
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(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 8b2be1e..87a1a88 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -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) {
-- 
2.1.4

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 11/20/2015 12:33 PM, Michael Wang wrote:
> 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 <yun.wang@profitbricks.com>

Reported-by: Danil Kipnis <danil.kipnis@profitbricks.com>

Regards,
Michael Wang

> ---
> 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(+)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 8b2be1e..87a1a88 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -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) {
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

Hi, Joery

On 11/20/2015 12:33 PM, Michael Wang wrote:
> 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.

Any comments on this one?

Regards,
Michael Wang

> 
> Signed-off-by: Michael Wang <yun.wang@profitbricks.com>
> ---
> 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(+)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> index 8b2be1e..87a1a88 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -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) {
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Joerg Roedel]

On Fri, Nov 20, 2015 at 12:33:50PM +0100, Michael Wang wrote:
> 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.

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

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 11/25/2015 04:08 PM, Joerg Roedel wrote:
> On Fri, Nov 20, 2015 at 12:33:50PM +0100, Michael Wang wrote:
>> 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.
> 
> 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

> 
> 
> 
> 	Joerg
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

Hi, Joerg

On 11/25/2015 04:14 PM, Michael Wang wrote:
> On 11/25/2015 04:08 PM, Joerg Roedel wrote:
[snip]
>>> 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.
> 
> 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?

Regards,
Michael Wang

> 
> Regards,
> Michael Wang
> 
>>
>>
>>
>> 	Joerg
>>

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Catalin Marinas]

On 2 December 2015 at 10:37, Michael Wang <yun.wang@profitbricks.com> wrote:
> On 11/25/2015 04:14 PM, Michael Wang wrote:
>> On 11/25/2015 04:08 PM, Joerg Roedel wrote:
> [snip]
>>>> 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.
>>
>> 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?

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

diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
index 013bdfff2d4d..c41609f71cbe 100644
--- a/drivers/iommu/amd_iommu_init.c
+++ b/drivers/iommu/amd_iommu_init.c
@@ -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;
  }

-- 
Catalin

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 11:52 AM, Catalin Marinas wrote:
[snip]
>>
>> 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?

Regards,
Michael Wang

> 
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 013bdfff2d4d..c41609f71cbe 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -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;
>   }
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Catalin Marinas]

On 2 December 2015 at 10:56, Michael Wang <yun.wang@profitbricks.com> wrote:
> On 12/02/2015 11:52 AM, Catalin Marinas wrote:
>>> 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?

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 ;)).

-- 
Catalin

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 12:31 PM, Catalin Marinas wrote:
> On 2 December 2015 at 10:56, Michael Wang <yun.wang@profitbricks.com> wrote:
[snip]
> 
> 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 ;)).

Unfortunately that's the same on my side... we already close the ticket
and I don't have resources to testing it again.

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.

Regards,
Michael Wang

> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Joerg Roedel]

On Wed, Dec 02, 2015 at 12:38:03PM +0100, Michael Wang wrote:
> 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

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 12:51 PM, Joerg Roedel wrote:
> On Wed, Dec 02, 2015 at 12:38:03PM +0100, Michael Wang wrote:
>> 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.

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?

Regards,
Michael Wang

> 
> 
> 	Joerg
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Borislav Petkov]

On Wed, Dec 02, 2015 at 01:31:38PM +0100, Michael Wang wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 01:53 PM, Borislav Petkov wrote:
> On Wed, Dec 02, 2015 at 01:31:38PM +0100, Michael Wang wrote:
>> 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.

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.

But seems like we can only wait for another chance to confirm the another
solution, frankly speaking I think we both will forgot this soon... fortunately
it's not that critical :-P

Regards,
Michael Wang

> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Borislav Petkov]

On Wed, Dec 02, 2015 at 02:01:55PM +0100, Michael Wang wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

Hi, Borislav

On 12/02/2015 02:13 PM, Borislav Petkov wrote:
> On Wed, Dec 02, 2015 at 02:01:55PM +0100, Michael Wang wrote:
>> 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.

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

> 
> 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.
> 

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...

Regards,
Michael Wang

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Borislav Petkov]

On Wed, Dec 02, 2015 at 02:18:44PM +0100, Michael Wang wrote:
> Do you mean this could be a real kmemleak? Could you please provide
> more details?

Did you read Joerg's last mail?

> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 02:40 PM, Borislav Petkov wrote:
> On Wed, Dec 02, 2015 at 02:18:44PM +0100, Michael Wang wrote:
[snip]
> 
>> 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.

If you mean the design of kmemleak, IMHO it's not that bad.

The problem is regarding performance, think about if kmemleak go into
every page to find out pointers, I guess the whole system will stuck.

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.

Regards,
Michael Wang

> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Borislav Petkov]

On Wed, Dec 02, 2015 at 02:48:47PM +0100, Michael Wang wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 02:59 PM, Borislav Petkov wrote:
> On Wed, Dec 02, 2015 at 02:48:47PM +0100, Michael Wang wrote:
>> 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.

That's the way we have to detect leak, no driver could get rid of
the possibility of memory leaking, so it should respect the rule to
help others locating the problem, if a driver full of false report then
most likely folks will gradually lost interests on help fix leaking
problem for it.

> 
> 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

Regards,
Michael Wang

> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Borislav Petkov]

On Wed, Dec 02, 2015 at 03:09:18PM +0100, Michael Wang wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 03:13 PM, Borislav Petkov wrote:
> On Wed, Dec 02, 2015 at 03:09:18PM +0100, Michael Wang wrote:
>> 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.

First of all, I respect all your reply, and reply regarding your point.

Frankly speaking, I think you know all these already, it's not a big
deal but you refuse to obey the rules setup by others, although it do
help make things better, and benefit yourself too.

If you refuse to make things better I'm totally fine, time is valuable
for all of us :-)

Regards,
Michael Wang

> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Catalin Marinas]

On 2 December 2015 at 13:59, Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Dec 02, 2015 at 02:48:47PM +0100, Michael Wang wrote:
>> 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.

-- 
Catalin

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Borislav Petkov]

On Wed, Dec 02, 2015 at 05:36:32PM +0000, Catalin Marinas wrote:
> Defending kmemleak here ;).

Oh sure, by all means. I'm also assuming it comes across that I wasn't
attacking kmemleak. I had the same arguments with KASAN and other stuff
in the past.

> 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

Yeah, so can you teach kmemleak to ignore pointers from
__get_free_pages()? Since it cannot track them properly and it causes
false positives and all. Now it invites people to add annotation which
makes unrelated code obscure.

> 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.

Well, how do you define "small"? When is it ok and when do we say, no
more?

> 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).

Yeah, and it doesn't look great. I know, it helps a lot and yadda
yadda...

> 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.

Well, I think it is a big win if it gets to keep the code clean. And we
don't care about performance with kmemleak - it is a performance hit
anyway but we take that hit for a reason.

Again, I'm not attacking kmemleak or any other tool for that matter -
I just want for tools to be honest: if kmemleak cannot classify that
pointer and until it is able to do so, it should be quiet about it
because, well, honestly, it doesn't really know.

All that annotation coming with every new tool is simply annoying and
obscures the code so perhaps we should aim at keeping it close to 0. I
hope you're catching my drift exactly as I intended. If not, I'll gladly
reiterate.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 06:36 PM, Catalin Marinas wrote:
> On 2 December 2015 at 13:59, Borislav Petkov <bp@alien8.de> wrote:
[snip]
> 
> 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.

Thanks for the very nice explain :-) I used to thought overhead is
the only concern, missing the point regarding allocator it self.

Regards,
Michael Wang

> 
> 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.
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Joerg Roedel]

On Wed, Dec 02, 2015 at 01:31:38PM +0100, Michael Wang wrote:
> 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.

> 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

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Michael Wang]

On 12/02/2015 01:56 PM, Joerg Roedel wrote:
> On Wed, Dec 02, 2015 at 01:31:38PM +0100, Michael Wang wrote:
>> 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.

Yeah, but caused by the driver :-P and have to be fixed in there too.

> 
>> 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.

Good point, anyone willing to help test the fix? Or better provide the new
patch to be the author.

Regards,
Michael Wang

> 
> 
> 
> 	Joerg
> 

Re: [RFC PATCH v2] iommu/amd: gray the 'irq_remap_table' object for kmemleak

[Joerg Roedel]

On Wed, Dec 02, 2015 at 10:52:02AM +0000, Catalin Marinas wrote:
> diff --git a/drivers/iommu/amd_iommu_init.c b/drivers/iommu/amd_iommu_init.c
> index 013bdfff2d4d..c41609f71cbe 100644
> --- a/drivers/iommu/amd_iommu_init.c
> +++ b/drivers/iommu/amd_iommu_init.c
> @@ -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;
>   }

Yes, this looks way better.