[PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Qian Cai]

a0fc5578f1d (efi: Let kmemleak ignore false positives) is no longer
needed due to efi_mem_reserve_persistent() uses __get_free_page()
instead where kmemelak is not able to track regardless. Otherwise,
kernel reported "kmemleak: Trying to color unknown object at
0xffff801060ef0000 as Black"

Signed-off-by: Qian Cai <cai@lca.pw>
---
 drivers/firmware/efi/efi.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 7ac09dd8f268..4c46ff6f2242 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -31,7 +31,6 @@
 #include <linux/acpi.h>
 #include <linux/ucs2_string.h>
 #include <linux/memblock.h>
-#include <linux/kmemleak.h>
 
 #include <asm/early_ioremap.h>
 
@@ -1027,8 +1026,6 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
 	if (!rsv)
 		return -ENOMEM;
 
-	kmemleak_ignore(rsv);
-
 	rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
 	atomic_set(&rsv->count, 1);
 	rsv->entry[0].base = addr;
-- 
2.17.2 (Apple Git-113)

Re: [PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Ard Biesheuvel]

On Wed, 26 Dec 2018 at 03:35, Qian Cai <cai@lca.pw> wrote:
>
> a0fc5578f1d (efi: Let kmemleak ignore false positives) is no longer
> needed due to efi_mem_reserve_persistent() uses __get_free_page()
> instead where kmemelak is not able to track regardless. Otherwise,
> kernel reported "kmemleak: Trying to color unknown object at
> 0xffff801060ef0000 as Black"
>
> Signed-off-by: Qian Cai <cai@lca.pw>

Why are you sending this to -mmotm?

Andrew, please disregard this patch. This is EFI/tip material.

> ---
>  drivers/firmware/efi/efi.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 7ac09dd8f268..4c46ff6f2242 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -31,7 +31,6 @@
>  #include <linux/acpi.h>
>  #include <linux/ucs2_string.h>
>  #include <linux/memblock.h>
> -#include <linux/kmemleak.h>
>
>  #include <asm/early_ioremap.h>
>
> @@ -1027,8 +1026,6 @@ int __ref efi_mem_reserve_persistent(phys_addr_t addr, u64 size)
>         if (!rsv)
>                 return -ENOMEM;
>
> -       kmemleak_ignore(rsv);
> -
>         rsv->size = EFI_MEMRESERVE_COUNT(PAGE_SIZE);
>         atomic_set(&rsv->count, 1);
>         rsv->entry[0].base = addr;

The patch that adds the kmemleak_ignore() call here is queued in
efi/urgent branch in the tip tree, but did not make it into v4.20.

