Re: [PATCH v2] drm: fix indirect goto into statement expression UB

["Christian König" <christian.koenig@amd.com>]

Am 31.07.23 um 09:03 schrieb Boris Brezillon:
> On Fri, 28 Jul 2023 10:17:57 -0700
> Nathan Chancellor <nathan@kernel.org> wrote:
>
>> + people from trailers of 09593216bff1
>>
>> On Thu, Jul 27, 2023 at 03:50:58PM -0700, ndesaulniers@google.com wrote:
>>> A new diagnostic in clang-17 now produces the following build error:
>>>
>>> drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this
>>> indirect goto statement to one of its possible targets
>>>     41 |                 drm_exec_retry_on_contention(&exec);
>>>        |                 ^
>>> include/drm/drm_exec.h:96:4: note: expanded from macro
>>> 'drm_exec_retry_on_contention'
>>>     96 |                         goto *__drm_exec_retry_ptr;
>>>        |                         ^
>>> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of
>>> indirect goto statement
>>>     39 |         drm_exec_until_all_locked(&exec) {
>>>        |         ^
>>> include/drm/drm_exec.h:79:33: note: expanded from macro
>>> 'drm_exec_until_all_locked'
>>>     79 |                 __label__ __drm_exec_retry;
>>> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a
>>> statement expression
>>>
>>> The GCC manually currently states that:
>>            ^ manual
>>
>>>>> Jumping into a statement expression with a computed goto (see Labels
>>>>> as Values) has undefined behavior.
>>> So the diagnostic appears correct, even if codegen happened to produce
>>> working code.
>>>
>>> Looking closer at this code, while the original combination of statement
>>> expression, local label, and computed/indirect goto GNU C expressions
>>> were clever, a simple while loop and continue block might have sufficed.
>>>
>>> This approach might not work as expected if drm_exec_until_all_locked
>>> "loops" can be nested, but that doesn't appear to be an existing use
>>> case in the codebase.
> Hm, that's exactly the sort of things we were trying to be robust
> against with the original approach. With this version, we're back to a
> situation where
>
> 	drm_exec_until_all_locked(exec) {
> 		for (...) {
> 			drm_exec_retry_on_contention(exec);
> 		}
> 	}
>
> doesn't do what we expect it to do, and that's a use case we want to
> support.

Yeah, agree that isn't what's supposed to happen here and would break a 
couple of use cases.

As a workaround we could define the label before the loop, but that 
makes the label local to the enclosing block, e.g. allows for using 
drm_exec_retry_on_contention() outside of drm_exec_until_all_locked().

Going to work on a patch, thanks for the notice.

Regards,
Christian.

>
>>> Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
>>> Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/1890
>>> Link: https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
>>> Reported-by: Nathan Chancellor <nathan@kernel.org>
>>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>
>>> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>> Thanks for the patch!
>>
>> Tested-by: Nathan Chancellor <nathan@kernel.org> # build
>>
>>> ---
>>> Changes in v2:
>>> Fix the continue to be outside of the do while
>>> - Link to v1: https://lore.kernel.org/r/20230727-amdgpu-v1-1-a95690e75388@google.com
>>> ---
>>>   include/drm/drm_exec.h | 21 +++++----------------
>>>   1 file changed, 5 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
>>> index 73205afec162..fa1cc5c3d021 100644
>>> --- a/include/drm/drm_exec.h
>>> +++ b/include/drm/drm_exec.h
>>> @@ -70,18 +70,8 @@ struct drm_exec {
>>>    * Core functionality of the drm_exec object. Loops until all GEM objects are
>>>    * locked and no more contention exists. At the beginning of the loop it is
>>>    * guaranteed that no GEM object is locked.
>>> - *
>>> - * Since labels can't be defined local to the loops body we use a jump pointer
>>> - * to make sure that the retry is only used from within the loops body.
>>>    */
>>> -#define drm_exec_until_all_locked(exec)				\
>>> -	for (void *__drm_exec_retry_ptr; ({			\
>>> -		__label__ __drm_exec_retry;			\
>>> -__drm_exec_retry:						\
>>> -		__drm_exec_retry_ptr = &&__drm_exec_retry;	\
>>> -		(void)__drm_exec_retry_ptr;			\
>>> -		drm_exec_cleanup(exec);				\
>>> -	});)
>>> +#define drm_exec_until_all_locked(exec)	while(drm_exec_cleanup(exec))
>>>   
>>>   /**
>>>    * drm_exec_retry_on_contention - restart the loop to grap all locks
>>> @@ -90,11 +80,10 @@ __drm_exec_retry:						\
>>>    * Control flow helper to continue when a contention was detected and we need to
>>>    * clean up and re-start the loop to prepare all GEM objects.
>>>    */
>>> -#define drm_exec_retry_on_contention(exec)			\
>>> -	do {							\
>>> -		if (unlikely(drm_exec_is_contended(exec)))	\
>>> -			goto *__drm_exec_retry_ptr;		\
>>> -	} while (0)
>>> +#define drm_exec_retry_on_contention(exec)		\
>>> +	if (unlikely(drm_exec_is_contended(exec)))	\
>>> +		continue;				\
>>> +	do {} while (0)
>>>   
>>>   /**
>>>    * drm_exec_is_contended - check for contention
>>>
>>> ---
>>> base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
>>> change-id: 20230727-amdgpu-93c0e5302951
>>>
>>> Best regards,
>>> -- 
>>> Nick Desaulniers <ndesaulniers@google.com>
>>>