| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z5fUu6XwUrZESb5H@intel.com>
Date: Mon, 27 Jan 2025 20:47:23 +0200
From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
To: Brian Geffon <bgeffon@...gle.com>
Cc: intel-gfx@...ts.freedesktop.org, chris.p.wilson@...el.com,
jani.saarinen@...el.com, tomasz.mistat@...el.com,
vidya.srinivas@...el.com, jani.nikula@...ux.intel.com,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
stable@...r.kernel.org, Tomasz Figa <tfiga@...gle.com>
Subject: Re: [PATCH v2] drm/i915: Fix page cleanup on DMA remap failure
On Thu, Jan 16, 2025 at 10:53:40AM -0500, Brian Geffon wrote:
> When converting to folios the cleanup path of shmem_get_pages() was
> missed. When a DMA remap fails and the max segment size is greater than
> PAGE_SIZE it will attempt to retry the remap with a PAGE_SIZEd segment
> size. The cleanup code isn't properly using the folio apis and as a
> result isn't handling compound pages correctly.
>
> v1 -> v2:
> (Ville) Fixed locations where we were not clearing mapping unevictable.
>
> Cc: stable@...r.kernel.org
> Cc: Ville Syrjala <ville.syrjala@...ux.intel.com>
> Cc: Vidya Srinivas <vidya.srinivas@...el.com>
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13487
> Link: https://lore.kernel.org/lkml/20250116135636.410164-1-bgeffon@google.com/
> Fixes: 0b62af28f249 ("i915: convert shmem_sg_free_table() to use a folio_batch")
> Signed-off-by: Brian Geffon <bgeffon@...gle.com>
> Suggested-by: Tomasz Figa <tfiga@...gle.com>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 +--
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 23 +++++++++-------------
> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++++---
> 3 files changed, 14 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 3dc61cbd2e11..0f122a12d4a5 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -843,8 +843,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
> size_t size, struct intel_memory_region *mr,
> struct address_space *mapping,
> unsigned int max_segment);
> -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
> - bool dirty, bool backup);
> +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup);
> void __shmem_writeback(size_t size, struct address_space *mapping);
>
> #ifdef CONFIG_MMU_NOTIFIER
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index fe69f2c8527d..b320d9dfd6d3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -29,16 +29,13 @@ static void check_release_folio_batch(struct folio_batch *fbatch)
> cond_resched();
> }
>
> -void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
> - bool dirty, bool backup)
> +void shmem_sg_free_table(struct sg_table *st, bool dirty, bool backup)
This still makes the alloc vs. free completely asymmetric.
This is not what we want because it just makes it very easy
to make it mistake in the caller.
I think the correct fix is to simply call the current
shmem_sg_free_table() from the now broken failure path.
mapping_{set,clear}_unevictable() just seems to be some
bit operation so AFAICS the slight ping-pong should be
inconsequential.
> {
> struct sgt_iter sgt_iter;
> struct folio_batch fbatch;
> struct folio *last = NULL;
> struct page *page;
>
> - mapping_clear_unevictable(mapping);
> -
> folio_batch_init(&fbatch);
> for_each_sgt_page(page, sgt_iter, st) {
> struct folio *folio = page_folio(page);
> @@ -180,10 +177,10 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
> return 0;
> err_sg:
> sg_mark_end(sg);
> + mapping_clear_unevictable(mapping);
> if (sg != st->sgl) {
> - shmem_sg_free_table(st, mapping, false, false);
> + shmem_sg_free_table(st, false, false);
> } else {
> - mapping_clear_unevictable(mapping);
> sg_free_table(st);
> }
>
> @@ -209,8 +206,6 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
> struct address_space *mapping = obj->base.filp->f_mapping;
> unsigned int max_segment = i915_sg_segment_size(i915->drm.dev);
> struct sg_table *st;
> - struct sgt_iter sgt_iter;
> - struct page *page;
> int ret;
>
> /*
> @@ -239,9 +234,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
> * for PAGE_SIZE chunks instead may be helpful.
> */
> if (max_segment > PAGE_SIZE) {
> - for_each_sgt_page(page, sgt_iter, st)
> - put_page(page);
> - sg_free_table(st);
> + /* Leave the mapping unevictable while we retry */
> + shmem_sg_free_table(st, false, false);
> kfree(st);
>
> max_segment = PAGE_SIZE;
> @@ -265,7 +259,8 @@ static int shmem_get_pages(struct drm_i915_gem_object *obj)
> return 0;
>
> err_pages:
> - shmem_sg_free_table(st, mapping, false, false);
> + mapping_clear_unevictable(mapping);
> + shmem_sg_free_table(st, false, false);
> /*
> * shmemfs first checks if there is enough memory to allocate the page
> * and reports ENOSPC should there be insufficient, along with the usual
> @@ -402,8 +397,8 @@ void i915_gem_object_put_pages_shmem(struct drm_i915_gem_object *obj, struct sg_
> if (i915_gem_object_needs_bit17_swizzle(obj))
> i915_gem_object_save_bit_17_swizzle(obj, pages);
>
> - shmem_sg_free_table(pages, file_inode(obj->base.filp)->i_mapping,
> - obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED);
> + mapping_clear_unevictable(file_inode(obj->base.filp)->i_mapping);
> + shmem_sg_free_table(pages, obj->mm.dirty, obj->mm.madv == I915_MADV_WILLNEED);
> kfree(pages);
> obj->mm.dirty = false;
> }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index 10d8673641f7..37f51a04b838 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -232,7 +232,8 @@ static int i915_ttm_tt_shmem_populate(struct ttm_device *bdev,
> return 0;
>
> err_free_st:
> - shmem_sg_free_table(st, filp->f_mapping, false, false);
> + mapping_clear_unevictable(filp->f_mapping);
> + shmem_sg_free_table(st, false, false);
>
> return err;
> }
> @@ -243,8 +244,8 @@ static void i915_ttm_tt_shmem_unpopulate(struct ttm_tt *ttm)
> bool backup = ttm->page_flags & TTM_TT_FLAG_SWAPPED;
> struct sg_table *st = &i915_tt->cached_rsgt.table;
>
> - shmem_sg_free_table(st, file_inode(i915_tt->filp)->i_mapping,
> - backup, backup);
> + mapping_clear_unevictable(file_inode(i915_tt->filp)->i_mapping);
> + shmem_sg_free_table(st, backup, backup);
> }
>
> static void i915_ttm_tt_release(struct kref *ref)
> --
> 2.48.0.rc2.279.g1de40edade-goog
--
Ville Syrjälä
Intel
Powered by blists - more mailing lists