Re: mmotm 2020-05-13-20-30 uploaded (objtool warnings)

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, May 29, 2020 at 05:33:36PM +0200, Peter Zijlstra wrote:
> On Fri, May 29, 2020 at 04:53:25PM +0200, Peter Zijlstra wrote:
> > On Fri, May 29, 2020 at 04:35:56PM +0200, Peter Zijlstra wrote:
> 
> > *groan*, this is one of those CONFIG_PROFILE_ALL_BRANCHES builds. If I
> > disable that it goes away.
> > 
> > Still trying to untangle the mess it generated, but on first go it
> > looks like objtool is right, but I'm not sure what went wrong.
> 
> $ tools/objtool/objtool check -fab arch/x86/lib/csum-wrappers_64.o
> arch/x86/lib/csum-wrappers_64.o: warning: objtool: csum_and_copy_from_user()+0x29f: call to memset() with UACCESS enabled
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0x283: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0x113: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   .altinstr_replacement+0xffffffffffffffff: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0xea: (alt)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   .altinstr_replacement+0xffffffffffffffff: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0xe7: (alt)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0xd2: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0x7e: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0x43: (branch)
> arch/x86/lib/csum-wrappers_64.o: warning: objtool:   csum_and_copy_from_user()+0x0: <=== (sym)
> 
> The problem is with the +0x113 branch, which is at 0x1d1.
> 
> That looks to be:
> 
> 	if (!likely(user_access_begin(src, len)))
> 		goto out_err;
> 
> Except that the brach profiling stuff confused GCC enough to leak STAC
> into the error path or something.

It looks to me like GCC is doing the right thing.  That likely()
translates to:

#  define likely(x)	(__branch_check__(x, 1, __builtin_constant_p(x)))

which becomes:

#define __branch_check__(x, expect, is_constant) ({			\
			long ______r;					\
			static struct ftrace_likely_data		\
				__aligned(4)				\
				__section(_ftrace_annotated_branch)	\
				______f = {				\
				.data.func = __func__,			\
				.data.file = __FILE__,			\
				.data.line = __LINE__,			\
			};						\
			______r = __builtin_expect(!!(x), expect);	\
			ftrace_likely_update(&______f, ______r,		\
					     expect, is_constant);	\
			______r;					\
		})

Here 'x' is the call to user_access_begin().  It evaluates 'x' -- and
thus calls user_access_begin() -- before the call to
ftrace_likely_update().

So it's working as designed, right?  The likely() just needs to be
changed to likely_notrace().

-- 
Josh