* [Intel-gfx] [PATCH i-g-t 2/3] tests/i915/kms_mmap_write_crc: handle missing gem_get_caching()
2022-06-27 16:10 [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf Matthew Auld
@ 2022-06-27 16:10 ` Matthew Auld
2022-06-28 12:26 ` Gwan-gyeong Mun
2022-06-27 16:10 ` [Intel-gfx] [PATCH i-g-t 3/3] tests/i915: adapt __copy_ccs for discrete Matthew Auld
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Matthew Auld @ 2022-06-27 16:10 UTC (permalink / raw)
To: igt-dev; +Cc: intel-gfx
The kernel is meant to force the caching level for the object to
CACHE_NONE or CACHE_WT when first scanning out the object, since the
display engine is not coherent (assuming userspace hasn't already done
this). On discrete we no longer support set/get_caching, but we can only
do the scanout from lmem, which can only be mapped as WC and so should
always be coherent for scanout. Adjust the test and ensure it still
passes as expected.
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5303
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
---
tests/i915/kms_mmap_write_crc.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/tests/i915/kms_mmap_write_crc.c b/tests/i915/kms_mmap_write_crc.c
index b17e5fdb..da7312d6 100644
--- a/tests/i915/kms_mmap_write_crc.c
+++ b/tests/i915/kms_mmap_write_crc.c
@@ -78,7 +78,6 @@ static void test(data_t *data)
drmModeModeInfo *mode;
cairo_t *cr;
char *ptr;
- uint32_t caching;
void *buf;
igt_crc_t crc;
@@ -102,9 +101,13 @@ static void test(data_t *data)
igt_plane_set_fb(data->primary, &data->fb[0]);
igt_display_commit(display);
- /* make sure caching mode has become UC/WT */
- caching = gem_get_caching(data->drm_fd, fb->gem_handle);
- igt_assert(caching == I915_CACHING_NONE || caching == I915_CACHING_DISPLAY);
+ if (!gem_has_lmem(data->drm_fd)) {
+ uint32_t caching;
+
+ /* make sure caching mode has become UC/WT */
+ caching = gem_get_caching(data->drm_fd, fb->gem_handle);
+ igt_assert(caching == I915_CACHING_NONE || caching == I915_CACHING_DISPLAY);
+ }
/*
* firstly demonstrate the need for DMA_BUF_SYNC_START ("begin_cpu_access")
--
2.36.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Intel-gfx] [PATCH i-g-t 2/3] tests/i915/kms_mmap_write_crc: handle missing gem_get_caching()
2022-06-27 16:10 ` [Intel-gfx] [PATCH i-g-t 2/3] tests/i915/kms_mmap_write_crc: handle missing gem_get_caching() Matthew Auld
@ 2022-06-28 12:26 ` Gwan-gyeong Mun
0 siblings, 0 replies; 8+ messages in thread
From: Gwan-gyeong Mun @ 2022-06-28 12:26 UTC (permalink / raw)
To: Matthew Auld, igt-dev; +Cc: intel-gfx
Looks good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
On 6/27/22 7:10 PM, Matthew Auld wrote:
> The kernel is meant to force the caching level for the object to
> CACHE_NONE or CACHE_WT when first scanning out the object, since the
> display engine is not coherent (assuming userspace hasn't already done
> this). On discrete we no longer support set/get_caching, but we can only
> do the scanout from lmem, which can only be mapped as WC and so should
> always be coherent for scanout. Adjust the test and ensure it still
> passes as expected.
>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5303
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> ---
> tests/i915/kms_mmap_write_crc.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/tests/i915/kms_mmap_write_crc.c b/tests/i915/kms_mmap_write_crc.c
> index b17e5fdb..da7312d6 100644
> --- a/tests/i915/kms_mmap_write_crc.c
> +++ b/tests/i915/kms_mmap_write_crc.c
> @@ -78,7 +78,6 @@ static void test(data_t *data)
> drmModeModeInfo *mode;
> cairo_t *cr;
> char *ptr;
> - uint32_t caching;
> void *buf;
> igt_crc_t crc;
>
> @@ -102,9 +101,13 @@ static void test(data_t *data)
> igt_plane_set_fb(data->primary, &data->fb[0]);
> igt_display_commit(display);
>
> - /* make sure caching mode has become UC/WT */
> - caching = gem_get_caching(data->drm_fd, fb->gem_handle);
> - igt_assert(caching == I915_CACHING_NONE || caching == I915_CACHING_DISPLAY);
> + if (!gem_has_lmem(data->drm_fd)) {
> + uint32_t caching;
> +
> + /* make sure caching mode has become UC/WT */
> + caching = gem_get_caching(data->drm_fd, fb->gem_handle);
> + igt_assert(caching == I915_CACHING_NONE || caching == I915_CACHING_DISPLAY);
> + }
>
> /*
> * firstly demonstrate the need for DMA_BUF_SYNC_START ("begin_cpu_access")
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Intel-gfx] [PATCH i-g-t 3/3] tests/i915: adapt __copy_ccs for discrete
2022-06-27 16:10 [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf Matthew Auld
2022-06-27 16:10 ` [Intel-gfx] [PATCH i-g-t 2/3] tests/i915/kms_mmap_write_crc: handle missing gem_get_caching() Matthew Auld
@ 2022-06-27 16:10 ` Matthew Auld
2022-06-28 10:24 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf Das, Nirmoy
2022-06-28 10:24 ` [Intel-gfx] " Gwan-gyeong Mun
3 siblings, 0 replies; 8+ messages in thread
From: Matthew Auld @ 2022-06-27 16:10 UTC (permalink / raw)
To: igt-dev; +Cc: intel-gfx
We can't explicitly control the mmap caching type for discrete, but
using mmap_device_coherent should be good enough here on such devices.
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4842
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
---
lib/intel_bufops.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/lib/intel_bufops.c b/lib/intel_bufops.c
index 05c0b0d4..c63a5760 100644
--- a/lib/intel_bufops.c
+++ b/lib/intel_bufops.c
@@ -451,11 +451,16 @@ static void __copy_ccs(struct buf_ops *bops, struct intel_buf *buf,
ccs_size = CCS_SIZE(gen, buf);
size = offset + ccs_size;
- map = __gem_mmap_offset__wc(bops->fd, buf->handle, 0, size,
- PROT_READ | PROT_WRITE);
- if (!map)
- map = gem_mmap__wc(bops->fd, buf->handle, 0, size,
- PROT_READ | PROT_WRITE);
+ if (gem_has_lmem(bops->fd)) {
+ map = gem_mmap__device_coherent(bops->fd, buf->handle, 0, size,
+ PROT_READ | PROT_WRITE);
+ } else {
+ map = __gem_mmap_offset__wc(bops->fd, buf->handle, 0, size,
+ PROT_READ | PROT_WRITE);
+ if (!map)
+ map = gem_mmap__wc(bops->fd, buf->handle, 0, size,
+ PROT_READ | PROT_WRITE);
+ }
switch (dir) {
case CCS_LINEAR_TO_BUF:
--
2.36.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf
2022-06-27 16:10 [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf Matthew Auld
2022-06-27 16:10 ` [Intel-gfx] [PATCH i-g-t 2/3] tests/i915/kms_mmap_write_crc: handle missing gem_get_caching() Matthew Auld
2022-06-27 16:10 ` [Intel-gfx] [PATCH i-g-t 3/3] tests/i915: adapt __copy_ccs for discrete Matthew Auld
@ 2022-06-28 10:24 ` Das, Nirmoy
2022-06-28 10:24 ` [Intel-gfx] " Gwan-gyeong Mun
3 siblings, 0 replies; 8+ messages in thread
From: Das, Nirmoy @ 2022-06-28 10:24 UTC (permalink / raw)
To: Matthew Auld, igt-dev; +Cc: intel-gfx
The series is Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>
On 6/27/2022 6:10 PM, Matthew Auld wrote:
> ../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after ‘free’ [-Wuse-after-free]
> 277 | igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
> ../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
> 667 | do { if (!(expr)) \
> | ^~~~
> ../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
> 274 | free(ctx);
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> ---
> tests/i915/gem_eio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
> index 913a21f9..6cbae6eb 100644
> --- a/tests/i915/gem_eio.c
> +++ b/tests/i915/gem_eio.c
> @@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
> igt_nsec_elapsed(&ctx->delay) / 1000.0);
>
> igt_assert_eq(timer_delete(ctx->timer), 0);
> - free(ctx);
>
> /* flush any excess work before we start timing our reset */
> igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
> "%d", DROP_RCU));
> + free(ctx);
>
> igt_nsec_elapsed(ts);
> igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf
2022-06-27 16:10 [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf Matthew Auld
` (2 preceding siblings ...)
2022-06-28 10:24 ` [Intel-gfx] [igt-dev] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf Das, Nirmoy
@ 2022-06-28 10:24 ` Gwan-gyeong Mun
2022-06-28 12:47 ` Matthew Auld
3 siblings, 1 reply; 8+ messages in thread
From: Gwan-gyeong Mun @ 2022-06-28 10:24 UTC (permalink / raw)
To: Matthew Auld, igt-dev; +Cc: intel-gfx
Looks good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
However, the use after free build issue did not occur only with the "$
meson build && ninja -C build" build command guided by the igt
README.md. How did you check it?
Br,
G.G.
On 6/27/22 7:10 PM, Matthew Auld wrote:
> ../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after ‘free’ [-Wuse-after-free]
> 277 | igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
> ../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
> 667 | do { if (!(expr)) \
> | ^~~~
> ../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
> 274 | free(ctx);
>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> ---
> tests/i915/gem_eio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
> index 913a21f9..6cbae6eb 100644
> --- a/tests/i915/gem_eio.c
> +++ b/tests/i915/gem_eio.c
> @@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
> igt_nsec_elapsed(&ctx->delay) / 1000.0);
>
> igt_assert_eq(timer_delete(ctx->timer), 0);
> - free(ctx);
>
> /* flush any excess work before we start timing our reset */
> igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
> "%d", DROP_RCU));
> + free(ctx);
>
> igt_nsec_elapsed(ts);
> igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf
2022-06-28 10:24 ` [Intel-gfx] " Gwan-gyeong Mun
@ 2022-06-28 12:47 ` Matthew Auld
2022-06-28 13:59 ` Gwan-gyeong Mun
0 siblings, 1 reply; 8+ messages in thread
From: Matthew Auld @ 2022-06-28 12:47 UTC (permalink / raw)
To: Gwan-gyeong Mun, igt-dev; +Cc: intel-gfx
On 28/06/2022 11:24, Gwan-gyeong Mun wrote:
> Looks good to me.
>
> Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>
> However, the use after free build issue did not occur only with the "$
> meson build && ninja -C build" build command guided by the igt
> README.md. How did you check it?
Hmm, I assume it's just a difference in compiler version or so. I have:
gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1).
>
> Br,
>
> G.G.
>
>
> On 6/27/22 7:10 PM, Matthew Auld wrote:
>> ../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after
>> ‘free’ [-Wuse-after-free]
>> 277 | igt_assert(igt_sysfs_printf(ctx->debugfs,
>> "i915_drop_caches",
>> ../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
>> 667 | do { if (!(expr)) \
>> | ^~~~
>> ../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
>> 274 | free(ctx);
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>> ---
>> tests/i915/gem_eio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
>> index 913a21f9..6cbae6eb 100644
>> --- a/tests/i915/gem_eio.c
>> +++ b/tests/i915/gem_eio.c
>> @@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
>> igt_nsec_elapsed(&ctx->delay) / 1000.0);
>> igt_assert_eq(timer_delete(ctx->timer), 0);
>> - free(ctx);
>> /* flush any excess work before we start timing our reset */
>> igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
>> "%d", DROP_RCU));
>> + free(ctx);
>> igt_nsec_elapsed(ts);
>> igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));
>>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Intel-gfx] [PATCH i-g-t 1/3] tests/i915/gem_eio: fix uaf
2022-06-28 12:47 ` Matthew Auld
@ 2022-06-28 13:59 ` Gwan-gyeong Mun
0 siblings, 0 replies; 8+ messages in thread
From: Gwan-gyeong Mun @ 2022-06-28 13:59 UTC (permalink / raw)
To: Matthew Auld, igt-dev; +Cc: intel-gfx
On 6/28/22 3:47 PM, Matthew Auld wrote:
> On 28/06/2022 11:24, Gwan-gyeong Mun wrote:
>> Looks good to me.
>>
>> Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>>
>> However, the use after free build issue did not occur only with the "$
>> meson build && ninja -C build" build command guided by the igt
>> README.md. How did you check it?
>
> Hmm, I assume it's just a difference in compiler version or so. I have:
> gcc (GCC) 12.1.1 20220507 (Red Hat 12.1.1-1).
>
Thanks for sharing your compiling environment info.
My gcc says its version is 11.1.0.
I'll try with the version you mentioned.
$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr
--libdir=/usr/lib --libexecdir=/usr/lib --mandir=/usr/share/man
--infodir=/usr/share/info --with-bugurl=https://bugs.archlinux.org/
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++,d --with-isl
--with-linker-hash-style=gnu --with-system-zlib --enable-__cxa_atexit
--enable-cet=auto --enable-checking=release --enable-clocale=gnu
--enable-default-pie --enable-default-ssp --enable-gnu-indirect-function
--enable-gnu-unique-object --enable-install-libiberty
--enable-linker-build-id --enable-lto --enable-multilib --enable-plugin
--enable-shared --enable-threads=posix --disable-libssp
--disable-libstdcxx-pch --disable-libunwind-exceptions --disable-werror
gdc_include_dir=/usr/include/dlang/gdc
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.1.0 (GCC)
Thanks,
G.G.
>>
>> Br,
>>
>> G.G.
>>
>>
>> On 6/27/22 7:10 PM, Matthew Auld wrote:
>>> ../tests/i915/gem_eio.c:277:20: warning: pointer ‘ctx’ used after
>>> ‘free’ [-Wuse-after-free]
>>> 277 | igt_assert(igt_sysfs_printf(ctx->debugfs,
>>> "i915_drop_caches",
>>> ../lib/igt_core.h:667:20: note: in definition of macro ‘igt_assert’
>>> 667 | do { if (!(expr)) \
>>> | ^~~~
>>> ../tests/i915/gem_eio.c:274:9: note: call to ‘free’ here
>>> 274 | free(ctx);
>>>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
>>> ---
>>> tests/i915/gem_eio.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
>>> index 913a21f9..6cbae6eb 100644
>>> --- a/tests/i915/gem_eio.c
>>> +++ b/tests/i915/gem_eio.c
>>> @@ -270,11 +270,11 @@ static void hang_handler(union sigval arg)
>>> igt_nsec_elapsed(&ctx->delay) / 1000.0);
>>> igt_assert_eq(timer_delete(ctx->timer), 0);
>>> - free(ctx);
>>> /* flush any excess work before we start timing our reset */
>>> igt_assert(igt_sysfs_printf(ctx->debugfs, "i915_drop_caches",
>>> "%d", DROP_RCU));
>>> + free(ctx);
>>> igt_nsec_elapsed(ts);
>>> igt_assert(igt_sysfs_printf(dir, "i915_wedged", "%llu", -1ull));
>>>
^ permalink raw reply [flat|nested] 8+ messages in thread