On Wed, 6 Jul 2022 19:33:22 +0300
Gwan-gyeong Mun <gwan-gyeong....@intel.com> wrote:
> On 7/5/22 5:35 PM, Mauro Carvalho Chehab wrote:
> > On Tue, 5 Jul 2022 15:24:50 +0300
> > Gwan-gyeong Mun <gwan-gyeong....@intel.com> wrote:
> >
> >> From: Chris Wilson <ch...@chris-wilson.co.uk>
> >>
> >> We need to check that we avoid integer overflows when looking up a page,
> >> and so fix all the instances where we have mistakenly used a plain
> >> integer instead of a more suitable long. Be pedantic and add integer
> >> typechecking to the lookup so that we can be sure that we are safe.
> >> And it also uses pgoff_t as our page lookups must remain compatible with
> >> the page cache, pgoff_t is currently exactly unsigned long.
> >>
> >> v2: Move added i915_utils's macro into drm_util header (Jani N)
> >>
> >> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> >> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong....@intel.com>
> >> Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
> >> Cc: Matthew Auld <matthew.a...@intel.com>
> >> Cc: Thomas Hellström <thomas.hellst...@linux.intel.com>
> >> Reviewed-by: Nirmoy Das <nirmoy....@intel.com>
> >> ---
> >> drivers/gpu/drm/i915/gem/i915_gem_object.c | 7 +-
> >> drivers/gpu/drm/i915/gem/i915_gem_object.h | 67 ++++++++++++++-----
> >> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 25 ++++---
> >> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
> >> .../drm/i915/gem/selftests/i915_gem_context.c | 12 ++--
> >> .../drm/i915/gem/selftests/i915_gem_mman.c | 8 +--
> >> .../drm/i915/gem/selftests/i915_gem_object.c | 8 +--
> >> drivers/gpu/drm/i915/i915_gem.c | 18 +++--
> >> drivers/gpu/drm/i915/i915_vma.c | 8 +--
> >> 9 files changed, 100 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> >> b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> >> index ccec4055fde3..90996fe8ad45 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> >> @@ -421,10 +421,11 @@ void __i915_gem_object_invalidate_frontbuffer(struct
> >> drm_i915_gem_object *obj,
> >> static void
> >> i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64
> >> offset, void *dst, int size)
> >> {
> >> + pgoff_t idx = offset >> PAGE_SHIFT;
> >> void *src_map;
> >> void *src_ptr;
> >>
> >> - src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >>
> >> PAGE_SHIFT));
> >> + src_map = kmap_atomic(i915_gem_object_get_page(obj, idx));
> >>
> >> src_ptr = src_map + offset_in_page(offset);
> >> if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> >> @@ -437,9 +438,10 @@ i915_gem_object_read_from_page_kmap(struct
> >> drm_i915_gem_object *obj, u64 offset,
> >> static void
> >> i915_gem_object_read_from_page_iomap(struct drm_i915_gem_object *obj,
> >> u64 offset, void *dst, int size)
> >> {
> >> + pgoff_t idx = offset >> PAGE_SHIFT;
> >> + dma_addr_t dma = i915_gem_object_get_dma_address(obj, idx);
> >> void __iomem *src_map;
> >> void __iomem *src_ptr;
> >> - dma_addr_t dma = i915_gem_object_get_dma_address(obj, offset >>
> >> PAGE_SHIFT);
> >>
> >> src_map = io_mapping_map_wc(&obj->mm.region->iomap,
> >> dma - obj->mm.region->region.start,
> >> @@ -468,6 +470,7 @@ i915_gem_object_read_from_page_iomap(struct
> >> drm_i915_gem_object *obj, u64 offset
> >> */
> >> int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64
> >> offset, void *dst, int size)
> >> {
> >> + GEM_BUG_ON(overflows_type(offset >> PAGE_SHIFT, pgoff_t));
> >> GEM_BUG_ON(offset >= obj->base.size);
> >> GEM_BUG_ON(offset_in_page(offset) > PAGE_SIZE - size);
> >> GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >> index 6f0a3ce35567..a60c6f4517d5 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> >> @@ -27,8 +27,10 @@ enum intel_region_id;
> >> * spot such a local variable, please consider fixing!
> >> *
> >> * Aside from our own locals (for which we have no excuse!):
> >> - * - sg_table embeds unsigned int for num_pages
> >> - * - get_user_pages*() mixed ints with longs
> >> + * - sg_table embeds unsigned int for nents
> >> + *
> >> + * We can check for invalidly typed locals with typecheck(), see for
> >> example
> >> + * i915_gem_object_get_sg().
> >> */
> >> #define GEM_CHECK_SIZE_OVERFLOW(sz) \
> >> GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX)
> >> @@ -366,41 +368,70 @@ int i915_gem_object_set_tiling(struct
> >> drm_i915_gem_object *obj,
> >> struct scatterlist *
> >> __i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
> >> struct i915_gem_object_page_iter *iter,
> >> - unsigned int n,
> >> - unsigned int *offset, bool dma);
> >> + pgoff_t n,
> >> + unsigned int *offset);
> >> +
> >> +#define __i915_gem_object_get_sg(obj, it, n, offset) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (__i915_gem_object_get_sg)(obj, it, n, offset); \
> >> +})
> >>
> >> static inline struct scatterlist *
> >> -i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
> >> - unsigned int n,
> >> +i915_gem_object_get_sg(struct drm_i915_gem_object *obj, pgoff_t n,
> >> unsigned int *offset)
> >> {
> >> - return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset,
> >> false);
> >> + return __i915_gem_object_get_sg(obj, &obj->mm.get_page, n, offset);
> >> }
> >>
> >> +#define i915_gem_object_get_sg(obj, n, offset) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (i915_gem_object_get_sg)(obj, n, offset); \
> >> +})
> >> +
> >> static inline struct scatterlist *
> >> -i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj,
> >> - unsigned int n,
> >> +i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj, pgoff_t n,
> >> unsigned int *offset)
> >> {
> >> - return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset,
> >> true);
> >> + return __i915_gem_object_get_sg(obj, &obj->mm.get_dma_page, n, offset);
> >> }
> >>
> >> +#define i915_gem_object_get_sg_dma(obj, n, offset) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (i915_gem_object_get_sg_dma)(obj, n, offset); \
> >> +})
> >> +
> >> struct page *
> >> -i915_gem_object_get_page(struct drm_i915_gem_object *obj,
> >> - unsigned int n);
> >> +i915_gem_object_get_page(struct drm_i915_gem_object *obj, pgoff_t n);
> >> +
> >> +#define i915_gem_object_get_page(obj, n) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (i915_gem_object_get_page)(obj, n); \
> >> +})
> >>
> >> struct page *
> >> -i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj,
> >> - unsigned int n);
> >> +i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, pgoff_t
> >> n);
> >> +
> >> +#define i915_gem_object_get_dirty_page(obj, n) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (i915_gem_object_get_dirty_page)(obj, n); \
> >> +})
> >>
> >> dma_addr_t
> >> -i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj,
> >> - unsigned long n,
> >> +i915_gem_object_get_dma_address_len(struct drm_i915_gem_object *obj,
> >> pgoff_t n,
> >> unsigned int *len);
> >>
> >> +#define i915_gem_object_get_dma_address_len(obj, n, len) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (i915_gem_object_get_dma_address_len)(obj, n, len); \
> >> +})
> >> +
> >> dma_addr_t
> >> -i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj,
> >> - unsigned long n);
> >> +i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, pgoff_t
> >> n);
> >> +
> >> +#define i915_gem_object_get_dma_address(obj, n) ({ \
> >> + exactly_pgoff_t(n); \
> >> + (i915_gem_object_get_dma_address)(obj, n); \
> >> +})
> >>
> >> void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
> >> struct sg_table *pages,
> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> >> b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> >> index 97c820eee115..1d1edcb3514b 100644
> >> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> >> @@ -503,14 +503,16 @@ void __i915_gem_object_release_map(struct
> >> drm_i915_gem_object *obj)
> >> }
> >>
> >> struct scatterlist *
> >> -__i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
> >> +(__i915_gem_object_get_sg)(struct drm_i915_gem_object *obj,
> >> struct i915_gem_object_page_iter *iter,
> >> - unsigned int n,
> >> - unsigned int *offset,
> >> - bool dma)
> >> + pgoff_t n,
> >> + unsigned int *offset)
> >> +
> >
> > Nitpick: no need to place the function name in parenthesis.
> >
> >> {
> >> - struct scatterlist *sg;
> >> + const bool dma = iter == &obj->mm.get_dma_page ||
> >> + iter == &obj->ttm.get_io_page;
> >> unsigned int idx, count;
> >> + struct scatterlist *sg;
> >>
> >> might_sleep();
> >> GEM_BUG_ON(n >= obj->base.size >> PAGE_SHIFT);
> >> @@ -618,7 +620,7 @@ __i915_gem_object_get_sg(struct drm_i915_gem_object
> >> *obj,
> >> }
> >>
> >> struct page *
> >> -i915_gem_object_get_page(struct drm_i915_gem_object *obj, unsigned int n)
> >> +(i915_gem_object_get_page)(struct drm_i915_gem_object *obj, pgoff_t n)
> >
> > Same as above: why are you placing parenthesis at the function name here?
> > Just use:
> >
> > struct page *
> > i915_gem_object_get_page(struct drm_i915_gem_object *obj, pgoff_t n)
> >
> In this case, the macro and function have the same name. If parenthesis
> is not used, the following compile error occurs as the macro is applied
> to the c code.
>
> ./drivers/gpu/drm/i915/gem/i915_gem_object.h:356:55: error: expected
> identifier or ‘(’ before ‘{’ token
> 356 | #define __i915_gem_object_get_sg(obj, it, n, offset) ({ \
> | ^
> drivers/gpu/drm/i915/gem/i915_gem_pages.c:506:1: note: in expansion of
> macro ‘__i915_gem_object_get_sg’
> 506 | __i915_gem_object_get_sg(struct drm_i915_gem_object *obj,
> | ^~~~~~~~~~~~~~~~~~~~~~~~
>
> And all of the parts you leave comments below are cases where the names
> of macros and functions are the same.
Don't use the same macro name on a function. This is very confusing
and will prevent ever adding documentation for it, as, for kernel-doc,
macros and functions are handled at the same namespace. So, no
duplication is allowed.
Probably the best here would be to replace the macros by inlined
functions.
Regards,
Mauro
