| Message ID | 20181226023534.64048-1-cai@lca.pw (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-mmotm] efi: drop kmemleak_ignore() for page allocator | expand |
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.
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.
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.
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?
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?
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.
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;
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(-)