| Message ID | 20200529172505.fdjppgquujab7ayv@treble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/uaccess: Remove redundant likely/unlikely annotations | expand |
On 5/29/20 10:25 AM, Josh Poimboeuf wrote: > On Fri, May 29, 2020 at 06:54:19PM +0200, Peter Zijlstra wrote: >> On Fri, May 29, 2020 at 11:50:11AM -0500, Josh Poimboeuf wrote: >>> The nested likelys seem like overkill anyway -- user_access_begin() is >>> __always_inline and it already has unlikely(), which should be >>> propagated. >>> >>> So just remove the outer likelys? >> >> That fixes it. Ack! > > If there are no objections to the patch, I can add it to my objtool-core > branch unless anybody else wants to take it. It only affects > linux-next. > > ---8<--- > > From: Josh Poimboeuf <jpoimboe@redhat.com> > Subject: [PATCH] x86/uaccess: Remove redundant likely/unlikely annotations > > Since user_access_begin() already has an unlikely() annotation for its > access_ok() check, "if (likely(user_access_begin))" results in nested > likely annotations. When combined with CONFIG_TRACE_BRANCH_PROFILING, > GCC converges the error/success paths of the nested ifs, using a > register value to distinguish between them. > > While the code is technically uaccess safe, it complicates the > branch-profiling generated code. It also confuses objtool, because it > doesn't do register value tracking, resulting in the following warnings: > > arch/x86/lib/csum-wrappers_64.o: warning: objtool: csum_and_copy_from_user()+0x2a4: call to memset() with UACCESS enabled > arch/x86/lib/csum-wrappers_64.o: warning: objtool: csum_and_copy_to_user()+0x243: return with UACCESS enabled > > The outer likely annotations aren't actually needed anyway, since the > compiler propagates the error path coldness when it inlines > user_access_begin(). > > Fixes: 18372ef87665 ("x86_64: csum_..._copy_..._user(): switch to unsafe_..._user()") > Reported-by: Randy Dunlap <rdunlap@infradead.org> > Acked-by: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested Thanks. > --- > arch/x86/lib/csum-wrappers_64.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c > index a12b8629206d..ee63d7576fd2 100644 > --- a/arch/x86/lib/csum-wrappers_64.c > +++ b/arch/x86/lib/csum-wrappers_64.c > @@ -27,7 +27,7 @@ csum_and_copy_from_user(const void __user *src, void *dst, > might_sleep(); > *errp = 0; > > - if (!likely(user_access_begin(src, len))) > + if (!user_access_begin(src, len)) > goto out_err; > > /* > @@ -89,7 +89,7 @@ csum_and_copy_to_user(const void *src, void __user *dst, > > might_sleep(); > > - if (unlikely(!user_access_begin(dst, len))) { > + if (!user_access_begin(dst, len)) { > *errp = -EFAULT; > return 0; > } >
diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c index a12b8629206d..ee63d7576fd2 100644 --- a/arch/x86/lib/csum-wrappers_64.c +++ b/arch/x86/lib/csum-wrappers_64.c @@ -27,7 +27,7 @@ csum_and_copy_from_user(const void __user *src, void *dst, might_sleep(); *errp = 0; - if (!likely(user_access_begin(src, len))) + if (!user_access_begin(src, len)) goto out_err; /* @@ -89,7 +89,7 @@ csum_and_copy_to_user(const void *src, void __user *dst, might_sleep(); - if (unlikely(!user_access_begin(dst, len))) { + if (!user_access_begin(dst, len)) { *errp = -EFAULT; return 0; }