linux-kernel - Re: New objtool warning..

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b1f96f39e78fc3869bf2786f50f833778dd09fb5.camel@perches.com>
Date:   Thu, 17 Dec 2020 10:25:29 -0800
From:   Joe Perches <joe@...ches.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Lyude Paul <lyude@...hat.com>,
        Ilia Mirkin <imirkin@...m.mit.edu>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: New objtool warning..

On Thu, 2020-12-17 at 09:27 -0800, Linus Torvalds wrote:
> On Thu, Dec 17, 2020 at 8:25 AM Josh Poimboeuf <jpoimboe@...hat.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++) {