linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 related	[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 related	[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
* [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 related	[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
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;
as well as URLs for NNTP newsgroup(s).