| Message ID | 20231029230205.93277-23-dmitry.osipenko@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers | expand |
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);
On 11/13/23 12:54, Boris Brezillon wrote: > 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. This patch is a minor improvement, it doesn't address any specific issue. This should be a common pattern in kernel. If you're giving a warning and know about the inevitable catastrophe, then avoid it if you can. Actually, there are other similar cases in drm-shmem that can be improved.
On Thu, 23 Nov 2023 01:30:24 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 11/13/23 12:54, Boris Brezillon wrote: > > 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. > > This patch is a minor improvement, it doesn't address any specific > issue. This should be a common pattern in kernel. If you're giving a > warning and know about the inevitable catastrophe, then avoid it if you can. Sure, I'm just asking that we add a comment to explain why we leak memory here. Is that too much to ask?
On 11/23/23 12:08, Boris Brezillon wrote: > On Thu, 23 Nov 2023 01:30:24 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> On 11/13/23 12:54, Boris Brezillon wrote: >>> 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. >> >> This patch is a minor improvement, it doesn't address any specific >> issue. This should be a common pattern in kernel. If you're giving a >> warning and know about the inevitable catastrophe, then avoid it if you can. > > Sure, I'm just asking that we add a comment to explain why we leak > memory here. Is that too much to ask? Will add the comment. The reason why I added this patch was unrelated to the sgt, that's what I'm talking about.
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; drm_gem_object_release(obj); kfree(shmem);
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(-)