[igt-dev] [PATCH i-g-t v2] igt/gem_exec_params: Drop drm master privileges only on drm master fds

[Antonio Argenziano]

On 31/01/18 13:26, Chris Wilson wrote:
> Quoting Antonio Argenziano (2018-01-31 21:21:32)
>> Subtest 'secure-non-master' tries to drop drm-master privileges on a
>> non drm-master fd. Instead drop master privileges after the subtest
>> acquires it so that other subtests are not affected by it.
>>
>> v2:
>>          - Require root privilege for the sub-test. (Chris)
>>
>> Signed-off-by: Antonio Argenziano <antonio.argenziano at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>   tests/gem_exec_params.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/tests/gem_exec_params.c b/tests/gem_exec_params.c
>> index e1c48b40..2f9227e3 100644
>> --- a/tests/gem_exec_params.c
>> +++ b/tests/gem_exec_params.c
>> @@ -343,13 +343,18 @@ igt_main
>>          }
>>   
>>          igt_subtest("secure-non-master") {
>> +               igt_require(__igt_device_set_master(fd)); /* Requires root privilege */
>>                  igt_device_drop_master(fd);
>> +
>>                  execbuf.flags = I915_EXEC_RENDER | I915_EXEC_SECURE;
>>                  RUN_FAIL(EPERM);
> 
> Fwiw, I like the spacing around the second stanza better as that couples
> acquiring root with the successful test. So I'd like the
> igt_device_drop_master() next to the RUN_FAIL block.
> 
> igt_assert_eq(__gem_execbuf(fd, &execbuf), -EPERM);
> 
>> +
>>                  igt_device_set_master(fd);
>>                  igt_assert(drmIoctl(fd,
>>                                      DRM_IOCTL_I915_GEM_EXECBUFFER2,
>>                                      &execbuf) == 0);
> gem_execbuf(fd, &execbuf);
> 
>> +
>> +               igt_device_drop_master(fd); /* Only needs temporary master */
>>          }
>>   
>>          /* HANDLE_LUT and NO_RELOC are already exercised by gem_exec_lut_handle,
> 
> Preferably with the whitespace tweak, and with or without the migration
> to gem_execbuf/__gem_execbuf,

I'll do the whitespace change but split the transition to gem_execbuf in 
a separate patch since it affects the whole test because of RUN_FAIL.

Thanks,
Antonio

> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris
>