diff mbox series

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

Message ID 20230727-amdgpu-v2-1-7fc66bc52bf6@google.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm: fix indirect goto into statement expression UB | expand

Commit Message

Nick Desaulniers July 27, 2023, 10:50 p.m. UTC 
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:
>> 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.

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>
---
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(-)


---
base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
change-id: 20230727-amdgpu-93c0e5302951

Best regards,

Comments

Nathan Chancellor July 28, 2023, 5:17 p.m. UTC | #1 
+ 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.
> 
> 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>
>
Boris Brezillon July 31, 2023, 7:03 a.m. UTC | #2 
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.

> > 
> > 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>
> >
Christian König July 31, 2023, 9:50 a.m. UTC | #3 
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>
>>>
diff mbox series

Patch

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