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

[Nick Desaulniers]

On Thu, Jul 27, 2023 at 3:47 PM <ndesaulniers@xxxxxxxxxx> 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:
> >> 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@xxxxxxxxxx>
> Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> ---
>  include/drm/drm_exec.h | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 73205afec162..393ac022ed3a 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -70,18 +70,9 @@ 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);                         \
> -       });)
> +       while(drm_exec_cleanup(exec))
>
>  /**
>   * drm_exec_retry_on_contention - restart the loop to grap all locks
> @@ -93,7 +84,7 @@ __drm_exec_retry:                                             \
>  #define drm_exec_retry_on_contention(exec)                     \
>         do {                                                    \
>                 if (unlikely(drm_exec_is_contended(exec)))      \
> -                       goto *__drm_exec_retry_ptr;             \
> +                       continue;                               \

d'oh that's going to continue the do {} while(0) ... let me send a v2...

>         } while (0)
>
>  /**
>
> ---
> base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
> change-id: 20230727-amdgpu-93c0e5302951
>
> Best regards,
> --
> Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
>


-- 
Thanks,
~Nick Desaulniers