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

[Christian König]

Am 31.07.23 um 09:03 schrieb Boris Brezillon:
> On Fri, 28 Jul 2023 10:17:57 -0700
> Nathan Chancellor <nathan@xxxxxxxxxx> wrote:
>
> > + people from trailers of 09593216bff1
> >
> > On Thu, Jul 27, 2023 at 03:50:58PM -0700, 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:
> >            ^ 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@xxxxxxxxxx>
> > > Reported-by: Naresh Kamboju <naresh.kamboju@xxxxxxxxxx>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> > Thanks for the patch!
> >
> > Tested-by: Nathan Chancellor <nathan@xxxxxxxxxx> # 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@xxxxxxxxxx
> > > ---
> > >   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@xxxxxxxxxx>
> > >