* [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
@ 2020-10-27 13:51 Hans de Goede
2020-10-27 15:23 ` Daniel Vetter
2020-11-19 11:35 ` Hans de Goede
0 siblings, 2 replies; 9+ messages in thread
From: Hans de Goede @ 2020-10-27 13:51 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Daniel Vetter, David Airlie
Cc: Hans de Goede, Dan Carpenter, kernel test robot, dri-devel
Add missing pci_iounmap() calls to properly unmap the memory on
probe-failure and remove.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/gpu/drm/vboxvideo/vbox_main.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c b/drivers/gpu/drm/vboxvideo/vbox_main.c
index d68d9bad7674..2eeb1d3be54a 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_main.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
@@ -71,6 +71,8 @@ static void vbox_accel_fini(struct vbox_private *vbox)
for (i = 0; i < vbox->num_crtcs; ++i)
vbva_disable(&vbox->vbva_info[i], vbox->guest_pool, i);
+
+ pci_iounmap(vbox->ddev.pdev, vbox->vbva_buffers);
}
/* Do we support the 4.3 plus mode hint reporting interface? */
@@ -124,19 +126,19 @@ int vbox_hw_init(struct vbox_private *vbox)
vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
"vboxvideo-accel");
if (!vbox->guest_pool)
- return -ENOMEM;
+ goto out_unmap_guest_heap;
ret = gen_pool_add_virt(vbox->guest_pool,
(unsigned long)vbox->guest_heap,
GUEST_HEAP_OFFSET(vbox),
GUEST_HEAP_USABLE_SIZE, -1);
if (ret)
- return ret;
+ goto out_unmap_guest_heap;
ret = hgsmi_test_query_conf(vbox->guest_pool);
if (ret) {
DRM_ERROR("vboxvideo: hgsmi_test_query_conf failed\n");
- return ret;
+ goto out_unmap_guest_heap;
}
/* Reduce available VRAM size to reflect the guest heap. */
@@ -148,23 +150,30 @@ int vbox_hw_init(struct vbox_private *vbox)
if (!have_hgsmi_mode_hints(vbox)) {
ret = -ENOTSUPP;
- return ret;
+ goto out_unmap_guest_heap;
}
vbox->last_mode_hints = devm_kcalloc(vbox->ddev.dev, vbox->num_crtcs,
sizeof(struct vbva_modehint),
GFP_KERNEL);
- if (!vbox->last_mode_hints)
- return -ENOMEM;
+ if (!vbox->last_mode_hints) {
+ ret = -ENOMEM;
+ goto out_unmap_guest_heap;
+ }
ret = vbox_accel_init(vbox);
if (ret)
- return ret;
+ goto out_unmap_guest_heap;
return 0;
+
+out_unmap_guest_heap:
+ pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
+ return ret;
}
void vbox_hw_fini(struct vbox_private *vbox)
{
vbox_accel_fini(vbox);
+ pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
}
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-10-27 13:51 [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove Hans de Goede
@ 2020-10-27 15:23 ` Daniel Vetter
2020-11-19 11:35 ` Hans de Goede
1 sibling, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2020-10-27 15:23 UTC (permalink / raw)
To: Hans de Goede
Cc: kernel test robot, David Airlie, dri-devel, Thomas Zimmermann,
Dan Carpenter
On Tue, Oct 27, 2020 at 2:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add missing pci_iounmap() calls to properly unmap the memory on
> probe-failure and remove.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
I think switching over to devm would be really nice. And for pci all
you need to do is use pcim_enable_device and delete all the cleanup
code, and it's all done. Hand rolling device cleanup code really isn't
a great idea and way too error-prone. Plus you're using lots of devm_
already.
-Daniel
> ---
> drivers/gpu/drm/vboxvideo/vbox_main.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index d68d9bad7674..2eeb1d3be54a 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -71,6 +71,8 @@ static void vbox_accel_fini(struct vbox_private *vbox)
>
> for (i = 0; i < vbox->num_crtcs; ++i)
> vbva_disable(&vbox->vbva_info[i], vbox->guest_pool, i);
> +
> + pci_iounmap(vbox->ddev.pdev, vbox->vbva_buffers);
> }
>
> /* Do we support the 4.3 plus mode hint reporting interface? */
> @@ -124,19 +126,19 @@ int vbox_hw_init(struct vbox_private *vbox)
> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
> "vboxvideo-accel");
> if (!vbox->guest_pool)
> - return -ENOMEM;
> + goto out_unmap_guest_heap;
>
> ret = gen_pool_add_virt(vbox->guest_pool,
> (unsigned long)vbox->guest_heap,
> GUEST_HEAP_OFFSET(vbox),
> GUEST_HEAP_USABLE_SIZE, -1);
> if (ret)
> - return ret;
> + goto out_unmap_guest_heap;
>
> ret = hgsmi_test_query_conf(vbox->guest_pool);
> if (ret) {
> DRM_ERROR("vboxvideo: hgsmi_test_query_conf failed\n");
> - return ret;
> + goto out_unmap_guest_heap;
> }
>
> /* Reduce available VRAM size to reflect the guest heap. */
> @@ -148,23 +150,30 @@ int vbox_hw_init(struct vbox_private *vbox)
>
> if (!have_hgsmi_mode_hints(vbox)) {
> ret = -ENOTSUPP;
> - return ret;
> + goto out_unmap_guest_heap;
> }
>
> vbox->last_mode_hints = devm_kcalloc(vbox->ddev.dev, vbox->num_crtcs,
> sizeof(struct vbva_modehint),
> GFP_KERNEL);
> - if (!vbox->last_mode_hints)
> - return -ENOMEM;
> + if (!vbox->last_mode_hints) {
> + ret = -ENOMEM;
> + goto out_unmap_guest_heap;
> + }
>
> ret = vbox_accel_init(vbox);
> if (ret)
> - return ret;
> + goto out_unmap_guest_heap;
>
> return 0;
> +
> +out_unmap_guest_heap:
> + pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
> + return ret;
> }
>
> void vbox_hw_fini(struct vbox_private *vbox)
> {
> vbox_accel_fini(vbox);
> + pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
> }
> --
> 2.28.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-10-27 13:51 [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove Hans de Goede
2020-10-27 15:23 ` Daniel Vetter
@ 2020-11-19 11:35 ` Hans de Goede
2020-11-19 17:51 ` Dan Carpenter
1 sibling, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2020-11-19 11:35 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
Daniel Vetter, David Airlie
Cc: Dan Carpenter, kernel test robot, dri-devel
Hi,
On 10/27/20 2:51 PM, Hans de Goede wrote:
> Add missing pci_iounmap() calls to properly unmap the memory on
> probe-failure and remove.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
For some reason the spam-filter used by Red Hat's email system has eaten
Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
Daniel Vetter wrote:
> I think switching over to devm would be really nice. And for pci all
> you need to do is use pcim_enable_device and delete all the cleanup
> code, and it's all done. Hand rolling device cleanup code really isn't
> a great idea and way too error-prone. Plus you're using lots of devm_
> already.
Good point, so I just checked and the vboxvideo code is already
using pcim_enable_device() so it looks like this is a false-positive
from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
makes all subsequent pci-resource acquiring calls behave like devm calls,
when he forwarded the report to me.
Tl;DR: there is no bug / leak and this patch can be dropped.
Is there a place where I can report a bug against the lkp@intel.com bot
for this false-positive ?
Regards,
Hans
> ---
> drivers/gpu/drm/vboxvideo/vbox_main.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_main.c b/drivers/gpu/drm/vboxvideo/vbox_main.c
> index d68d9bad7674..2eeb1d3be54a 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_main.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_main.c
> @@ -71,6 +71,8 @@ static void vbox_accel_fini(struct vbox_private *vbox)
>
> for (i = 0; i < vbox->num_crtcs; ++i)
> vbva_disable(&vbox->vbva_info[i], vbox->guest_pool, i);
> +
> + pci_iounmap(vbox->ddev.pdev, vbox->vbva_buffers);
> }
>
> /* Do we support the 4.3 plus mode hint reporting interface? */
> @@ -124,19 +126,19 @@ int vbox_hw_init(struct vbox_private *vbox)
> vbox->guest_pool = devm_gen_pool_create(vbox->ddev.dev, 4, -1,
> "vboxvideo-accel");
> if (!vbox->guest_pool)
> - return -ENOMEM;
> + goto out_unmap_guest_heap;
>
> ret = gen_pool_add_virt(vbox->guest_pool,
> (unsigned long)vbox->guest_heap,
> GUEST_HEAP_OFFSET(vbox),
> GUEST_HEAP_USABLE_SIZE, -1);
> if (ret)
> - return ret;
> + goto out_unmap_guest_heap;
>
> ret = hgsmi_test_query_conf(vbox->guest_pool);
> if (ret) {
> DRM_ERROR("vboxvideo: hgsmi_test_query_conf failed\n");
> - return ret;
> + goto out_unmap_guest_heap;
> }
>
> /* Reduce available VRAM size to reflect the guest heap. */
> @@ -148,23 +150,30 @@ int vbox_hw_init(struct vbox_private *vbox)
>
> if (!have_hgsmi_mode_hints(vbox)) {
> ret = -ENOTSUPP;
> - return ret;
> + goto out_unmap_guest_heap;
> }
>
> vbox->last_mode_hints = devm_kcalloc(vbox->ddev.dev, vbox->num_crtcs,
> sizeof(struct vbva_modehint),
> GFP_KERNEL);
> - if (!vbox->last_mode_hints)
> - return -ENOMEM;
> + if (!vbox->last_mode_hints) {
> + ret = -ENOMEM;
> + goto out_unmap_guest_heap;
> + }
>
> ret = vbox_accel_init(vbox);
> if (ret)
> - return ret;
> + goto out_unmap_guest_heap;
>
> return 0;
> +
> +out_unmap_guest_heap:
> + pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
> + return ret;
> }
>
> void vbox_hw_fini(struct vbox_private *vbox)
> {
> vbox_accel_fini(vbox);
> + pci_iounmap(vbox->ddev.pdev, vbox->guest_pool);
> }
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-11-19 11:35 ` Hans de Goede
@ 2020-11-19 17:51 ` Dan Carpenter
2020-11-19 19:30 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2020-11-19 17:51 UTC (permalink / raw)
To: Hans de Goede
Cc: Thomas Zimmermann, David Airlie, dri-devel, kernel test robot
On Thu, Nov 19, 2020 at 12:35:56PM +0100, Hans de Goede wrote:
> Hi,
>
> On 10/27/20 2:51 PM, Hans de Goede wrote:
> > Add missing pci_iounmap() calls to properly unmap the memory on
> > probe-failure and remove.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> For some reason the spam-filter used by Red Hat's email system has eaten
> Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
>
> Daniel Vetter wrote:
>
> > I think switching over to devm would be really nice. And for pci all
> > you need to do is use pcim_enable_device and delete all the cleanup
> > code, and it's all done. Hand rolling device cleanup code really isn't
> > a great idea and way too error-prone. Plus you're using lots of devm_
> > already.
>
> Good point, so I just checked and the vboxvideo code is already
> using pcim_enable_device() so it looks like this is a false-positive
> from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
> makes all subsequent pci-resource acquiring calls behave like devm calls,
> when he forwarded the report to me.
>
> Tl;DR: there is no bug / leak and this patch can be dropped.
>
> Is there a place where I can report a bug against the lkp@intel.com bot
> for this false-positive ?
Ah. Thanks!
This is a Smatch bug. There is a list for that smatch@vger.kernel.org
but I already remove the pci_iomap() from the list of functions that
needs to be unwound based on your report.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-11-19 17:51 ` Dan Carpenter
@ 2020-11-19 19:30 ` Daniel Vetter
2020-11-20 7:58 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2020-11-19 19:30 UTC (permalink / raw)
To: Dan Carpenter
Cc: kernel test robot, David Airlie, dri-devel, Hans de Goede,
Thomas Zimmermann
On Thu, Nov 19, 2020 at 6:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, Nov 19, 2020 at 12:35:56PM +0100, Hans de Goede wrote:
> > Hi,
> >
> > On 10/27/20 2:51 PM, Hans de Goede wrote:
> > > Add missing pci_iounmap() calls to properly unmap the memory on
> > > probe-failure and remove.
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >
> > For some reason the spam-filter used by Red Hat's email system has eaten
> > Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
> >
> > Daniel Vetter wrote:
> >
> > > I think switching over to devm would be really nice. And for pci all
> > > you need to do is use pcim_enable_device and delete all the cleanup
> > > code, and it's all done. Hand rolling device cleanup code really isn't
> > > a great idea and way too error-prone. Plus you're using lots of devm_
> > > already.
> >
> > Good point, so I just checked and the vboxvideo code is already
> > using pcim_enable_device() so it looks like this is a false-positive
> > from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
> > makes all subsequent pci-resource acquiring calls behave like devm calls,
> > when he forwarded the report to me.
> >
> > Tl;DR: there is no bug / leak and this patch can be dropped.
> >
> > Is there a place where I can report a bug against the lkp@intel.com bot
> > for this false-positive ?
>
> Ah. Thanks!
>
> This is a Smatch bug. There is a list for that smatch@vger.kernel.org
> but I already remove the pci_iomap() from the list of functions that
> needs to be unwound based on your report.
I guess if smatch sees a pci_enable_device but not pcim_enable_device
on the same device as passed to pci_iomap (and a pile of other pci
functions) then it still must be unwound. Could smatch detect that?
There's a lot of pci drivers not using the managed functions, catching
bugs in these would be good.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-11-19 19:30 ` Daniel Vetter
@ 2020-11-20 7:58 ` Dan Carpenter
2020-11-20 9:39 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2020-11-20 7:58 UTC (permalink / raw)
To: Daniel Vetter
Cc: kernel test robot, David Airlie, dri-devel, Hans de Goede,
Thomas Zimmermann
On Thu, Nov 19, 2020 at 08:30:59PM +0100, Daniel Vetter wrote:
> On Thu, Nov 19, 2020 at 6:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, Nov 19, 2020 at 12:35:56PM +0100, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 10/27/20 2:51 PM, Hans de Goede wrote:
> > > > Add missing pci_iounmap() calls to properly unmap the memory on
> > > > probe-failure and remove.
> > > >
> > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >
> > > For some reason the spam-filter used by Red Hat's email system has eaten
> > > Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
> > >
> > > Daniel Vetter wrote:
> > >
> > > > I think switching over to devm would be really nice. And for pci all
> > > > you need to do is use pcim_enable_device and delete all the cleanup
> > > > code, and it's all done. Hand rolling device cleanup code really isn't
> > > > a great idea and way too error-prone. Plus you're using lots of devm_
> > > > already.
> > >
> > > Good point, so I just checked and the vboxvideo code is already
> > > using pcim_enable_device() so it looks like this is a false-positive
> > > from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
> > > makes all subsequent pci-resource acquiring calls behave like devm calls,
> > > when he forwarded the report to me.
> > >
> > > Tl;DR: there is no bug / leak and this patch can be dropped.
> > >
> > > Is there a place where I can report a bug against the lkp@intel.com bot
> > > for this false-positive ?
> >
> > Ah. Thanks!
> >
> > This is a Smatch bug. There is a list for that smatch@vger.kernel.org
> > but I already remove the pci_iomap() from the list of functions that
> > needs to be unwound based on your report.
>
> I guess if smatch sees a pci_enable_device but not pcim_enable_device
> on the same device as passed to pci_iomap (and a pile of other pci
> functions) then it still must be unwound. Could smatch detect that?
> There's a lot of pci drivers not using the managed functions, catching
> bugs in these would be good.
It's a lot of code. There would be two ways to implement this:
1) Somehow store the links to figure out the value of:
devres_find(vbox->ddev.pdev.dev)->enabled
That's very complicated. I'm sort of working on some of the steps
involved but and it's probably a multi year process before it's
possible.
2) Create a data base table with driver data, then store if the driver
calls pcim_enable_device(). This is still a bit of work, but probably
straight forward. Storing driver data would be useful for other things
as well.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-11-20 7:58 ` Dan Carpenter
@ 2020-11-20 9:39 ` Daniel Vetter
2020-11-20 11:00 ` Dan Carpenter
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2020-11-20 9:39 UTC (permalink / raw)
To: Dan Carpenter
Cc: kernel test robot, David Airlie, dri-devel, Hans de Goede,
Thomas Zimmermann
On Fri, Nov 20, 2020 at 8:58 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Thu, Nov 19, 2020 at 08:30:59PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 19, 2020 at 6:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 12:35:56PM +0100, Hans de Goede wrote:
> > > > Hi,
> > > >
> > > > On 10/27/20 2:51 PM, Hans de Goede wrote:
> > > > > Add missing pci_iounmap() calls to properly unmap the memory on
> > > > > probe-failure and remove.
> > > > >
> > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > >
> > > > For some reason the spam-filter used by Red Hat's email system has eaten
> > > > Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
> > > >
> > > > Daniel Vetter wrote:
> > > >
> > > > > I think switching over to devm would be really nice. And for pci all
> > > > > you need to do is use pcim_enable_device and delete all the cleanup
> > > > > code, and it's all done. Hand rolling device cleanup code really isn't
> > > > > a great idea and way too error-prone. Plus you're using lots of devm_
> > > > > already.
> > > >
> > > > Good point, so I just checked and the vboxvideo code is already
> > > > using pcim_enable_device() so it looks like this is a false-positive
> > > > from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
> > > > makes all subsequent pci-resource acquiring calls behave like devm calls,
> > > > when he forwarded the report to me.
> > > >
> > > > Tl;DR: there is no bug / leak and this patch can be dropped.
> > > >
> > > > Is there a place where I can report a bug against the lkp@intel.com bot
> > > > for this false-positive ?
> > >
> > > Ah. Thanks!
> > >
> > > This is a Smatch bug. There is a list for that smatch@vger.kernel.org
> > > but I already remove the pci_iomap() from the list of functions that
> > > needs to be unwound based on your report.
> >
> > I guess if smatch sees a pci_enable_device but not pcim_enable_device
> > on the same device as passed to pci_iomap (and a pile of other pci
> > functions) then it still must be unwound. Could smatch detect that?
> > There's a lot of pci drivers not using the managed functions, catching
> > bugs in these would be good.
>
> It's a lot of code. There would be two ways to implement this:
>
> 1) Somehow store the links to figure out the value of:
>
> devres_find(vbox->ddev.pdev.dev)->enabled
>
> That's very complicated. I'm sort of working on some of the steps
> involved but and it's probably a multi year process before it's
> possible.
>
> 2) Create a data base table with driver data, then store if the driver
> calls pcim_enable_device(). This is still a bit of work, but probably
> straight forward. Storing driver data would be useful for other things
> as well.
Hm maybe I totally misunderstand how smatch works, but I thought you
can track additional invariants and stuff on various pointers. So I
figured you just track whether pci_enable_device has been called on a
struct pci_device *dev, and then if anyone calls pci_iomap on the same
pointer and there's no cleanup, it's a bug. For any other case you
just can't tell (since absence of pcim_enable_device might just mean
that smatch doesn't see through the maze). Or is that what you meant
with approach 2?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-11-20 9:39 ` Daniel Vetter
@ 2020-11-20 11:00 ` Dan Carpenter
2020-11-20 14:55 ` Daniel Vetter
0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2020-11-20 11:00 UTC (permalink / raw)
To: Daniel Vetter
Cc: kernel test robot, David Airlie, dri-devel, Hans de Goede,
Thomas Zimmermann
On Fri, Nov 20, 2020 at 10:39:45AM +0100, Daniel Vetter wrote:
> On Fri, Nov 20, 2020 at 8:58 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Thu, Nov 19, 2020 at 08:30:59PM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 19, 2020 at 6:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > >
> > > > On Thu, Nov 19, 2020 at 12:35:56PM +0100, Hans de Goede wrote:
> > > > > Hi,
> > > > >
> > > > > On 10/27/20 2:51 PM, Hans de Goede wrote:
> > > > > > Add missing pci_iounmap() calls to properly unmap the memory on
> > > > > > probe-failure and remove.
> > > > > >
> > > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > >
> > > > > For some reason the spam-filter used by Red Hat's email system has eaten
> > > > > Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
> > > > >
> > > > > Daniel Vetter wrote:
> > > > >
> > > > > > I think switching over to devm would be really nice. And for pci all
> > > > > > you need to do is use pcim_enable_device and delete all the cleanup
> > > > > > code, and it's all done. Hand rolling device cleanup code really isn't
> > > > > > a great idea and way too error-prone. Plus you're using lots of devm_
> > > > > > already.
> > > > >
> > > > > Good point, so I just checked and the vboxvideo code is already
> > > > > using pcim_enable_device() so it looks like this is a false-positive
> > > > > from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
> > > > > makes all subsequent pci-resource acquiring calls behave like devm calls,
> > > > > when he forwarded the report to me.
> > > > >
> > > > > Tl;DR: there is no bug / leak and this patch can be dropped.
> > > > >
> > > > > Is there a place where I can report a bug against the lkp@intel.com bot
> > > > > for this false-positive ?
> > > >
> > > > Ah. Thanks!
> > > >
> > > > This is a Smatch bug. There is a list for that smatch@vger.kernel.org
> > > > but I already remove the pci_iomap() from the list of functions that
> > > > needs to be unwound based on your report.
> > >
> > > I guess if smatch sees a pci_enable_device but not pcim_enable_device
> > > on the same device as passed to pci_iomap (and a pile of other pci
> > > functions) then it still must be unwound. Could smatch detect that?
> > > There's a lot of pci drivers not using the managed functions, catching
> > > bugs in these would be good.
> >
> > It's a lot of code. There would be two ways to implement this:
> >
> > 1) Somehow store the links to figure out the value of:
> >
> > devres_find(vbox->ddev.pdev.dev)->enabled
> >
> > That's very complicated. I'm sort of working on some of the steps
> > involved but and it's probably a multi year process before it's
> > possible.
> >
> > 2) Create a data base table with driver data, then store if the driver
> > calls pcim_enable_device(). This is still a bit of work, but probably
> > straight forward. Storing driver data would be useful for other things
> > as well.
>
> Hm maybe I totally misunderstand how smatch works, but I thought you
> can track additional invariants and stuff on various pointers. So I
> figured you just track whether pci_enable_device has been called on a
> struct pci_device *dev, and then if anyone calls pci_iomap on the same
> pointer and there's no cleanup, it's a bug. For any other case you
> just can't tell (since absence of pcim_enable_device might just mean
> that smatch doesn't see through the maze). Or is that what you meant
> with approach 2?
Hm... Your idea is another option #3. It's probably less work.
I'll take a look at it.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove
2020-11-20 11:00 ` Dan Carpenter
@ 2020-11-20 14:55 ` Daniel Vetter
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Vetter @ 2020-11-20 14:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Thomas Zimmermann, David Airlie, Hans de Goede, dri-devel,
kernel test robot
On Fri, Nov 20, 2020 at 02:00:55PM +0300, Dan Carpenter wrote:
> On Fri, Nov 20, 2020 at 10:39:45AM +0100, Daniel Vetter wrote:
> > On Fri, Nov 20, 2020 at 8:58 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Thu, Nov 19, 2020 at 08:30:59PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 19, 2020 at 6:51 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > > > >
> > > > > On Thu, Nov 19, 2020 at 12:35:56PM +0100, Hans de Goede wrote:
> > > > > > Hi,
> > > > > >
> > > > > > On 10/27/20 2:51 PM, Hans de Goede wrote:
> > > > > > > Add missing pci_iounmap() calls to properly unmap the memory on
> > > > > > > probe-failure and remove.
> > > > > > >
> > > > > > > Reported-by: kernel test robot <lkp@intel.com>
> > > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > >
> > > > > > For some reason the spam-filter used by Red Hat's email system has eaten
> > > > > > Daniel Vetter's reply to this, so let me copy and paste that from patchwork:
> > > > > >
> > > > > > Daniel Vetter wrote:
> > > > > >
> > > > > > > I think switching over to devm would be really nice. And for pci all
> > > > > > > you need to do is use pcim_enable_device and delete all the cleanup
> > > > > > > code, and it's all done. Hand rolling device cleanup code really isn't
> > > > > > > a great idea and way too error-prone. Plus you're using lots of devm_
> > > > > > > already.
> > > > > >
> > > > > > Good point, so I just checked and the vboxvideo code is already
> > > > > > using pcim_enable_device() so it looks like this is a false-positive
> > > > > > from the lkp@intel.com bot, and Dan Carpenter missed that pcim_enable_device()
> > > > > > makes all subsequent pci-resource acquiring calls behave like devm calls,
> > > > > > when he forwarded the report to me.
> > > > > >
> > > > > > Tl;DR: there is no bug / leak and this patch can be dropped.
> > > > > >
> > > > > > Is there a place where I can report a bug against the lkp@intel.com bot
> > > > > > for this false-positive ?
> > > > >
> > > > > Ah. Thanks!
> > > > >
> > > > > This is a Smatch bug. There is a list for that smatch@vger.kernel.org
> > > > > but I already remove the pci_iomap() from the list of functions that
> > > > > needs to be unwound based on your report.
> > > >
> > > > I guess if smatch sees a pci_enable_device but not pcim_enable_device
> > > > on the same device as passed to pci_iomap (and a pile of other pci
> > > > functions) then it still must be unwound. Could smatch detect that?
> > > > There's a lot of pci drivers not using the managed functions, catching
> > > > bugs in these would be good.
> > >
> > > It's a lot of code. There would be two ways to implement this:
> > >
> > > 1) Somehow store the links to figure out the value of:
> > >
> > > devres_find(vbox->ddev.pdev.dev)->enabled
> > >
> > > That's very complicated. I'm sort of working on some of the steps
> > > involved but and it's probably a multi year process before it's
> > > possible.
> > >
> > > 2) Create a data base table with driver data, then store if the driver
> > > calls pcim_enable_device(). This is still a bit of work, but probably
> > > straight forward. Storing driver data would be useful for other things
> > > as well.
> >
> > Hm maybe I totally misunderstand how smatch works, but I thought you
> > can track additional invariants and stuff on various pointers. So I
> > figured you just track whether pci_enable_device has been called on a
> > struct pci_device *dev, and then if anyone calls pci_iomap on the same
> > pointer and there's no cleanup, it's a bug. For any other case you
> > just can't tell (since absence of pcim_enable_device might just mean
> > that smatch doesn't see through the maze). Or is that what you meant
> > with approach 2?
>
> Hm... Your idea is another option #3. It's probably less work.
>
> I'll take a look at it.
btw if you do this, the inverse isn't an issue. I.e. a pcim_enable_device
on the same pci_device you see a pci_iounmap. This can happen when a
driver maps something just to check a few things, reliazes the feature
isn't there, and then drops the mapping.
It's only redundant when it's on a direct return path of the driver's
pci_driver->probe function, or anything that's only called from
pci_driver->remove. So quite tricky to correctly catch all cases.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-20 14:55 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-27 13:51 [PATCH] drm/vboxvideo: Unmap iomem on probe-failure and remove Hans de Goede
2020-10-27 15:23 ` Daniel Vetter
2020-11-19 11:35 ` Hans de Goede
2020-11-19 17:51 ` Dan Carpenter
2020-11-19 19:30 ` Daniel Vetter
2020-11-20 7:58 ` Dan Carpenter
2020-11-20 9:39 ` Daniel Vetter
2020-11-20 11:00 ` Dan Carpenter
2020-11-20 14:55 ` Daniel Vetter
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).