On Wed, Feb 19, 2020 at 03:41:07PM +0100, Daniel Vetter wrote:
On Wed, Feb 19, 2020 at 2:39 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Daniel,
Thank you for the patch.
On Wed, Feb 19, 2020 at 11:20:34AM +0100, Daniel Vetter wrote:
I also did a full review of all callers, and only the xen driver forgot to call drm_dev_put in the failure path. Fix that up too.
I'd split this patch in two then, with the Xen first coming first, and with an explanation in the commit message of the second patch about why you call drmm_add_final_kfree() in drm_dev_alloc().
Forgot to reply to this I think.
Breaks biscting, so no can't split. It's just a leak, but this entire series is full of these "would break bisecting because it would introduce a subtle leak or use-after-free". Still isn't as simple as it looks unfortunately :-/
v2: I noticed that xen has a drm_driver.release hook, and uses drm_dev_alloc(). We need to remove the kfree from xen_drm_drv_release().
bochs also has a release hook, but leaked the drm_device ever since
commit 0a6659bdc5e8221da99eebb176fd9591435e38de Author: Gerd Hoffmann kraxel@redhat.com Date: Tue Dec 17 18:04:46 2013 +0100
drm/bochs: new driverThis patch here fixes that leak.
Same for virtio, started leaking with
commit b1df3a2b24a917f8853d43fe9683c0e360d2c33a Author: Gerd Hoffmann kraxel@redhat.com Date: Tue Feb 11 14:58:04 2020 +0100
drm/virtio: add drm_driver.release callback.Cc: Gerd Hoffmann kraxel@redhat.com Cc: Oleksandr Andrushchenko oleksandr_andrushchenko@epam.com Cc: xen-devel@lists.xenproject.org
Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Oleksandr Andrushchenko oleksandr_andrushchenko@epam.com Cc: xen-devel@lists.xenproject.org
drivers/gpu/drm/drm_drv.c | 3 +++ drivers/gpu/drm/xen/xen_drm_front.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 3e5627d6eba6..9e62e28bbc62 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -39,6 +39,7 @@ #include <drm/drm_color_mgmt.h> #include <drm/drm_drv.h> #include <drm/drm_file.h> +#include <drm/drm_managed.h> #include <drm/drm_mode_object.h> #include <drm/drm_print.h>
@@ -819,6 +820,8 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver, return ERR_PTR(ret); }
drmm_add_final_kfree(dev, dev);drmm_add_final_kfree() can only be called once. Does this mean that a driver using drm_dev_alloc() isn't allowed to use drmm_add_final_kfree() to tract its own private structure ?
There is only _one_ final kfree() for the structure containing drm_device. Anything else you can just allocate with drmm_kzalloc, and it will be cleaned up before. The chicken/egg doesn't just exist around init time with drm_device, but also at cleanup time - the list of cleanup actions is stored in drm_device, plus the logging macros also need a drm_device. Which means we really, really, really need to make sure that the drm_device is the very last thing that goes away. Hence this special case. I was semi-tempted to drill through the slab debug layer and add a check that the drm_device pointer in the final_kfree is actually within the slab allocation block. Just to make sure people use this correctly, and not just as a "hey here's a random kmalloc block I want you to release, thxokbye". Because doing that would cause a few use-after-free (or a leak).
I've added those checks now. -Daniel
-Daniel
return dev;} EXPORT_SYMBOL(drm_dev_alloc); diff --git a/drivers/gpu/drm/xen/xen_drm_front.c b/drivers/gpu/drm/xen/xen_drm_front.c index 4be49c1aef51..d22b5da38935 100644 --- a/drivers/gpu/drm/xen/xen_drm_front.c +++ b/drivers/gpu/drm/xen/xen_drm_front.c @@ -461,7 +461,6 @@ static void xen_drm_drv_release(struct drm_device *dev) drm_mode_config_cleanup(dev);
drm_dev_fini(dev);
kfree(dev); if (front_info->cfg.be_alloc) xenbus_switch_state(front_info->xb_dev,@@ -561,6 +560,7 @@ static int xen_drm_drv_init(struct xen_drm_front_info *front_info) fail_modeset: drm_kms_helper_poll_fini(drm_dev); drm_mode_config_cleanup(drm_dev);
drm_dev_put(drm_dev);fail: kfree(drm_info); return ret;
-- Regards,
Laurent Pinchart
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch