| Message ID | 20200219102122.1607365-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 2/19/20 12:20 PM, 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. > > 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 driver > > This 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> Thank you, Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.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); > + > 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;
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(). > 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 driver > > This 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 ? > + > 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;
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(). > > > 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 driver > > > > This 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). -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
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 driver > > > > > > This 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
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); + 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;
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. 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 driver This 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(-)