efi/urgent does not apply cleanly to efi/core, since the kmalloc()
call [which requires the kmemleak_ignore() call] has been replaced
with alloc_pages() [which doesn't], necessitating this patch to remove
the kmemleak_ignore() call again.

So what I would like to suggest is that Ingo resolves this conflict by
simply dropping the call to kmemleak_ignore(). That way, we don't need
this patch, and we can still backport the efi/urgent change to
v4.20-stable.

Re: [PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Qian Cai]

On 12/26/18 7:02 AM, Ard Biesheuvel wrote:
> On Wed, 26 Dec 2018 at 03:35, Qian Cai <cai@lca.pw> wrote:
>>
>> a0fc5578f1d (efi: Let kmemleak ignore false positives) is no longer
>> needed due to efi_mem_reserve_persistent() uses __get_free_page()
>> instead where kmemelak is not able to track regardless. Otherwise,
>> kernel reported "kmemleak: Trying to color unknown object at
>> 0xffff801060ef0000 as Black"
>>
>> Signed-off-by: Qian Cai <cai@lca.pw>
> 
> Why are you sending this to -mmotm?
> 
> Andrew, please disregard this patch. This is EFI/tip material.

Well, I'd like to primarily develop on the -mmotm tree as it fits in a
sweet-spot where the mainline is too slow and linux-next is too chaotic.

The bug was reproduced and the patch was tested on -mmotm. If for every bugs
people found in -mmtom, they have to check out the corresponding sub-system tree
and reproduce/verify the bug over there, that is quite a burden to bear.

That's why sub-system maintainers are copied on those patches, so they can
decide to fix directly in the sub-system tree instead of -mmotm, and then it
will propagate to -mmotm one way or another.

Re: [PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Ard Biesheuvel]

On Wed, 26 Dec 2018 at 16:13, Qian Cai <cai@lca.pw> wrote:
>
> On 12/26/18 7:02 AM, Ard Biesheuvel wrote:
> > On Wed, 26 Dec 2018 at 03:35, Qian Cai <cai@lca.pw> wrote:
> >>
> >> a0fc5578f1d (efi: Let kmemleak ignore false positives) is no longer
> >> needed due to efi_mem_reserve_persistent() uses __get_free_page()
> >> instead where kmemelak is not able to track regardless. Otherwise,
> >> kernel reported "kmemleak: Trying to color unknown object at
> >> 0xffff801060ef0000 as Black"
> >>
> >> Signed-off-by: Qian Cai <cai@lca.pw>
> >
> > Why are you sending this to -mmotm?
> >
> > Andrew, please disregard this patch. This is EFI/tip material.
>
> Well, I'd like to primarily develop on the -mmotm tree as it fits in a
> sweet-spot where the mainline is too slow and linux-next is too chaotic.
>
> The bug was reproduced and the patch was tested on -mmotm. If for every bugs
> people found in -mmtom, they have to check out the corresponding sub-system tree
> and reproduce/verify the bug over there, that is quite a burden to bear.
>

Yes. But you know what? We all have our burden to bear, and shifting
this burden to someone else, in this case the subsystem maintainer who
typically deals with a sizable workload already, is not a very nice
thing to do.

> That's why sub-system maintainers are copied on those patches, so they can
> decide to fix directly in the sub-system tree instead of -mmotm, and then it
> will propagate to -mmotm one way or another.
>

Please stop sending EFI patches if you can't be bothered to
test/reproduce against the EFI tree.

Thanks,
Ard.

Re: [PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Andrew Morton]

On Wed, 26 Dec 2018 16:31:59 +0100 Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> Please stop sending EFI patches if you can't be bothered to
> test/reproduce against the EFI tree.

um, sorry, but that's a bit strong.  Finding (let alone fixing) a bug
in EFI is a great contribution (thanks!) and the EFI maintainers are
perfectly capable of reviewing and testing the proposed fix.  Or of
fixing the bug by other means.

Let's not beat people up for helping us in a less-than-perfect way, no?

Re: [PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Ard Biesheuvel]

On Fri, 28 Dec 2018 at 04:04, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 26 Dec 2018 16:31:59 +0100 Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> > Please stop sending EFI patches if you can't be bothered to
> > test/reproduce against the EFI tree.
>
> um, sorry, but that's a bit strong.  Finding (let alone fixing) a bug
> in EFI is a great contribution (thanks!) and the EFI maintainers are
> perfectly capable of reviewing and testing the proposed fix.  Or of
> fixing the bug by other means.
>

Qian did spot some issues recently, which was really helpful. But I
really think that reporting all issues you find against the -mmotm
tree because that happens to be your preferred tree for development is
not the correct approach.

> Let's not beat people up for helping us in a less-than-perfect way, no?

Fair enough. But asking people to ensure that an issue they found
actually exists on the subsystem tree in question is not that much to
ask, is it?

Re: [PATCH -mmotm] efi: drop kmemleak_ignore() for page allocator

[Qian Cai]

On 12/29/18 4:17 AM, Ard Biesheuvel wrote:
> On Fri, 28 Dec 2018 at 04:04, Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>> On Wed, 26 Dec 2018 16:31:59 +0100 Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>>> Please stop sending EFI patches if you can't be bothered to
>>> test/reproduce against the EFI tree.
>>
>> um, sorry, but that's a bit strong.  Finding (let alone fixing) a bug
>> in EFI is a great contribution (thanks!) and the EFI maintainers are
>> perfectly capable of reviewing and testing the proposed fix.  Or of
>> fixing the bug by other means.
>>
> 
> Qian did spot some issues recently, which was really helpful. But I
> really think that reporting all issues you find against the -mmotm
> tree because that happens to be your preferred tree for development is
> not the correct approach.
> 
>> Let's not beat people up for helping us in a less-than-perfect way, no?
> 
> Fair enough. But asking people to ensure that an issue they found
> actually exists on the subsystem tree in question is not that much to
> ask, is it?

It is not too much to ask to test on EFI subsystem tree only for this patch, but
if every maintainer asked for the same thing to test each subsystem tree after
found a bug even a trivial one in -mmotm or linux-next, it then become an issue.

There are people genuinely interested in the kernel in general rather than focus
on a few subsystems (yet). There are many subsystem git trees out there. It at
least needs to figure out which branch to test and adjust the config file
accordingly. Those subsystem trees usually are not well-documented like
linux-next or -mmotm trees. Then, they may need to deal with the subsystem
tree-specific issues.

Those people may just better switch to use mainline instead where they don't
need to bother testing the subsystem tree for every single patch. However, that
will cause delay in fixing those issues because mainline is usually a bit lag
behind the development.