lkml.org 
[lkml]   [2023]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v18 22/26] drm/shmem-helper: Don't free refcounted GEM
    On Mon, 30 Oct 2023 02:02:01 +0300
    Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

    > Don't free refcounted shmem object to prevent use-after-free bug that
    > is worse than a memory leak.
    >
    > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
    > ---
    > drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++---
    > 1 file changed, 4 insertions(+), 3 deletions(-)
    >
    > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
    > index 6dd087f19ea3..4253c367dc07 100644
    > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
    > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
    > @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
    > if (obj->import_attach)
    > drm_prime_gem_destroy(obj, shmem->sgt);
    >
    > - drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
    > - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count));
    > - drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count));
    > + if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) ||
    > + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) ||
    > + drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)))
    > + return;

    I guess you're worried about ->sgt being referenced by the driver after
    the GEM is destroyed. If we assume drivers don't cache the sgt and
    always call get_pages_sgt() when they need it that shouldn't be an
    issue. What we really don't want to release is the pages themselves,
    but the GPU MMU might still have active mappings pointing to these
    pages.

    In any case, I'm not against leaking the GEM object when any of these
    counters are not zero, but can we at least have a comment in the
    code explaining why we're doing that, so people don't have to go look
    at the git history to figure it out.

    >
    > drm_gem_object_release(obj);
    > kfree(shmem);

    \
     
     \ /
      Last update: 2023-11-20 13:57    [W:8.548 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site