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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/