| Message ID | 20230530-i915-gt-cache_level-wincompatible-function-pointer-types-strict-v1-1-54501d598229@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gt: Fix recent kCFI violations | expand |
Hi Nathan, On Tue, May 30, 2023 at 11:24:38AM -0700, Nathan Chancellor wrote: > When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a > CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode() > via an indirect call: > > [ 5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc) > > With kCFI, indirect calls are validated against their expected type > versus actual type and failures occur when the two types do not match. > > clang's -Wincompatible-function-pointer-types-strict can catch this at > compile time but it is not enabled for the kernel yet: > > drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = iris_pte_encode; > ^ ~~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = hsw_pte_encode; > ^ ~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = byt_pte_encode; > ^ ~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = ivb_pte_encode; > ^ ~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = snb_pte_encode; > ^ ~~~~~~~~~~~~~~ > 5 errors generated. > > In this case, the pre-gen8 pte_encode functions have a second parameter > type of 'enum i915_cache_level' whereas the function pointer prototype > in 'struct i915_address_space' expects a second parameter type of > 'unsigned int'. > > Update the second parameter of the callbacks and the comment above them > noting that these statements are still valid, which matches other > functions and files, to clear up the kCFI failures at run time. > > Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") > Signed-off-by: Nathan Chancellor <nathan@kernel.org> That's correct! Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
> Subject: [PATCH 1/2] drm/i915/gt: Fix second parameter type of pre-gen8 pte_encode callbacks > > When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode() via an indirect call: > > [ 5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc) > > With kCFI, indirect calls are validated against their expected type versus actual type and failures occur when the two types do not match. > > clang's -Wincompatible-function-pointer-types-strict can catch this at compile time but it is not enabled for the kernel yet: > > drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = iris_pte_encode; > ^ ~~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = hsw_pte_encode; > ^ ~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = byt_pte_encode; > ^ ~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = ivb_pte_encode; > ^ ~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, > enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] > ggtt->vm.pte_encode = snb_pte_encode; > ^ ~~~~~~~~~~~~~~ > 5 errors generated. > > In this case, the pre-gen8 pte_encode functions have a second parameter type of 'enum i915_cache_level' whereas the function pointer prototype in 'struct i915_address_space' expects a second parameter type of 'unsigned int'. > > Update the second parameter of the callbacks and the comment above them noting that these statements are still valid, which matches other functions and files, to clear up the kCFI failures at run time. > > Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") > Signed-off-by: Nathan Chancellor <nathan@kernel.org> Reviewed-by: Fei Yang <fei.yang@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c > index 2a7942fac798..122197737ef2 100644 > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c > @@ -1015,16 +1015,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt) > > /* > * For pre-gen8 platforms pat_index is the same as enum i915_cache_level, > - * so these PTE encode functions are left with using cache_level. > + * so the switch-case statements in these PTE encode functions are still valid. > * See translation table LEGACY_CACHELEVEL. > */ > static u64 snb_pte_encode(dma_addr_t addr, > - enum i915_cache_level level, > + unsigned int pat_index, > u32 flags) > { > gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; > > - switch (level) { > + switch (pat_index) { > case I915_CACHE_L3_LLC: > case I915_CACHE_LLC: > pte |= GEN6_PTE_CACHE_LLC; > @@ -1033,19 +1033,19 @@ static u64 snb_pte_encode(dma_addr_t addr, > pte |= GEN6_PTE_UNCACHED; > break; > default: > - MISSING_CASE(level); > + MISSING_CASE(pat_index); > } > > return pte; > } > > static u64 ivb_pte_encode(dma_addr_t addr, > - enum i915_cache_level level, > + unsigned int pat_index, > u32 flags) > { > gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; > > - switch (level) { > + switch (pat_index) { > case I915_CACHE_L3_LLC: > pte |= GEN7_PTE_CACHE_L3_LLC; > break; > @@ -1056,14 +1056,14 @@ static u64 ivb_pte_encode(dma_addr_t addr, > pte |= GEN6_PTE_UNCACHED; > break; > default: > - MISSING_CASE(level); > + MISSING_CASE(pat_index); > } > > return pte; > } > > static u64 byt_pte_encode(dma_addr_t addr, > - enum i915_cache_level level, > + unsigned int pat_index, > u32 flags) > { > gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; @@ -1071,31 +1071,31 @@ static u64 byt_pte_encode(dma_addr_t addr, > if (!(flags & PTE_READ_ONLY)) > pte |= BYT_PTE_WRITEABLE; > > - if (level != I915_CACHE_NONE) > + if (pat_index != I915_CACHE_NONE) > pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES; > > return pte; > } > > static u64 hsw_pte_encode(dma_addr_t addr, > - enum i915_cache_level level, > + unsigned int pat_index, > u32 flags) > { > gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; > > - if (level != I915_CACHE_NONE) > + if (pat_index != I915_CACHE_NONE) > pte |= HSW_WB_LLC_AGE3; > > return pte; > } > > static u64 iris_pte_encode(dma_addr_t addr, > - enum i915_cache_level level, > + unsigned int pat_index, > u32 flags) > { > gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; > > - switch (level) { > + switch (pat_index) { > case I915_CACHE_NONE: > break; > case I915_CACHE_WT: > > -- > 2.40.1
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 2a7942fac798..122197737ef2 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -1015,16 +1015,16 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt) /* * For pre-gen8 platforms pat_index is the same as enum i915_cache_level, - * so these PTE encode functions are left with using cache_level. + * so the switch-case statements in these PTE encode functions are still valid. * See translation table LEGACY_CACHELEVEL. */ static u64 snb_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; - switch (level) { + switch (pat_index) { case I915_CACHE_L3_LLC: case I915_CACHE_LLC: pte |= GEN6_PTE_CACHE_LLC; @@ -1033,19 +1033,19 @@ static u64 snb_pte_encode(dma_addr_t addr, pte |= GEN6_PTE_UNCACHED; break; default: - MISSING_CASE(level); + MISSING_CASE(pat_index); } return pte; } static u64 ivb_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; - switch (level) { + switch (pat_index) { case I915_CACHE_L3_LLC: pte |= GEN7_PTE_CACHE_L3_LLC; break; @@ -1056,14 +1056,14 @@ static u64 ivb_pte_encode(dma_addr_t addr, pte |= GEN6_PTE_UNCACHED; break; default: - MISSING_CASE(level); + MISSING_CASE(pat_index); } return pte; } static u64 byt_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = GEN6_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; @@ -1071,31 +1071,31 @@ static u64 byt_pte_encode(dma_addr_t addr, if (!(flags & PTE_READ_ONLY)) pte |= BYT_PTE_WRITEABLE; - if (level != I915_CACHE_NONE) + if (pat_index != I915_CACHE_NONE) pte |= BYT_PTE_SNOOPED_BY_CPU_CACHES; return pte; } static u64 hsw_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; - if (level != I915_CACHE_NONE) + if (pat_index != I915_CACHE_NONE) pte |= HSW_WB_LLC_AGE3; return pte; } static u64 iris_pte_encode(dma_addr_t addr, - enum i915_cache_level level, + unsigned int pat_index, u32 flags) { gen6_pte_t pte = HSW_PTE_ADDR_ENCODE(addr) | GEN6_PTE_VALID; - switch (level) { + switch (pat_index) { case I915_CACHE_NONE: break; case I915_CACHE_WT:
When booting a kernel compiled with CONFIG_CFI_CLANG (kCFI), there is a CFI failure in ggtt_probe_common() when trying to call hsw_pte_encode() via an indirect call: [ 5.030027] CFI failure at ggtt_probe_common+0xd1/0x130 [i915] (target: hsw_pte_encode+0x0/0x30 [i915]; expected type: 0xf5c1d0fc) With kCFI, indirect calls are validated against their expected type versus actual type and failures occur when the two types do not match. clang's -Wincompatible-function-pointer-types-strict can catch this at compile time but it is not enabled for the kernel yet: drivers/gpu/drm/i915/gt/intel_ggtt.c:1155:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = iris_pte_encode; ^ ~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1157:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = hsw_pte_encode; ^ ~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1159:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = byt_pte_encode; ^ ~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1161:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = ivb_pte_encode; ^ ~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/intel_ggtt.c:1163:23: error: incompatible function pointer types assigning to 'u64 (*)(dma_addr_t, unsigned int, u32)' (aka 'unsigned long long (*)(unsigned int, unsigned int, unsigned int)') from 'u64 (dma_addr_t, enum i915_cache_level, u32)' (aka 'unsigned long long (unsigned int, enum i915_cache_level, unsigned int)') [-Werror,-Wincompatible-function-pointer-types-strict] ggtt->vm.pte_encode = snb_pte_encode; ^ ~~~~~~~~~~~~~~ 5 errors generated. In this case, the pre-gen8 pte_encode functions have a second parameter type of 'enum i915_cache_level' whereas the function pointer prototype in 'struct i915_address_space' expects a second parameter type of 'unsigned int'. Update the second parameter of the callbacks and the comment above them noting that these statements are still valid, which matches other functions and files, to clear up the kCFI failures at run time. Fixes: 9275277d5324 ("drm/i915: use pat_index instead of cache_level") Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)