Re: [PATCH] drm: manage drm_minor cleanup with drmm_
On Tue, Mar 24, 2020 at 10:42 PM Sam Ravnborg wrote:
>
> On Tue, Mar 24, 2020 at 09:39:36PM +0100, Daniel Vetter wrote:
> > The cleanup here is somewhat tricky, since we can't tell apart the
> > allocated minor index from 0. So register a cleanup action first, and
> > if the index allocation fails, unregister that cleanup action again to
> > avoid bad mistakes.
> >
> > The kdev for the minor already handles NULL, so no problem there.
> >
> > Hence add drmm_remove_action() to the drm_managed library.
> >
> > v2: Make pointer math around void ** consistent with what Laurent
> > suggested.
> >
> > v3: Use drmm_add_action_or_reset and remove drmm_remove_action. Noticed
> > because of some questions from Thomas. This also means we need to move
> > the drmm_add_action_or_reset helper earlier in the series.
> >
> > v4: Uh ... fix slightly embarrassing bug CI spotted.
> Looks like the one I spotted in my review.
> Saw your mail only after posting.
Yup, but thanks for spotting it in your review too, gives me lots of
confidence that you really checked all the details - I was totally
blind and took me an afternoon with the clue bat to find the bug I
created :-)
> One Q below.
>
> >
> > Cc: Thomas Zimmermann
> > Cc: Laurent Pinchart
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/drm_drv.c | 69 ---
> > drivers/gpu/drm/drm_managed.c | 14 +++
> > include/drm/drm_managed.h | 9 -
> > 3 files changed, 46 insertions(+), 46 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index a710c53d13a8..50c56ff24c71 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -93,13 +93,25 @@ static struct drm_minor **drm_minor_get_slot(struct
> > drm_device *dev,
> > }
> > }
> >
> > +static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> > +{
> > + struct drm_minor *minor = data;
> > + unsigned long flags;
> > +
> > + put_device(minor->kdev);
> > +
> > + spin_lock_irqsave(_minor_lock, flags);
> > + idr_remove(_minors_idr, minor->index);
> > + spin_unlock_irqrestore(_minor_lock, flags);
> > +}
> > +
> > static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> > {
> > struct drm_minor *minor;
> > unsigned long flags;
> > int r;
> >
> > - minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> > + minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> > if (!minor)
> > return -ENOMEM;
> >
> > @@ -117,46 +129,20 @@ static int drm_minor_alloc(struct drm_device *dev,
> > unsigned int type)
> > idr_preload_end();
> >
> > if (r < 0)
> > - goto err_free;
> > + return r;
> >
> > minor->index = r;
> >
> > + r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
> > + if (r)
> > + return r;
> > +
> > minor->kdev = drm_sysfs_minor_alloc(minor);
> > - if (IS_ERR(minor->kdev)) {
> > - r = PTR_ERR(minor->kdev);
> > - goto err_index;
> > - }
> > + if (IS_ERR(minor->kdev))
> > + return PTR_ERR(minor->kdev);
> >
> > *drm_minor_get_slot(dev, type) = minor;
> > return 0;
> > -
> > -err_index:
> > - spin_lock_irqsave(_minor_lock, flags);
> > - idr_remove(_minors_idr, minor->index);
> > - spin_unlock_irqrestore(_minor_lock, flags);
> > -err_free:
> > - kfree(minor);
> > - return r;
> > -}
> > -
> > -static void drm_minor_free(struct drm_device *dev, unsigned int type)
> > -{
> > - struct drm_minor **slot, *minor;
> > - unsigned long flags;
> > -
> > - slot = drm_minor_get_slot(dev, type);
> > - minor = *slot;
> > - if (!minor)
> > - return;
> > -
> > - put_device(minor->kdev);
> > -
> > - spin_lock_irqsave(_minor_lock, flags);
> > - idr_remove(_minors_idr, minor->index);
> > - spin_unlock_irqrestore(_minor_lock, flags);
> > -
> > - kfree(minor);
>
> > - *slot = NULL;
>
> In drm_minor_alloc_release() there is no equivalent to this
> NULL assignment.
> Did you consider if there was any real reason for the NULL assignment?
I think they're just prudence, so that you oops if you do a
use-after-free. But nowadays we have KASAN (and slab poisoning, and
lots more stuff), which is massively more powerful at catching these
kinds of bugs. There's a bunch of these "set to NULL" that I've
dropped, since with drmm you easily get access to the higher level
structure that has the pointer to the thing you're cleaning up.
-Daniel
>
> Sam
>
>
> > }
> >
> > static int drm_minor_register(struct drm_device *dev, unsigned int type)
> > @@ -678,16 +664,16 @@ int drm_dev_init(struct drm_device *dev,
> > if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> > ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
> > if (ret)
> > - goto err_minors;
> > +
Re: [PATCH] drm: manage drm_minor cleanup with drmm_
On Tue, Mar 24, 2020 at 09:39:36PM +0100, Daniel Vetter wrote:
> The cleanup here is somewhat tricky, since we can't tell apart the
> allocated minor index from 0. So register a cleanup action first, and
> if the index allocation fails, unregister that cleanup action again to
> avoid bad mistakes.
>
> The kdev for the minor already handles NULL, so no problem there.
>
> Hence add drmm_remove_action() to the drm_managed library.
>
> v2: Make pointer math around void ** consistent with what Laurent
> suggested.
>
> v3: Use drmm_add_action_or_reset and remove drmm_remove_action. Noticed
> because of some questions from Thomas. This also means we need to move
> the drmm_add_action_or_reset helper earlier in the series.
>
> v4: Uh ... fix slightly embarrassing bug CI spotted.
Looks like the one I spotted in my review.
Saw your mail only after posting.
One Q below.
>
> Cc: Thomas Zimmermann
> Cc: Laurent Pinchart
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_drv.c | 69 ---
> drivers/gpu/drm/drm_managed.c | 14 +++
> include/drm/drm_managed.h | 9 -
> 3 files changed, 46 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index a710c53d13a8..50c56ff24c71 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -93,13 +93,25 @@ static struct drm_minor **drm_minor_get_slot(struct
> drm_device *dev,
> }
> }
>
> +static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> +{
> + struct drm_minor *minor = data;
> + unsigned long flags;
> +
> + put_device(minor->kdev);
> +
> + spin_lock_irqsave(_minor_lock, flags);
> + idr_remove(_minors_idr, minor->index);
> + spin_unlock_irqrestore(_minor_lock, flags);
> +}
> +
> static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> {
> struct drm_minor *minor;
> unsigned long flags;
> int r;
>
> - minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> + minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> if (!minor)
> return -ENOMEM;
>
> @@ -117,46 +129,20 @@ static int drm_minor_alloc(struct drm_device *dev,
> unsigned int type)
> idr_preload_end();
>
> if (r < 0)
> - goto err_free;
> + return r;
>
> minor->index = r;
>
> + r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
> + if (r)
> + return r;
> +
> minor->kdev = drm_sysfs_minor_alloc(minor);
> - if (IS_ERR(minor->kdev)) {
> - r = PTR_ERR(minor->kdev);
> - goto err_index;
> - }
> + if (IS_ERR(minor->kdev))
> + return PTR_ERR(minor->kdev);
>
> *drm_minor_get_slot(dev, type) = minor;
> return 0;
> -
> -err_index:
> - spin_lock_irqsave(_minor_lock, flags);
> - idr_remove(_minors_idr, minor->index);
> - spin_unlock_irqrestore(_minor_lock, flags);
> -err_free:
> - kfree(minor);
> - return r;
> -}
> -
> -static void drm_minor_free(struct drm_device *dev, unsigned int type)
> -{
> - struct drm_minor **slot, *minor;
> - unsigned long flags;
> -
> - slot = drm_minor_get_slot(dev, type);
> - minor = *slot;
> - if (!minor)
> - return;
> -
> - put_device(minor->kdev);
> -
> - spin_lock_irqsave(_minor_lock, flags);
> - idr_remove(_minors_idr, minor->index);
> - spin_unlock_irqrestore(_minor_lock, flags);
> -
> - kfree(minor);
> - *slot = NULL;
In drm_minor_alloc_release() there is no equivalent to this
NULL assignment.
Did you consider if there was any real reason for the NULL assignment?
Sam
> }
>
> static int drm_minor_register(struct drm_device *dev, unsigned int type)
> @@ -678,16 +664,16 @@ int drm_dev_init(struct drm_device *dev,
> if (drm_core_check_feature(dev, DRIVER_RENDER)) {
> ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
> if (ret)
> - goto err_minors;
> + goto err;
> }
>
> ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
> if (ret)
> - goto err_minors;
> + goto err;
>
> ret = drm_legacy_create_map_hash(dev);
> if (ret)
> - goto err_minors;
> + goto err;
>
> drm_legacy_ctxbitmap_init(dev);
>
> @@ -695,7 +681,7 @@ int drm_dev_init(struct drm_device *dev,
> ret = drm_gem_init(dev);
> if (ret) {
> DRM_ERROR("Cannot initialize graphics execution manager
> (GEM)\n");
> - goto err_ctxbitmap;
> + goto err;
> }
> }
>
> @@ -708,10 +694,6 @@ int drm_dev_init(struct drm_device *dev,
> err_setunique:
> if (drm_core_check_feature(dev, DRIVER_GEM))
> drm_gem_destroy(dev);
> -err_ctxbitmap:
> -err_minors:
> - drm_minor_free(dev,
[PATCH] drm: manage drm_minor cleanup with drmm_
The cleanup here is somewhat tricky, since we can't tell apart the
allocated minor index from 0. So register a cleanup action first, and
if the index allocation fails, unregister that cleanup action again to
avoid bad mistakes.
The kdev for the minor already handles NULL, so no problem there.
Hence add drmm_remove_action() to the drm_managed library.
v2: Make pointer math around void ** consistent with what Laurent
suggested.
v3: Use drmm_add_action_or_reset and remove drmm_remove_action. Noticed
because of some questions from Thomas. This also means we need to move
the drmm_add_action_or_reset helper earlier in the series.
v4: Uh ... fix slightly embarrassing bug CI spotted.
Cc: Thomas Zimmermann
Cc: Laurent Pinchart
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_drv.c | 69 ---
drivers/gpu/drm/drm_managed.c | 14 +++
include/drm/drm_managed.h | 9 -
3 files changed, 46 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index a710c53d13a8..50c56ff24c71 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -93,13 +93,25 @@ static struct drm_minor **drm_minor_get_slot(struct
drm_device *dev,
}
}
+static void drm_minor_alloc_release(struct drm_device *dev, void *data)
+{
+ struct drm_minor *minor = data;
+ unsigned long flags;
+
+ put_device(minor->kdev);
+
+ spin_lock_irqsave(_minor_lock, flags);
+ idr_remove(_minors_idr, minor->index);
+ spin_unlock_irqrestore(_minor_lock, flags);
+}
+
static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
{
struct drm_minor *minor;
unsigned long flags;
int r;
- minor = kzalloc(sizeof(*minor), GFP_KERNEL);
+ minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
if (!minor)
return -ENOMEM;
@@ -117,46 +129,20 @@ static int drm_minor_alloc(struct drm_device *dev,
unsigned int type)
idr_preload_end();
if (r < 0)
- goto err_free;
+ return r;
minor->index = r;
+ r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
+ if (r)
+ return r;
+
minor->kdev = drm_sysfs_minor_alloc(minor);
- if (IS_ERR(minor->kdev)) {
- r = PTR_ERR(minor->kdev);
- goto err_index;
- }
+ if (IS_ERR(minor->kdev))
+ return PTR_ERR(minor->kdev);
*drm_minor_get_slot(dev, type) = minor;
return 0;
-
-err_index:
- spin_lock_irqsave(_minor_lock, flags);
- idr_remove(_minors_idr, minor->index);
- spin_unlock_irqrestore(_minor_lock, flags);
-err_free:
- kfree(minor);
- return r;
-}
-
-static void drm_minor_free(struct drm_device *dev, unsigned int type)
-{
- struct drm_minor **slot, *minor;
- unsigned long flags;
-
- slot = drm_minor_get_slot(dev, type);
- minor = *slot;
- if (!minor)
- return;
-
- put_device(minor->kdev);
-
- spin_lock_irqsave(_minor_lock, flags);
- idr_remove(_minors_idr, minor->index);
- spin_unlock_irqrestore(_minor_lock, flags);
-
- kfree(minor);
- *slot = NULL;
}
static int drm_minor_register(struct drm_device *dev, unsigned int type)
@@ -678,16 +664,16 @@ int drm_dev_init(struct drm_device *dev,
if (drm_core_check_feature(dev, DRIVER_RENDER)) {
ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
if (ret)
- goto err_minors;
+ goto err;
}
ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
if (ret)
- goto err_minors;
+ goto err;
ret = drm_legacy_create_map_hash(dev);
if (ret)
- goto err_minors;
+ goto err;
drm_legacy_ctxbitmap_init(dev);
@@ -695,7 +681,7 @@ int drm_dev_init(struct drm_device *dev,
ret = drm_gem_init(dev);
if (ret) {
DRM_ERROR("Cannot initialize graphics execution manager
(GEM)\n");
- goto err_ctxbitmap;
+ goto err;
}
}
@@ -708,10 +694,6 @@ int drm_dev_init(struct drm_device *dev,
err_setunique:
if (drm_core_check_feature(dev, DRIVER_GEM))
drm_gem_destroy(dev);
-err_ctxbitmap:
-err_minors:
- drm_minor_free(dev, DRM_MINOR_PRIMARY);
- drm_minor_free(dev, DRM_MINOR_RENDER);
err:
drm_managed_release(dev);
@@ -776,9 +758,6 @@ void drm_dev_fini(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_GEM))
drm_gem_destroy(dev);
-
- drm_minor_free(dev, DRM_MINOR_PRIMARY);
- drm_minor_free(dev, DRM_MINOR_RENDER);
}
EXPORT_SYMBOL(drm_dev_fini);
diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
index
