| Message ID | 1574234218-49195-5-git-send-email-decui@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Enhance pci-hyperv to support hibernation, and 2 misc fixes | expand |
On Tue, Nov 19, 2019 at 11:16:58PM -0800, Dexuan Cui wrote: > The page allocated for struct hv_pcibus_device contains pointers to other > slab allocations in new_pcichild_device(). Since kmemleak does not track > and scan page allocations, the slab objects will be reported as leaks > (false positives). Use the kmemleak APIs to allow tracking of such pages. > > Reported-by: Lili Deng <v-lide@microsoft.com> > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > This is actually v1. I use "v2" in the Subject just to be consistent with > the other patches in the patchset. That's a mistake, you should have posted patches separately. I need hyper-V ACKs on this series to get it through. Thanks, Lorenzo > Without the patch, we can see the below warning in dmesg, if kmemleak is > enabled: > > kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) > > and "cat /sys/kernel/debug/kmemleak" shows: > > unreferenced object 0xffff9217d1f2bec0 (size 192): > comm "kworker/u256:7", pid 100821, jiffies 4501481057 (age 61409.997s) > hex dump (first 32 bytes): > a8 60 b1 63 17 92 ff ff a8 60 b1 63 17 92 ff ff .`.c.....`.c.... > 02 00 00 00 00 00 00 00 80 92 cd 61 17 92 ff ff ...........a.... > backtrace: > [<0000000006f7ae93>] pci_devices_present_work+0x326/0x5e0 [pci_hyperv] > [<00000000278eb88a>] process_one_work+0x15f/0x360 > [<00000000c59501e6>] worker_thread+0x49/0x3c0 > [<000000000a0a7a94>] kthread+0xf8/0x130 > [<000000007075c2e7>] ret_from_fork+0x35/0x40 > > drivers/pci/controller/pci-hyperv.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index d7e05d436b5e..cc73f49c9e03 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -46,6 +46,7 @@ > #include <asm/irqdomain.h> > #include <asm/apic.h> > #include <linux/irq.h> > +#include <linux/kmemleak.h> > #include <linux/msi.h> > #include <linux/hyperv.h> > #include <linux/refcount.h> > @@ -2907,6 +2908,16 @@ static int hv_pci_probe(struct hv_device *hdev, > hbus = (struct hv_pcibus_device *)get_zeroed_page(GFP_KERNEL); > if (!hbus) > return -ENOMEM; > + > + /* > + * kmemleak doesn't track page allocations as they are not commonly > + * used for kernel data structures, but here hbus->children indeed > + * contains pointers to hv_pci_dev structs, which are dynamically > + * created in new_pcichild_device(). Allow kmemleak to scan the page > + * so we don't get a false warning of memory leak. > + */ > + kmemleak_alloc(hbus, PAGE_SIZE, 1, GFP_KERNEL); > + > hbus->state = hv_pcibus_init; > > /* > @@ -3133,6 +3144,8 @@ static int hv_pci_remove(struct hv_device *hdev) > > hv_put_dom_num(hbus->sysdata.domain); > > + /* Remove kmemleak object previously allocated in hv_pci_probe() */ > + kmemleak_free(hbus); > free_page((unsigned long)hbus); > return ret; > } > -- > 2.19.1 >
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Sent: Wednesday, November 20, 2019 9:12 AM > > On Tue, Nov 19, 2019 at 11:16:58PM -0800, Dexuan Cui wrote: > > The page allocated for struct hv_pcibus_device contains pointers to other > > slab allocations in new_pcichild_device(). Since kmemleak does not track > > and scan page allocations, the slab objects will be reported as leaks > > (false positives). Use the kmemleak APIs to allow tracking of such pages. > > > > Reported-by: Lili Deng <v-lide@microsoft.com> > > Signed-off-by: Dexuan Cui <decui@microsoft.com> > > --- > > > > This is actually v1. I use "v2" in the Subject just to be consistent with > > the other patches in the patchset. > > That's a mistake, you should have posted patches separately. I need Got it. I'll remember to do it separately in future. > hyper-V ACKs on this series to get it through. > > Thanks, > Lorenzo Sure. I'm asking the Hyper-V maintainers to review the patchset. Thanks, -- Dexuan
From: Dexuan Cui <decui@microsoft.com> Sent: Tuesday, November 19, 2019 11:17 PM > > The page allocated for struct hv_pcibus_device contains pointers to other > slab allocations in new_pcichild_device(). Since kmemleak does not track > and scan page allocations, the slab objects will be reported as leaks > (false positives). Use the kmemleak APIs to allow tracking of such pages. > > Reported-by: Lili Deng <v-lide@microsoft.com> > Signed-off-by: Dexuan Cui <decui@microsoft.com> > --- > > This is actually v1. I use "v2" in the Subject just to be consistent with > the other patches in the patchset. > > Without the patch, we can see the below warning in dmesg, if kmemleak is > enabled: > > kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) > > and "cat /sys/kernel/debug/kmemleak" shows: > > unreferenced object 0xffff9217d1f2bec0 (size 192): > comm "kworker/u256:7", pid 100821, jiffies 4501481057 (age 61409.997s) > hex dump (first 32 bytes): > a8 60 b1 63 17 92 ff ff a8 60 b1 63 17 92 ff ff .`.c.....`.c.... > 02 00 00 00 00 00 00 00 80 92 cd 61 17 92 ff ff ...........a.... > backtrace: > [<0000000006f7ae93>] pci_devices_present_work+0x326/0x5e0 [pci_hyperv] > [<00000000278eb88a>] process_one_work+0x15f/0x360 > [<00000000c59501e6>] worker_thread+0x49/0x3c0 > [<000000000a0a7a94>] kthread+0xf8/0x130 > [<000000007075c2e7>] ret_from_fork+0x35/0x40 > > drivers/pci/controller/pci-hyperv.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c > index d7e05d436b5e..cc73f49c9e03 100644 > --- a/drivers/pci/controller/pci-hyperv.c > +++ b/drivers/pci/controller/pci-hyperv.c > @@ -46,6 +46,7 @@ > #include <asm/irqdomain.h> > #include <asm/apic.h> > #include <linux/irq.h> > +#include <linux/kmemleak.h> > #include <linux/msi.h> > #include <linux/hyperv.h> > #include <linux/refcount.h> > @@ -2907,6 +2908,16 @@ static int hv_pci_probe(struct hv_device *hdev, > hbus = (struct hv_pcibus_device *)get_zeroed_page(GFP_KERNEL); > if (!hbus) > return -ENOMEM; > + > + /* > + * kmemleak doesn't track page allocations as they are not commonly > + * used for kernel data structures, but here hbus->children indeed > + * contains pointers to hv_pci_dev structs, which are dynamically > + * created in new_pcichild_device(). Allow kmemleak to scan the page > + * so we don't get a false warning of memory leak. > + */ > + kmemleak_alloc(hbus, PAGE_SIZE, 1, GFP_KERNEL); > + As noted in the existing comments, the hbus data structure must not cross a page boundary, so that a portion of it can be used as a hypercall argument. Historically kzalloc() didn't provide any alignment guarantee, so get_zeroed_page() is used. But a recent commit (59bb47985c1d) changes the behavior of kmalloc()/kzalloc() so that alignment *is* guaranteed for power of 2 allocation sizes. With this change, the better fix is to use kzalloc() instead of get_zeroed_page(). Note that the allocation size should be HV_HYP_PAGE_SIZE instead of PAGE_SIZE, as the hbus structure must not cross a page boundary based on Hyper-V's concept of a page, which may be different from the guest page size on some architectures. Michael > hbus->state = hv_pcibus_init; > > /* > @@ -3133,6 +3144,8 @@ static int hv_pci_remove(struct hv_device *hdev) > > hv_put_dom_num(hbus->sysdata.domain); > > + /* Remove kmemleak object previously allocated in hv_pci_probe() */ > + kmemleak_free(hbus); > free_page((unsigned long)hbus); > return ret; > } > -- > 2.19.1
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c index d7e05d436b5e..cc73f49c9e03 100644 --- a/drivers/pci/controller/pci-hyperv.c +++ b/drivers/pci/controller/pci-hyperv.c @@ -46,6 +46,7 @@ #include <asm/irqdomain.h> #include <asm/apic.h> #include <linux/irq.h> +#include <linux/kmemleak.h> #include <linux/msi.h> #include <linux/hyperv.h> #include <linux/refcount.h> @@ -2907,6 +2908,16 @@ static int hv_pci_probe(struct hv_device *hdev, hbus = (struct hv_pcibus_device *)get_zeroed_page(GFP_KERNEL); if (!hbus) return -ENOMEM; + + /* + * kmemleak doesn't track page allocations as they are not commonly + * used for kernel data structures, but here hbus->children indeed + * contains pointers to hv_pci_dev structs, which are dynamically + * created in new_pcichild_device(). Allow kmemleak to scan the page + * so we don't get a false warning of memory leak. + */ + kmemleak_alloc(hbus, PAGE_SIZE, 1, GFP_KERNEL); + hbus->state = hv_pcibus_init; /* @@ -3133,6 +3144,8 @@ static int hv_pci_remove(struct hv_device *hdev) hv_put_dom_num(hbus->sysdata.domain); + /* Remove kmemleak object previously allocated in hv_pci_probe() */ + kmemleak_free(hbus); free_page((unsigned long)hbus); return ret; }
The page allocated for struct hv_pcibus_device contains pointers to other slab allocations in new_pcichild_device(). Since kmemleak does not track and scan page allocations, the slab objects will be reported as leaks (false positives). Use the kmemleak APIs to allow tracking of such pages. Reported-by: Lili Deng <v-lide@microsoft.com> Signed-off-by: Dexuan Cui <decui@microsoft.com> --- This is actually v1. I use "v2" in the Subject just to be consistent with the other patches in the patchset. Without the patch, we can see the below warning in dmesg, if kmemleak is enabled: kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) and "cat /sys/kernel/debug/kmemleak" shows: unreferenced object 0xffff9217d1f2bec0 (size 192): comm "kworker/u256:7", pid 100821, jiffies 4501481057 (age 61409.997s) hex dump (first 32 bytes): a8 60 b1 63 17 92 ff ff a8 60 b1 63 17 92 ff ff .`.c.....`.c.... 02 00 00 00 00 00 00 00 80 92 cd 61 17 92 ff ff ...........a.... backtrace: [<0000000006f7ae93>] pci_devices_present_work+0x326/0x5e0 [pci_hyperv] [<00000000278eb88a>] process_one_work+0x15f/0x360 [<00000000c59501e6>] worker_thread+0x49/0x3c0 [<000000000a0a7a94>] kthread+0xf8/0x130 [<000000007075c2e7>] ret_from_fork+0x35/0x40 drivers/pci/controller/pci-hyperv.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)