Give this a nice summary,
drm/syncobj: Avoid kmalloc(GFP_KERNEL) under spinlock
Quoting Chunming Zhou (2018-10-25 11:21:05)
> drivers/gpu/drm/drm_syncobj.c:202:4-14: ERROR: function drm_syncobj_find_signal_pt_for_point called on line 390 inside lock on line 389 but uses GFP_KERNEL
>
> Find functions that refer to GFP_KERNEL but are called with locks held.
>
> Generated by: scripts/coccinelle/locks/call_kern.cocci
>
> v2:
> syncobj->timeline still needs protect.
>
> v3:
> use a global signaled fence instead of re-allocation.
>
Good practice, would be to add Testcase: (and as penance for the bug,
write it ;)
> Signed-off-by: Chunming Zhou <david1.zhou@xxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Christian König <easy2remember.chk@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_drv.c | 2 ++
> drivers/gpu/drm/drm_syncobj.c | 52 +++++++++++++++++++++--------------
> include/drm/drm_syncobj.h | 1 +
> 3 files changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 36e8e9cbec52..0a6f1023d6c3 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -37,6 +37,7 @@
> #include <drm/drm_client.h>
> #include <drm/drm_drv.h>
> #include <drm/drmP.h>
> +#include <drm/drm_syncobj.h>
>
> #include "drm_crtc_internal.h"
> #include "drm_legacy.h"
> @@ -1003,6 +1004,7 @@ static int __init drm_core_init(void)
> if (ret < 0)
> goto error;
>
> + drm_syncobj_stub_fence_init();
> drm_core_init_complete = true;
>
> DRM_DEBUG("Initialized\n");
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index b7eaa603f368..6b3f5a06e4d3 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -80,6 +80,27 @@ struct drm_syncobj_signal_pt {
> struct list_head list;
> };
>
> +static struct drm_syncobj_stub_fence stub_signaled_fence;
> +static void global_stub_fence_release(struct dma_fence *fence)
> +{
> + /* it is impossible to come here */
> + BUG();
BUG() is overkill, kasan will complain for us anyway, so we can just use
the default release function.
> +}
> +static const struct dma_fence_ops global_stub_fence_ops = {
> + .get_driver_name = drm_syncobj_stub_fence_get_name,
> + .get_timeline_name = drm_syncobj_stub_fence_get_name,
> + .release = global_stub_fence_release,
> +};
> +
> +void drm_syncobj_stub_fence_init(void)
I think we can avoid having this exposed by:
static DECLARE_SPINLOCK(signaled_fence_lock);
static dma_fence signaled_fence;
static struct dma_fence *signaled_fence_get(void)
{
spin_lock(&signaled_fenced_lock);
if (!signaled_fence.ops) {
dma_fence_init(&signaled_fence,
&signaled_fence_ops,
&signaled_fence_lock,
0, 0);
dma_fence_signal_locked(&signaled_fence.base);
}
spin_unlock(&signaled_fenced_lock);
return dma_fence_get(&signaled_fence);
}
> /**
> * drm_syncobj_find - lookup and reference a sync object.
> * @file_private: drm file private pointer
> @@ -111,24 +132,14 @@ static struct dma_fence
> uint64_t point)
> {
> struct drm_syncobj_signal_pt *signal_pt;
> + struct dma_fence *f = NULL;
>
> + spin_lock(&syncobj->pt_lock);
> if ((syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) &&
> (point <= syncobj->timeline)) {
> - struct drm_syncobj_stub_fence *fence =
> - kzalloc(sizeof(struct drm_syncobj_stub_fence),
> - GFP_KERNEL);
> -
> - if (!fence)
> - return NULL;
> - spin_lock_init(&fence->lock);
> - dma_fence_init(&fence->base,
> - &drm_syncobj_stub_fence_ops,
> - &fence->lock,
> - syncobj->timeline_context,
> - point);
> -
> - dma_fence_signal(&fence->base);
> - return &fence->base;
> + dma_fence_get(&stub_signaled_fence.base);
> + spin_unlock(&syncobj->pt_lock);
> + return &stub_signaled_fence.base;
f = signaled_fence_get();
goto unlock;
> }
>
> list_for_each_entry(signal_pt, &syncobj->signal_pt_list, list) {
> @@ -137,9 +148,12 @@ static struct dma_fence
> if ((syncobj->type == DRM_SYNCOBJ_TYPE_BINARY) &&
> (point != signal_pt->value))
> continue;
> - return dma_fence_get(&signal_pt->fence_array->base);
> + f = dma_fence_get(&signal_pt->fence_array->base);
> + break;
> }
> - return NULL;
unlock:
> + spin_unlock(&syncobj->pt_lock);
> +
> + return f;
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
