New objtool warning..

New objtool warning..

[Linus Torvalds]

I only see this on my laptop, but that's probably because my desktop
is built using clang. So it's a gcc code generation interaction, I
suspect..

Anyway, the new warning is

    drivers/gpu/drm/drm_edid.o: warning: objtool: do_cvt_mode() falls
through to next function drm_mode_detailed.isra.0()

and googling around a bit I see that 0day ended up reporting it on the
linux-next lists, and blames commit 991fcb77f490 ("drm/edid: Fix
uninitialized variable in drm_cvt_modes()").

That presumably then makes gcc generate that odd code.

That "unreachable()" is because the compiler isn't smart enough to see
that yes, there really are case statements for every single possible
case. Oh well. Maybe the code should just make one of the possible
cases also be the "default:" case, and that might fix it.

But maybe this is worth looking into for objtool too?

Anyway, I see it with gcc-10.2.1 as per current F32. Holler if you
can't reproduce it, I can send the object file around.

            Linus

Re: New objtool warning..

[Josh Poimboeuf]

On Tue, Dec 15, 2020 at 08:22:23PM -0800, Linus Torvalds wrote:
> I only see this on my laptop, but that's probably because my desktop
> is built using clang. So it's a gcc code generation interaction, I
> suspect..
> 
> Anyway, the new warning is
> 
>     drivers/gpu/drm/drm_edid.o: warning: objtool: do_cvt_mode() falls
> through to next function drm_mode_detailed.isra.0()
> 
> and googling around a bit I see that 0day ended up reporting it on the
> linux-next lists, and blames commit 991fcb77f490 ("drm/edid: Fix
> uninitialized variable in drm_cvt_modes()").
> 
> That presumably then makes gcc generate that odd code.
> 
> That "unreachable()" is because the compiler isn't smart enough to see
> that yes, there really are case statements for every single possible
> case. Oh well. Maybe the code should just make one of the possible
> cases also be the "default:" case, and that might fix it.
> 
> But maybe this is worth looking into for objtool too?
> 
> Anyway, I see it with gcc-10.2.1 as per current F32. Holler if you
> can't reproduce it, I can send the object file around.

I can't recreate with my compiler, but I think I've seen one like this
before.  I suspect s/unreachable()/BUG()/ would fix it?

But yeah, it _might_ be possible to make objtool a little smarter here.
Gimme the .o file and I can take a look tomorrow.

-- 
Josh

Re: New objtool warning..

[Linus Torvalds]

On Tue, Dec 15, 2020 at 8:49 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> But yeah, it _might_ be possible to make objtool a little smarter here.
> Gimme the .o file and I can take a look tomorrow.

Hmm. I tried to send it to you, but then I get a bounce with

  554 Email rejected due to security policies

because apparently your redhat address doesn't allow object files.

Annoying.

I'll try sending it as a tar-file instead ;)

            Linus

RE: New objtool warning..

[David Laight]

From: Josh Poimboeuf
> Sent: 16 December 2020 04:49
> 
> On Tue, Dec 15, 2020 at 08:22:23PM -0800, Linus Torvalds wrote:
> > I only see this on my laptop, but that's probably because my desktop
> > is built using clang. So it's a gcc code generation interaction, I
> > suspect..
> >
> > Anyway, the new warning is
> >
> >     drivers/gpu/drm/drm_edid.o: warning: objtool: do_cvt_mode() falls
> > through to next function drm_mode_detailed.isra.0()
> >
> > and googling around a bit I see that 0day ended up reporting it on the
> > linux-next lists, and blames commit 991fcb77f490 ("drm/edid: Fix
> > uninitialized variable in drm_cvt_modes()").
> >
> > That presumably then makes gcc generate that odd code.
> >
> > That "unreachable()" is because the compiler isn't smart enough to see
> > that yes, there really are case statements for every single possible
> > case. Oh well. Maybe the code should just make one of the possible
> > cases also be the "default:" case, and that might fix it.
> >
> > But maybe this is worth looking into for objtool too?
> >
> > Anyway, I see it with gcc-10.2.1 as per current F32. Holler if you
> > can't reproduce it, I can send the object file around.
> 
> I can't recreate with my compiler, but I think I've seen one like this
> before.  I suspect s/unreachable()/BUG()/ would fix it?

Then a smart(er) compiler will report that the BUG() is unreachable.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: New objtool warning..

[Josh Poimboeuf]

On Tue, Dec 15, 2020 at 09:31:50PM -0800, Linus Torvalds wrote:
> On Tue, Dec 15, 2020 at 8:49 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >
> > But yeah, it _might_ be possible to make objtool a little smarter here.
> > Gimme the .o file and I can take a look tomorrow.
> 
> Hmm. I tried to send it to you, but then I get a bounce with
> 
>   554 Email rejected due to security policies
> 
> because apparently your redhat address doesn't allow object files.
> 
> Annoying.

Well that's embarrassing.  You're the second person to complain about
that in as many days...

/me goes open a ticket

-- 
Josh

Re: New objtool warning..

[Josh Poimboeuf]

On Wed, Dec 16, 2020 at 10:46:31AM +0000, David Laight wrote:
> From: Josh Poimboeuf
> > Sent: 16 December 2020 04:49
> > 
> > On Tue, Dec 15, 2020 at 08:22:23PM -0800, Linus Torvalds wrote:
> > > I only see this on my laptop, but that's probably because my desktop
> > > is built using clang. So it's a gcc code generation interaction, I
> > > suspect..
> > >
> > > Anyway, the new warning is
> > >
> > >     drivers/gpu/drm/drm_edid.o: warning: objtool: do_cvt_mode() falls
> > > through to next function drm_mode_detailed.isra.0()
> > >
> > > and googling around a bit I see that 0day ended up reporting it on the
> > > linux-next lists, and blames commit 991fcb77f490 ("drm/edid: Fix
> > > uninitialized variable in drm_cvt_modes()").
> > >
> > > That presumably then makes gcc generate that odd code.
> > >
> > > That "unreachable()" is because the compiler isn't smart enough to see
> > > that yes, there really are case statements for every single possible
> > > case. Oh well. Maybe the code should just make one of the possible
> > > cases also be the "default:" case, and that might fix it.
> > >
> > > But maybe this is worth looking into for objtool too?
> > >
> > > Anyway, I see it with gcc-10.2.1 as per current F32. Holler if you
> > > can't reproduce it, I can send the object file around.
> > 
> > I can't recreate with my compiler, but I think I've seen one like this
> > before.  I suspect s/unreachable()/BUG()/ would fix it?
> 
> Then a smart(er) compiler will report that the BUG() is unreachable.

It shouldn't, BUG() already has unreachable().

-- 
Josh

Re: New objtool warning..

[Josh Poimboeuf]

On Tue, Dec 15, 2020 at 09:32:29PM -0800, Linus Torvalds wrote:
> > The asm code looks like this:
> >
> >         cmpb    $4, %al #, _30
> >         jne     .L176   #,
> > ...
> >         cmpb    $12, %al        #, _30
> >         jne     .L176   #,
> > ...
> > .L176:
> > # drivers/gpu/drm/drm_edid.c:3118:                      unreachable();
> > #APP
> > # 3118 "drivers/gpu/drm/drm_edid.c" 1
> >         320:    #
> >         .pushsection .discard.unreachable
> >         .long 320b - .  #
> >         .popsection
> > # 0 "" 2
> > #NO_APP
> >    .. this falls through..
> >
> > So you *should* find that label that then falls through in that
> > ".discard.unreachable" section, and so it should be possible to teach
> > objdump about that (insane) unreachable code that way. No?

So this is kind of tricky, because the unreachable() annotation usually
means "the previous instruction is a dead end".  Most of the time, the
next instruction -- the one actually pointed to by the annotation -- is
actually reachable from another path.


For example, here's a typical usage of unreachable():

	je not_a_bug
	ud2

	.pushsection .discard.unreachable
	.long not_a_bug - .
	.popsection

not_a_bug:
	... normal non-buggy code ...

The 'not_a_bug' label is pointed to by the unreachable annotation, but
it's actually reachable.


In your .o, .discard.unreachable points to 0xbb3, so objtool marks the
previous instruction (0xbae) as a dead end:

     bae:       e9 30 ff ff ff          jmpq   ae3 <do_cvt_mode+0xd3>
     bb3:       66 66 2e 0f 1f 84 00    data16 nopw %cs:0x0(%rax,%rax,1)
     bba:       00 00 00 00
     bbe:       66 90                   xchg   %ax,%ax

And there's another path to 0xbb3 from the switch statement, so objtool
assumes it's reachable.

So maybe we need to make objtool's unreachable logic a little more
nuanced: If the previous instruction is an unconditional jump, then
consider *the annotated instruction itself* to be a dead end.

I'm not quite able to convince myself this wouldn't produce false
positives.  It did immediately produce one false positive in
no_context(), but that should be easily fixable (see patch).

I can run it through more testing, if you don't see any obvious problems
with it.


diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index f1f1b5a0956a..c888821bb40c 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -699,7 +699,6 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 		 */
 		asm volatile ("movq %[stack], %%rsp\n\t"
 			      "call handle_stack_overflow\n\t"
-			      "1: jmp 1b"
 			      : ASM_CALL_CONSTRAINT
 			      : "D" ("kernel stack overflow (page fault)"),
 				"S" (regs), "d" (address),
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c6ab44543c92..267e8b88ca3a 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -370,9 +370,12 @@ static int add_dead_ends(struct objtool_file *file)
 			return -1;
 		}
 		insn = find_insn(file, reloc->sym->sec, reloc->addend);
-		if (insn)
-			insn = list_prev_entry(insn, list);
-		else if (reloc->addend == reloc->sym->sec->len) {
+		if (insn) {
+			struct instruction *prev = list_prev_entry(insn, list);
+			if (prev->type != INSN_JUMP_UNCONDITIONAL)
+				insn = prev;
+
+		} else if (reloc->addend == reloc->sym->sec->len) {
 			insn = find_last_insn(file, reloc->sym->sec);
 			if (!insn) {
 				WARN("can't find unreachable insn at %s+0x%x",

Re: New objtool warning..

[Peter Zijlstra]

On Wed, Dec 16, 2020 at 02:01:58PM -0600, Josh Poimboeuf wrote:
> So this is kind of tricky, because the unreachable() annotation usually
> means "the previous instruction is a dead end".  Most of the time, the
> next instruction -- the one actually pointed to by the annotation -- is
> actually reachable from another path.

*groan*, this is why I ended up sticking a nop in
instrumentation_begin()/_end().

Re: New objtool warning..

[Josh Poimboeuf]

On Thu, Dec 17, 2020 at 11:45:56AM +0100, Peter Zijlstra wrote:
> On Wed, Dec 16, 2020 at 02:01:58PM -0600, Josh Poimboeuf wrote:
> > So this is kind of tricky, because the unreachable() annotation usually
> > means "the previous instruction is a dead end".  Most of the time, the
> > next instruction -- the one actually pointed to by the annotation -- is
> > actually reachable from another path.
> 
> *groan*, this is why I ended up sticking a nop in
> instrumentation_begin()/_end().

Oh yeah, I forgot about that.  That would be another option if my patch
doesn't work out.

/me grumbles something about integrating objtool with compiler plugins
someday...

-- 
Josh

Re: New objtool warning..

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

On Thu, Dec 17, 2020 at 8:25 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> Oh yeah, I forgot about that.  That would be another option if my patch
> doesn't work out.

Well, one option is to just say "ok, we know gcc generates horrible
code that falls through to another function in a situation that we
claim is unreachable, so let's not claim it is unreachable".

IOW, the problem here is that the compiler fundamentally isn't smart
enough to see that something is unreachable, and the "unreachable()"
annotation we did didn't actually really cause any code that makes it
so. So we basically have code that _if_ we ever change it, it will
simply be wrong, and we'll never see any warnings about it but it will
fall through to nonsensical code.

So maybe the option here is simply "objtool was right before, the
unreachable() is fragile and wrong".

We can easily write that case statement in a way that actually makes
the compiler generate better code and avoids the issue by just making
case 0x00 also be the default case.

So I think I'll just apply this patch instead.

            Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 791 bytes --]

 drivers/gpu/drm/drm_edid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 74f5a3197214..e95cce8e736d 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3102,6 +3102,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
 
 		height = (cvt->code[0] + ((cvt->code[1] & 0xf0) << 4) + 1) * 2;
 		switch (cvt->code[1] & 0x0c) {
+		/* default - because compiler doesn't see that we've enumerated all cases */
+		default:
 		case 0x00:
 			width = height * 4 / 3;
 			break;
@@ -3114,8 +3116,6 @@ static int drm_cvt_modes(struct drm_connector *connector,
 		case 0x0c:
 			width = height * 15 / 9;
 			break;
-		default:
-			unreachable();
 		}
 
 		for (j = 1; j < 5; j++) {

Re: New objtool warning..

[Linus Torvalds]

On Thu, Dec 17, 2020 at 9:27 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I think I'll just apply this patch instead.

Commit d652d5f1eeeb ("drm/edid: fix objtool warning in
drm_cvt_modes()") has the long and boring explanation.

          Linus

Re: New objtool warning..

[Joe Perches]

On Thu, 2020-12-17 at 09:27 -0800, Linus Torvalds wrote:
> On Thu, Dec 17, 2020 at 8:25 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > Oh yeah, I forgot about that.  That would be another option if my patch
> > doesn't work out.
> 
> Well, one option is to just say "ok, we know gcc generates horrible
> code that falls through to another function in a situation that we
> claim is unreachable, so let's not claim it is unreachable".
> 
> IOW, the problem here is that the compiler fundamentally isn't smart
> enough to see that something is unreachable, and the "unreachable()"
> annotation we did didn't actually really cause any code that makes it
> so. So we basically have code that _if_ we ever change it, it will
> simply be wrong, and we'll never see any warnings about it but it will
> fall through to nonsensical code.
> 
> So maybe the option here is simply "objtool was right before, the
> unreachable() is fragile and wrong".
> 
> We can easily write that case statement in a way that actually makes
> the compiler generate better code and avoids the issue by just making
> case 0x00 also be the default case.
> 
> So I think I'll just apply this patch instead.

Using default somewhere other than the bottom of a switch/case block
isn't common/pretty.  It's easy to visually skip/glaze over.

Perhaps reorder the block.
Maybe add static to the const arrays too.
---
 drivers/gpu/drm/drm_edid.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 74f5a3197214..53b7bb281edb 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3089,8 +3089,8 @@ static int drm_cvt_modes(struct drm_connector *connector,
 	struct drm_display_mode *newmode;
 	struct drm_device *dev = connector->dev;
 	struct cvt_timing *cvt;
-	const int rates[] = { 60, 85, 75, 60, 50 };
-	const u8 empty[3] = { 0, 0, 0 };
+	static const int rates[] = { 60, 85, 75, 60, 50 };
+	static const u8 empty[3] = { 0, 0, 0 };
 
 	for (i = 0; i < 4; i++) {
 		int width, height;
@@ -3102,20 +3102,18 @@ static int drm_cvt_modes(struct drm_connector *connector,
 
 		height = (cvt->code[0] + ((cvt->code[1] & 0xf0) << 4) + 1) * 2;
 		switch (cvt->code[1] & 0x0c) {
-		case 0x00:
-			width = height * 4 / 3;
-			break;
-		case 0x04:
-			width = height * 16 / 9;
+		case 0x0c:
+			width = height * 15 / 9;
 			break;
 		case 0x08:
 			width = height * 16 / 10;
 			break;
-		case 0x0c:
-			width = height * 15 / 9;
+		case 0x04:
+			width = height * 16 / 9;
 			break;
 		default:
-			unreachable();
+			width = height * 4 / 3;
+			break;
 		}
 
 		for (j = 1; j < 5; j++) {