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

From: Nathan Chancellor <nathan@kernel.org>
To: ndesaulniers@google.com
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Tom Rix <trix@redhat.com>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	llvm@lists.linux.dev, Naresh Kamboju <naresh.kamboju@linaro.org>,
	christian.koenig@amd.com, boris.brezillon@collabora.com,
	dakr@redhat.com, alexander.deucher@amd.com
Subject: Re: [PATCH v2] drm: fix indirect goto into statement expression UB
Date: Fri, 28 Jul 2023 10:17:57 -0700	[thread overview]
Message-ID: <20230728171757.GA433645@dev-arch.thelio-3990X> (raw)
In-Reply-To: <20230727-amdgpu-v2-1-7fc66bc52bf6@google.com>

+ 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>
> 