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

[Boris Brezillon <boris.brezillon@collabora.com>]

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