| Message ID | 20230827175449.1766701-2-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 Sun, 27 Aug 2023 20:54:27 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > Freeing drm-shmem GEM right after creating it using > drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf > and then dma-buf frees this SGT second time. > > The v3d_prime_import_sg_table() is example of a error code path where > dma-buf's SGT is freed by drm-shmem and then it's freed second time by > dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). > > Add drm-shmem GEM flag telling that this is imported SGT shall not be > treated as own SGT, fixing the use-after-free bug. > > Cc: stable@vger.kernel.org > Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- > include/drm/drm_gem_shmem_helper.h | 7 +++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index a783d2245599..78d9cf2355a5 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > > if (obj->import_attach) { > drm_prime_gem_destroy(obj, shmem->sgt); > - } else { > + } else if (!shmem->imported_sgt) { > dma_resv_lock(shmem->base.resv, NULL); > > drm_WARN_ON(obj->dev, shmem->vmap_use_count); > @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, > return ERR_CAST(shmem); > > shmem->sgt = sgt; > + shmem->imported_sgt = true; I feel like adding more fields that can be used to do the is_imported() check is going to be even more confusing. Can we instead have /* drm_gem_shmem_prime_import_sg_table() can be called from a * driver specific ->import_sg_table() implementations that * have extra failable initialization steps. Assign * drm_gem_object::import_attach here (even though it's * assigned in drm_gem_prime_import_dev()), so we don't end up * with driver error paths calling drm_gem_shmem_free() with an * imported sg_table assigned to drm_gem_shmem_object::sgt and * drm_gem_object::import_attach left uninitialized. */ shmem->base.import_attach = attach; here? > > drm_dbg_prime(dev, "size = %zu\n", size); > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index bf0c31aa8fbe..ec70a98a8fe1 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -73,6 +73,13 @@ struct drm_gem_shmem_object { > */ > unsigned int vmap_use_count; > > + /** > + * @imported_sgt: > + * > + * True if SG table belongs to imported dma-buf. > + */ > + bool imported_sgt : 1; > + > /** > * @pages_mark_dirty_on_put: > *
On 8/28/23 14:16, Boris Brezillon wrote: > On Sun, 27 Aug 2023 20:54:27 +0300 > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > >> Freeing drm-shmem GEM right after creating it using >> drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf >> and then dma-buf frees this SGT second time. >> >> The v3d_prime_import_sg_table() is example of a error code path where >> dma-buf's SGT is freed by drm-shmem and then it's freed second time by >> dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). >> >> Add drm-shmem GEM flag telling that this is imported SGT shall not be >> treated as own SGT, fixing the use-after-free bug. >> >> Cc: stable@vger.kernel.org >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- >> include/drm/drm_gem_shmem_helper.h | 7 +++++++ >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index a783d2245599..78d9cf2355a5 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) >> >> if (obj->import_attach) { >> drm_prime_gem_destroy(obj, shmem->sgt); >> - } else { >> + } else if (!shmem->imported_sgt) { >> dma_resv_lock(shmem->base.resv, NULL); >> >> drm_WARN_ON(obj->dev, shmem->vmap_use_count); >> @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, >> return ERR_CAST(shmem); >> >> shmem->sgt = sgt; >> + shmem->imported_sgt = true; > > > I feel like adding more fields that can be used to do the is_imported() > check is going to be even more confusing. Can we instead have > > /* drm_gem_shmem_prime_import_sg_table() can be called from a > * driver specific ->import_sg_table() implementations that > * have extra failable initialization steps. Assign > * drm_gem_object::import_attach here (even though it's > * assigned in drm_gem_prime_import_dev()), so we don't end up > * with driver error paths calling drm_gem_shmem_free() with an > * imported sg_table assigned to drm_gem_shmem_object::sgt and > * drm_gem_object::import_attach left uninitialized. > */ > shmem->base.import_attach = attach; > > here? AFAICT, this is not going to work because obj->import_attach will be released by drm_prime core by the time drm_gem_shmem_free() is invoked and drm_gem_shmem_free() uses obj->import_attach as well. I'll keep this patch around unless there will be other suggestions. To me the flag is good enough, I'll add a clarifying comment to the code in v16.
On Sat, 2 Sep 2023 21:15:39 +0300 Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 8/28/23 14:16, Boris Brezillon wrote: > > On Sun, 27 Aug 2023 20:54:27 +0300 > > Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > > > >> Freeing drm-shmem GEM right after creating it using > >> drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf > >> and then dma-buf frees this SGT second time. > >> > >> The v3d_prime_import_sg_table() is example of a error code path where > >> dma-buf's SGT is freed by drm-shmem and then it's freed second time by > >> dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). > >> > >> Add drm-shmem GEM flag telling that this is imported SGT shall not be > >> treated as own SGT, fixing the use-after-free bug. > >> > >> Cc: stable@vger.kernel.org > >> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") > >> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > >> --- > >> drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- > >> include/drm/drm_gem_shmem_helper.h | 7 +++++++ > >> 2 files changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> index a783d2245599..78d9cf2355a5 100644 > >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > >> @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) > >> > >> if (obj->import_attach) { > >> drm_prime_gem_destroy(obj, shmem->sgt); > >> - } else { > >> + } else if (!shmem->imported_sgt) { > >> dma_resv_lock(shmem->base.resv, NULL); > >> > >> drm_WARN_ON(obj->dev, shmem->vmap_use_count); > >> @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, > >> return ERR_CAST(shmem); > >> > >> shmem->sgt = sgt; > >> + shmem->imported_sgt = true; > > > > > > I feel like adding more fields that can be used to do the is_imported() > > check is going to be even more confusing. Can we instead have > > > > /* drm_gem_shmem_prime_import_sg_table() can be called from a > > * driver specific ->import_sg_table() implementations that > > * have extra failable initialization steps. Assign > > * drm_gem_object::import_attach here (even though it's > > * assigned in drm_gem_prime_import_dev()), so we don't end up > > * with driver error paths calling drm_gem_shmem_free() with an > > * imported sg_table assigned to drm_gem_shmem_object::sgt and > > * drm_gem_object::import_attach left uninitialized. > > */ > > shmem->base.import_attach = attach; > > > > here? > > AFAICT, this is not going to work because obj->import_attach will be > released by drm_prime core by the time drm_gem_shmem_free() is invoked > and drm_gem_shmem_free() uses obj->import_attach as well. How can this happen? If something wrong happens in the driver-specific ->gem_prime_import_sg_table() implementation, drm_gem_shmem_free() will be called before ->gem_prime_import_sg_table() returns, and the attachment will only be released after that [1]. > I'll keep this > patch around unless there will be other suggestions. To me the flag is > good enough, I'll add a clarifying comment to the code in v16. I really think this is a bad idea, for the same reasons I gave in my reply to patch 2 (adding fields that need to be maintained when the state can be inferred from other fields is error prone). [1]https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_prime.c#L958
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index a783d2245599..78d9cf2355a5 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -141,7 +141,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem) if (obj->import_attach) { drm_prime_gem_destroy(obj, shmem->sgt); - } else { + } else if (!shmem->imported_sgt) { dma_resv_lock(shmem->base.resv, NULL); drm_WARN_ON(obj->dev, shmem->vmap_use_count); @@ -758,6 +758,7 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, return ERR_CAST(shmem); shmem->sgt = sgt; + shmem->imported_sgt = true; drm_dbg_prime(dev, "size = %zu\n", size); diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index bf0c31aa8fbe..ec70a98a8fe1 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -73,6 +73,13 @@ struct drm_gem_shmem_object { */ unsigned int vmap_use_count; + /** + * @imported_sgt: + * + * True if SG table belongs to imported dma-buf. + */ + bool imported_sgt : 1; + /** * @pages_mark_dirty_on_put: *
Freeing drm-shmem GEM right after creating it using drm_gem_shmem_prime_import_sg_table() frees SGT of the imported dma-buf and then dma-buf frees this SGT second time. The v3d_prime_import_sg_table() is example of a error code path where dma-buf's SGT is freed by drm-shmem and then it's freed second time by dma_buf_unmap_attachment() in drm_gem_prime_import_dev(). Add drm-shmem GEM flag telling that this is imported SGT shall not be treated as own SGT, fixing the use-after-free bug. Cc: stable@vger.kernel.org Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects") Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 3 ++- include/drm/drm_gem_shmem_helper.h | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletion(-)