* [PATCH 0/2] uaccess fixes
@ 2019-07-24 22:47 Josh Poimboeuf
2019-07-24 22:47 ` [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path Josh Poimboeuf
2019-07-24 22:47 ` [PATCH 2/2] objtool: Improve UACCESS coverage Josh Poimboeuf
0 siblings, 2 replies; 6+ messages in thread
From: Josh Poimboeuf @ 2019-07-24 22:47 UTC (permalink / raw)
To: x86
Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Chris Wilson,
Linus Torvalds, Sedat Dilek, Nick Desaulniers, Nathan Chancellor,
Arnd Bergmann
An i915 redundant CLAC fix, and an objtool fix which would have caught
it earlier.
Josh Poimboeuf (1):
drm/i915: Remove redundant user_access_end() from __copy_from_user()
error path
Peter Zijlstra (1):
objtool: Improve UACCESS coverage
.../gpu/drm/i915/gem/i915_gem_execbuffer.c | 20 +++++++++----------
tools/objtool/check.c | 7 ++++---
tools/objtool/check.h | 3 ++-
3 files changed, 15 insertions(+), 15 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path 2019-07-24 22:47 [PATCH 0/2] uaccess fixes Josh Poimboeuf @ 2019-07-24 22:47 ` Josh Poimboeuf 2019-07-25 6:10 ` Sedat Dilek 2019-07-24 22:47 ` [PATCH 2/2] objtool: Improve UACCESS coverage Josh Poimboeuf 1 sibling, 1 reply; 6+ messages in thread From: Josh Poimboeuf @ 2019-07-24 22:47 UTC (permalink / raw) To: x86 Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Chris Wilson, Linus Torvalds, Sedat Dilek, Nick Desaulniers, Nathan Chancellor, Arnd Bergmann Objtool reports: drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x36: redundant UACCESS disable __copy_from_user() already does both STAC and CLAC, so the user_access_end() in its error path adds an extra unnecessary CLAC. Fixes: 0b2c8f8b6b0c ("i915: fix missing user_access_end() in page fault exception case") Reported-by: Thomas Gleixner <tglx@linutronix.de> Reported-by: Sedat Dilek <sedat.dilek@gmail.com> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> Tested-by: Nick Desaulniers <ndesaulniers@google.com> Link: https://github.com/ClangBuiltLinux/linux/issues/617 Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 5fae0e50aad0..41dab9ea33cd 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1628,6 +1628,7 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) static int eb_copy_relocations(const struct i915_execbuffer *eb) { + struct drm_i915_gem_relocation_entry *relocs; const unsigned int count = eb->buffer_count; unsigned int i; int err; @@ -1635,7 +1636,6 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) for (i = 0; i < count; i++) { const unsigned int nreloc = eb->exec[i].relocation_count; struct drm_i915_gem_relocation_entry __user *urelocs; - struct drm_i915_gem_relocation_entry *relocs; unsigned long size; unsigned long copied; @@ -1663,14 +1663,8 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) if (__copy_from_user((char *)relocs + copied, (char __user *)urelocs + copied, - len)) { -end_user: - user_access_end(); -end: - kvfree(relocs); - err = -EFAULT; - goto err; - } + len)) + goto end; copied += len; } while (copied < size); @@ -1699,10 +1693,14 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) return 0; +end_user: + user_access_end(); +end: + kvfree(relocs); + err = -EFAULT; err: while (i--) { - struct drm_i915_gem_relocation_entry *relocs = - u64_to_ptr(typeof(*relocs), eb->exec[i].relocs_ptr); + relocs = u64_to_ptr(typeof(*relocs), eb->exec[i].relocs_ptr); if (eb->exec[i].relocation_count) kvfree(relocs); } -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path 2019-07-24 22:47 ` [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path Josh Poimboeuf @ 2019-07-25 6:10 ` Sedat Dilek 2019-07-25 20:21 ` Josh Poimboeuf 0 siblings, 1 reply; 6+ messages in thread From: Sedat Dilek @ 2019-07-25 6:10 UTC (permalink / raw) To: Josh Poimboeuf Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner, Chris Wilson, Linus Torvalds, Nick Desaulniers, Nathan Chancellor, Arnd Bergmann On Thu, Jul 25, 2019 at 12:48 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > Objtool reports: > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x36: redundant UACCESS disable > > __copy_from_user() already does both STAC and CLAC, so the > user_access_end() in its error path adds an extra unnecessary CLAC. > > Fixes: 0b2c8f8b6b0c ("i915: fix missing user_access_end() in page fault exception case") > Reported-by: Thomas Gleixner <tglx@linutronix.de> > Reported-by: Sedat Dilek <sedat.dilek@gmail.com> > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > Tested-by: Nick Desaulniers <ndesaulniers@google.com> > Link: https://github.com/ClangBuiltLinux/linux/issues/617 > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> Just for the records and ensuing ages: Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 20 +++++++++---------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 5fae0e50aad0..41dab9ea33cd 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -1628,6 +1628,7 @@ static int check_relocations(const struct drm_i915_gem_exec_object2 *entry) > > static int eb_copy_relocations(const struct i915_execbuffer *eb) > { > + struct drm_i915_gem_relocation_entry *relocs; > const unsigned int count = eb->buffer_count; > unsigned int i; > int err; > @@ -1635,7 +1636,6 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) > for (i = 0; i < count; i++) { > const unsigned int nreloc = eb->exec[i].relocation_count; > struct drm_i915_gem_relocation_entry __user *urelocs; > - struct drm_i915_gem_relocation_entry *relocs; > unsigned long size; > unsigned long copied; > > @@ -1663,14 +1663,8 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) > > if (__copy_from_user((char *)relocs + copied, > (char __user *)urelocs + copied, > - len)) { > -end_user: > - user_access_end(); > -end: > - kvfree(relocs); > - err = -EFAULT; > - goto err; > - } > + len)) > + goto end; > > copied += len; > } while (copied < size); > @@ -1699,10 +1693,14 @@ static int eb_copy_relocations(const struct i915_execbuffer *eb) > > return 0; > > +end_user: > + user_access_end(); > +end: > + kvfree(relocs); > + err = -EFAULT; > err: > while (i--) { > - struct drm_i915_gem_relocation_entry *relocs = > - u64_to_ptr(typeof(*relocs), eb->exec[i].relocs_ptr); > + relocs = u64_to_ptr(typeof(*relocs), eb->exec[i].relocs_ptr); > if (eb->exec[i].relocation_count) > kvfree(relocs); > } > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path 2019-07-25 6:10 ` Sedat Dilek @ 2019-07-25 20:21 ` Josh Poimboeuf 0 siblings, 0 replies; 6+ messages in thread From: Josh Poimboeuf @ 2019-07-25 20:21 UTC (permalink / raw) To: Sedat Dilek Cc: x86, linux-kernel, Peter Zijlstra, Thomas Gleixner, Chris Wilson, Linus Torvalds, Nick Desaulniers, Nathan Chancellor, Arnd Bergmann On Thu, Jul 25, 2019 at 08:10:49AM +0200, Sedat Dilek wrote: > On Thu, Jul 25, 2019 at 12:48 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > > > Objtool reports: > > > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x36: redundant UACCESS disable > > > > __copy_from_user() already does both STAC and CLAC, so the > > user_access_end() in its error path adds an extra unnecessary CLAC. > > > > Fixes: 0b2c8f8b6b0c ("i915: fix missing user_access_end() in page fault exception case") > > Reported-by: Thomas Gleixner <tglx@linutronix.de> > > Reported-by: Sedat Dilek <sedat.dilek@gmail.com> > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > Tested-by: Nick Desaulniers <ndesaulniers@google.com> > > Link: https://github.com/ClangBuiltLinux/linux/issues/617 > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> > > Just for the records and ensuing ages: > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> Thanks Sedat. Since the objtool fix already got merged separately I'll resend this patch to the drm folks. -- Josh ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] objtool: Improve UACCESS coverage 2019-07-24 22:47 [PATCH 0/2] uaccess fixes Josh Poimboeuf 2019-07-24 22:47 ` [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path Josh Poimboeuf @ 2019-07-24 22:47 ` Josh Poimboeuf 2019-07-25 6:40 ` [tip:core/urgent] " tip-bot for Peter Zijlstra 1 sibling, 1 reply; 6+ messages in thread From: Josh Poimboeuf @ 2019-07-24 22:47 UTC (permalink / raw) To: x86 Cc: linux-kernel, Peter Zijlstra, Thomas Gleixner, Chris Wilson, Linus Torvalds, Sedat Dilek, Nick Desaulniers, Nathan Chancellor, Arnd Bergmann From: Peter Zijlstra <peterz@infradead.org> A clang build reported an (obvious) double CLAC while a GCC build did not; it turns out we only re-visit instructions if the first visit was with AC=0. If OTOH the first visit was with AC=1, we completely ignore any subsequent visit, even when it has AC=0. Fix this by using a visited mask instead of a boolean, and (explicitly) mark the AC state. $ ./objtool check -b --no-fp --retpoline --uaccess drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x22: redundant UACCESS disable drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0xea: (alt) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0xffffffffffffffff: (branch) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0xd9: (alt) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0xb2: (branch) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0x39: (branch) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0x0: <=== (func) Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> Reported-by: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Reported-by: Sedat Dilek <sedat.dilek@gmail.com> Tested-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Sedat Dilek <sedat.dilek@gmail.com> Link: https://github.com/ClangBuiltLinux/linux/issues/617 Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> --- tools/objtool/check.c | 7 ++++--- tools/objtool/check.h | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 5f26620f13f5..176f2f084060 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1946,6 +1946,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, struct alternative *alt; struct instruction *insn, *next_insn; struct section *sec; + u8 visited; int ret; insn = first; @@ -1972,12 +1973,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, return 1; } + visited = 1 << state.uaccess; if (insn->visited) { if (!insn->hint && !insn_state_match(insn, &state)) return 1; - /* If we were here with AC=0, but now have AC=1, go again */ - if (insn->state.uaccess || !state.uaccess) + if (insn->visited & visited) return 0; } @@ -2024,7 +2025,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, } else insn->state = state; - insn->visited = true; + insn->visited |= visited; if (!insn->ignore_alts) { bool skip_orig = false; diff --git a/tools/objtool/check.h b/tools/objtool/check.h index b881fafcf55d..6d875ca6fce0 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -33,8 +33,9 @@ struct instruction { unsigned int len; enum insn_type type; unsigned long immediate; - bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts; + bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts; bool retpoline_safe; + u8 visited; struct symbol *call_dest; struct instruction *jump_dest; struct instruction *first_jump_src; -- 2.20.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:core/urgent] objtool: Improve UACCESS coverage 2019-07-24 22:47 ` [PATCH 2/2] objtool: Improve UACCESS coverage Josh Poimboeuf @ 2019-07-25 6:40 ` tip-bot for Peter Zijlstra 0 siblings, 0 replies; 6+ messages in thread From: tip-bot for Peter Zijlstra @ 2019-07-25 6:40 UTC (permalink / raw) To: linux-tip-commits Cc: hpa, ndesaulniers, natechancellor, sedat.dilek, linux-kernel, peterz, jpoimboe, tglx, mingo Commit-ID: 882a0db9d143e5e8dac54b96e83135bccd1f68d1 Gitweb: https://git.kernel.org/tip/882a0db9d143e5e8dac54b96e83135bccd1f68d1 Author: Peter Zijlstra <peterz@infradead.org> AuthorDate: Wed, 24 Jul 2019 17:47:26 -0500 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Thu, 25 Jul 2019 08:36:39 +0200 objtool: Improve UACCESS coverage A clang build reported an (obvious) double CLAC while a GCC build did not; it turns out that objtool only re-visits instructions if the first visit was with AC=0. If OTOH the first visit was with AC=1, it completely ignores any subsequent visit, even when it has AC=0. Fix this by using a visited mask instead of a boolean, and (explicitly) mark the AC state. $ ./objtool check -b --no-fp --retpoline --uaccess drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0x22: redundant UACCESS disable drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0xea: (alt) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: .altinstr_replacement+0xffffffffffffffff: (branch) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0xd9: (alt) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0xb2: (branch) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0x39: (branch) drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning: objtool: eb_copy_relocations.isra.34()+0x0: <=== (func) Reported-by: Josh Poimboeuf <jpoimboe@redhat.com> Reported-by: Thomas Gleixner <tglx@linutronix.de> Reported-by: Sedat Dilek <sedat.dilek@gmail.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> Signed-off-by: Thomas Gleixner <tglx@linutronix.de> Tested-by: Nathan Chancellor <natechancellor@gmail.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Sedat Dilek <sedat.dilek@gmail.com> Link: https://github.com/ClangBuiltLinux/linux/issues/617 Link: https://lkml.kernel.org/r/5359166aad2d53f3145cd442d83d0e5115e0cd17.1564007838.git.jpoimboe@redhat.com --- tools/objtool/check.c | 7 ++++--- tools/objtool/check.h | 3 ++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tools/objtool/check.c b/tools/objtool/check.c index 5f26620f13f5..176f2f084060 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -1946,6 +1946,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, struct alternative *alt; struct instruction *insn, *next_insn; struct section *sec; + u8 visited; int ret; insn = first; @@ -1972,12 +1973,12 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, return 1; } + visited = 1 << state.uaccess; if (insn->visited) { if (!insn->hint && !insn_state_match(insn, &state)) return 1; - /* If we were here with AC=0, but now have AC=1, go again */ - if (insn->state.uaccess || !state.uaccess) + if (insn->visited & visited) return 0; } @@ -2024,7 +2025,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func, } else insn->state = state; - insn->visited = true; + insn->visited |= visited; if (!insn->ignore_alts) { bool skip_orig = false; diff --git a/tools/objtool/check.h b/tools/objtool/check.h index b881fafcf55d..6d875ca6fce0 100644 --- a/tools/objtool/check.h +++ b/tools/objtool/check.h @@ -33,8 +33,9 @@ struct instruction { unsigned int len; enum insn_type type; unsigned long immediate; - bool alt_group, visited, dead_end, ignore, hint, save, restore, ignore_alts; + bool alt_group, dead_end, ignore, hint, save, restore, ignore_alts; bool retpoline_safe; + u8 visited; struct symbol *call_dest; struct instruction *jump_dest; struct instruction *first_jump_src; ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-25 20:21 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-24 22:47 [PATCH 0/2] uaccess fixes Josh Poimboeuf 2019-07-24 22:47 ` [PATCH 1/2] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path Josh Poimboeuf 2019-07-25 6:10 ` Sedat Dilek 2019-07-25 20:21 ` Josh Poimboeuf 2019-07-24 22:47 ` [PATCH 2/2] objtool: Improve UACCESS coverage Josh Poimboeuf 2019-07-25 6:40 ` [tip:core/urgent] " tip-bot for Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
Powered by JetHome