* kmemleak reports firmware loader funnies in iwlwifi @ 2009-06-26 17:19 Dave Jones 2009-06-29 9:39 ` Catalin Marinas 0 siblings, 1 reply; 6+ messages in thread From: Dave Jones @ 2009-06-26 17:19 UTC (permalink / raw) To: linux-kernel; +Cc: Catalin Marinas, netdev After enabling kmemleak in the Fedora rawhide kernels, we've mostly seen a flood of what appear to be false positives, but the below looks really suspect.. https://bugzilla.redhat.com/show_bug.cgi?id=507971 Here's the summary... iwlagn 0000:03:00.0: loaded firmware version 8.24.2.12 kmemleak: Freeing unknown object at 0xffffc90018070000 Pid: 1034, comm: NetworkManager Not tainted 2.6.31-0.25.rc0.git22.fc12.x86_64 #1 Call Trace: [<ffffffff81139f74>] delete_object+0x5b/0x13b [<ffffffff8113b012>] kmemleak_free+0x5b/0xb5 [<ffffffff8111dc51>] vfree+0x40/0x68 [<ffffffff813485e6>] release_firmware+0x49/0x6c [<ffffffffa021997c>] ? iwl_mac_start+0xc5c/0x106b [iwlagn] [<ffffffffa0219adc>] iwl_mac_start+0xdbc/0x106b [iwlagn] [<ffffffff8109df9b>] ? __module_text_address+0x25/0x85 So it appears to be vfree'ing something that it had no knowledge of ever allocating. afaict _request_firmware only vmallocs when it's using a firmware image built into the driver, which isn't the case here, so I'm not sure why we end up trying to vfree instead of kfree when we call release_firmware anyone know what's going on here? Dave ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kmemleak reports firmware loader funnies in iwlwifi 2009-06-26 17:19 kmemleak reports firmware loader funnies in iwlwifi Dave Jones @ 2009-06-29 9:39 ` Catalin Marinas 2009-06-29 9:48 ` Pekka Enberg 0 siblings, 1 reply; 6+ messages in thread From: Catalin Marinas @ 2009-06-29 9:39 UTC (permalink / raw) To: Dave Jones; +Cc: linux-kernel, netdev Dave Jones <davej@redhat.com> wrote: > iwlagn 0000:03:00.0: loaded firmware version 8.24.2.12 > kmemleak: Freeing unknown object at 0xffffc90018070000 > Pid: 1034, comm: NetworkManager Not tainted 2.6.31-0.25.rc0.git22.fc12.x86_64 > #1 > Call Trace: > [<ffffffff81139f74>] delete_object+0x5b/0x13b > [<ffffffff8113b012>] kmemleak_free+0x5b/0xb5 > [<ffffffff8111dc51>] vfree+0x40/0x68 > [<ffffffff813485e6>] release_firmware+0x49/0x6c > [<ffffffffa021997c>] ? iwl_mac_start+0xc5c/0x106b [iwlagn] > [<ffffffffa0219adc>] iwl_mac_start+0xdbc/0x106b [iwlagn] > [<ffffffff8109df9b>] ? __module_text_address+0x25/0x85 > > > So it appears to be vfree'ing something that it had no knowledge of > ever allocating. afaict _request_firmware only vmallocs when it's > using a firmware image built into the driver, which isn't the case > here, so I'm not sure why we end up trying to vfree instead of kfree > when we call release_firmware It seems that firmware_loading_store uses vmap() to set fw->data and it later calls vfree() on this (maybe as a short-cut to vunmap + __free_page). Kmemleak doesn't track vmap allocations but tracks vmalloc/vfree calls. A solution here is to track vmap/vunmap calls (setting the block size to 0 or adding extra checks to make sure it doesn't scan I/O or user pages). Another solution (my preferred one) is to annotate some of the few places where vmap may be used with vfree. In this particular case: diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index ddeb819..26fb808 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -179,6 +179,13 @@ static ssize_t firmware_loading_store(struct device *dev, dev_err(dev, "%s: vmap() failed\n", __func__); goto err; } + /* + * This block of memory is later freed using vfree. + * Since kmemleak does not track vmap calls, just + * inform it about this block but ignore it during + * scanning. + */ + kmemleak_alloc(fw_priv->fw->data, 0, -1, GFP_KERNEL); /* Pages will be freed by vfree() */ fw_priv->pages = NULL; fw_priv->page_array_size = 0; -- Catalin ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: kmemleak reports firmware loader funnies in iwlwifi 2009-06-29 9:39 ` Catalin Marinas @ 2009-06-29 9:48 ` Pekka Enberg 2009-06-29 9:50 ` Catalin Marinas 2009-06-29 14:33 ` Catalin Marinas 0 siblings, 2 replies; 6+ messages in thread From: Pekka Enberg @ 2009-06-29 9:48 UTC (permalink / raw) To: Catalin Marinas; +Cc: Dave Jones, linux-kernel, netdev Hi Catalin, On Mon, Jun 29, 2009 at 12:39 PM, Catalin Marinas<catalin.marinas@arm.com> wrote: > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index ddeb819..26fb808 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -179,6 +179,13 @@ static ssize_t firmware_loading_store(struct device *dev, > dev_err(dev, "%s: vmap() failed\n", __func__); > goto err; > } > + /* > + * This block of memory is later freed using vfree. > + * Since kmemleak does not track vmap calls, just > + * inform it about this block but ignore it during > + * scanning. > + */ > + kmemleak_alloc(fw_priv->fw->data, 0, -1, GFP_KERNEL); > /* Pages will be freed by vfree() */ > fw_priv->pages = NULL; > fw_priv->page_array_size = 0; Would it be possible to put this hook in vmap() somehow? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kmemleak reports firmware loader funnies in iwlwifi 2009-06-29 9:48 ` Pekka Enberg @ 2009-06-29 9:50 ` Catalin Marinas 2009-06-29 14:33 ` Catalin Marinas 1 sibling, 0 replies; 6+ messages in thread From: Catalin Marinas @ 2009-06-29 9:50 UTC (permalink / raw) To: Pekka Enberg; +Cc: Dave Jones, linux-kernel, netdev On Mon, 2009-06-29 at 12:48 +0300, Pekka Enberg wrote: > Hi Catalin, > > On Mon, Jun 29, 2009 at 12:39 PM, Catalin > Marinas<catalin.marinas@arm.com> wrote: > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index ddeb819..26fb808 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -179,6 +179,13 @@ static ssize_t firmware_loading_store(struct device *dev, > > dev_err(dev, "%s: vmap() failed\n", __func__); > > goto err; > > } > > + /* > > + * This block of memory is later freed using vfree. > > + * Since kmemleak does not track vmap calls, just > > + * inform it about this block but ignore it during > > + * scanning. > > + */ > > + kmemleak_alloc(fw_priv->fw->data, 0, -1, GFP_KERNEL); > > /* Pages will be freed by vfree() */ > > fw_priv->pages = NULL; > > fw_priv->page_array_size = 0; > > Would it be possible to put this hook in vmap() somehow? It can be (and it could track vmap leaks as well). BTW, is there any use-case where vmap'ed memory may contain pointers? I did a grep but none of the vmap'ed blocks seem to have pointers. -- Catalin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kmemleak reports firmware loader funnies in iwlwifi 2009-06-29 9:48 ` Pekka Enberg 2009-06-29 9:50 ` Catalin Marinas @ 2009-06-29 14:33 ` Catalin Marinas 2009-06-29 14:35 ` Pekka Enberg 1 sibling, 1 reply; 6+ messages in thread From: Catalin Marinas @ 2009-06-29 14:33 UTC (permalink / raw) To: Pekka Enberg; +Cc: Dave Jones, linux-kernel, netdev On Mon, 2009-06-29 at 12:48 +0300, Pekka Enberg wrote: > Hi Catalin, > > On Mon, Jun 29, 2009 at 12:39 PM, Catalin > Marinas<catalin.marinas@arm.com> wrote: > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > > index ddeb819..26fb808 100644 > > --- a/drivers/base/firmware_class.c > > +++ b/drivers/base/firmware_class.c > > @@ -179,6 +179,13 @@ static ssize_t firmware_loading_store(struct device *dev, > > dev_err(dev, "%s: vmap() failed\n", __func__); > > goto err; > > } > > + /* > > + * This block of memory is later freed using vfree. > > + * Since kmemleak does not track vmap calls, just > > + * inform it about this block but ignore it during > > + * scanning. > > + */ > > + kmemleak_alloc(fw_priv->fw->data, 0, -1, GFP_KERNEL); > > /* Pages will be freed by vfree() */ > > fw_priv->pages = NULL; > > fw_priv->page_array_size = 0; > > Would it be possible to put this hook in vmap() somehow? I tried to do this but it has some other implications. If I add the vmap hook, I would need to add vunmap as well. On ARM, at least, iounmap calls vunmap (but not vmap) which means that I would need to add ioremap support as well. That's not a bug issue but this is more like a new feature than a bug fix. I propose that for now I disable the kmemleak warning for unknown pointers and add a patch to linux-next which tracks ioremap and vmap mappings. Does this sound fine? Thanks. -- Catalin ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: kmemleak reports firmware loader funnies in iwlwifi 2009-06-29 14:33 ` Catalin Marinas @ 2009-06-29 14:35 ` Pekka Enberg 0 siblings, 0 replies; 6+ messages in thread From: Pekka Enberg @ 2009-06-29 14:35 UTC (permalink / raw) To: Catalin Marinas; +Cc: Dave Jones, linux-kernel, netdev Hi Catalin, On Mon, Jun 29, 2009 at 5:33 PM, Catalin Marinas<catalin.marinas@arm.com> wrote: > I tried to do this but it has some other implications. If I add the vmap > hook, I would need to add vunmap as well. On ARM, at least, iounmap > calls vunmap (but not vmap) which means that I would need to add ioremap > support as well. That's not a bug issue but this is more like a new > feature than a bug fix. > > I propose that for now I disable the kmemleak warning for unknown > pointers and add a patch to linux-next which tracks ioremap and vmap > mappings. Does this sound fine? Yup, makes sense. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-06-29 14:35 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-06-26 17:19 kmemleak reports firmware loader funnies in iwlwifi Dave Jones 2009-06-29 9:39 ` Catalin Marinas 2009-06-29 9:48 ` Pekka Enberg 2009-06-29 9:50 ` Catalin Marinas 2009-06-29 14:33 ` Catalin Marinas 2009-06-29 14:35 ` Pekka Enberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).