linux-next: build warning after merge of the tip tree

linux-next: build warning after merge of the tip tree

[Stephen Rothwell]

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

Hi all,

After merging the tip tree, today's linux-next build (x86_64 allmodconfig)
produced these warnings:

In file included from arch/x86/vdso/vdso2c.c:161:0:
arch/x86/vdso/vdso2c.c: In function 'main':
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
In file included from arch/x86/vdso/vdso2c.c:165:0:
arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]

Probably introduced by commit e6577a7ce99a ("x86, vdso: Move the vvar
area before the vdso text").

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH] x86, vdso: Fix vdso2c's special_pages error checking

[Andy Lutomirski]

Stephen Rothwell's compiler did something amazing: it unrolled a
loop, discovered that one iteration of that loop contained an
always-true test, and emitted a warning that will IMO only serve to
convince people to disable the warning.

That bogus warning caused me to wonder what prompted such an
absurdity from his compiler, and I discovered that the code in
question was, in fact, completely wrong -- I was looking things up
in the wrong array.

This affects 3.16 as well, but the only effect is to screw up the
error checking a bit.  vdso2c's output is unaffected.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 arch/x86/vdso/vdso2c.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/vdso/vdso2c.h b/arch/x86/vdso/vdso2c.h
index fd57829..0224987 100644
--- a/arch/x86/vdso/vdso2c.h
+++ b/arch/x86/vdso/vdso2c.h
@@ -109,16 +109,18 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 
 	/* Validate mapping addresses. */
 	for (i = 0; i < sizeof(special_pages) / sizeof(special_pages[0]); i++) {
-		if (!syms[i])
+		INT_BITS symval = syms[special_pages[i]];
+
+		if (!symval)
 			continue;  /* The mapping isn't used; ignore it. */
 
-		if (syms[i] % 4096)
+		if (symval % 4096)
 			fail("%s must be a multiple of 4096\n",
 			     required_syms[i].name);
-		if (syms[sym_vvar_start] > syms[i] + 4096)
-			fail("%s underruns begin_vvar\n",
+		if (symval + 4096 < syms[sym_vvar_start])
+			fail("%s underruns vvar_start\n",
 			     required_syms[i].name);
-		if (syms[i] + 4096 > 0)
+		if (symval + 4096 > 0)
 			fail("%s is on the wrong side of the vdso text\n",
 			     required_syms[i].name);
 	}
-- 
1.9.3

Re: linux-next: build warning after merge of the tip tree

[H. Peter Anvin]

On 07/17/2014 10:00 PM, Stephen Rothwell wrote:
> Hi all,
> 
> After merging the tip tree, today's linux-next build (x86_64
> allmodconfig) produced these warnings:
> 
> In file included from arch/x86/vdso/vdso2c.c:161:0: 
> arch/x86/vdso/vdso2c.c: In function 'main': 
> arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow
> does not occur when assuming that (X + c) < X is always false
> [-Wstrict-overflow] In file included from
> arch/x86/vdso/vdso2c.c:165:0: arch/x86/vdso/vdso2c.h:118:6:
> warning: assuming signed overflow does not occur when assuming that
> (X + c) < X is always false [-Wstrict-overflow]
> 
> Probably introduced by commit e6577a7ce99a ("x86, vdso: Move the
> vvar area before the vdso text").
> 

This seems toxic.

I always wonder if we shouldn't use -fwrapv for the kernel...

	-hpa

Re: linux-next: build warning after merge of the tip tree

[Andy Lutomirski]

On Fri, Jul 18, 2014 at 12:16 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/17/2014 10:00 PM, Stephen Rothwell wrote:
>> Hi all,
>>
>> After merging the tip tree, today's linux-next build (x86_64
>> allmodconfig) produced these warnings:
>>
>> In file included from arch/x86/vdso/vdso2c.c:161:0:
>> arch/x86/vdso/vdso2c.c: In function 'main':
>> arch/x86/vdso/vdso2c.h:118:6: warning: assuming signed overflow
>> does not occur when assuming that (X + c) < X is always false
>> [-Wstrict-overflow] In file included from
>> arch/x86/vdso/vdso2c.c:165:0: arch/x86/vdso/vdso2c.h:118:6:
>> warning: assuming signed overflow does not occur when assuming that
>> (X + c) < X is always false [-Wstrict-overflow]
>>
>> Probably introduced by commit e6577a7ce99a ("x86, vdso: Move the
>> vvar area before the vdso text").
>>
>
> This seems toxic.
>
> I always wonder if we shouldn't use -fwrapv for the kernel...

This particular warning is IMO in a particularly dumb category: GCC
optimizes some code and then warns about a construct that wasn't there
in the original code.  In this case, I think it unrolled a loop and
discovered that one iteration contained a test that was always true.
Big deal.

(OTOH, the code in question was buggy, but not all for the reason that
GCC thought it was.)

--Andy

>
>         -hpa
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: linux-next: build warning after merge of the tip tree

[H. Peter Anvin]

On 07/18/2014 12:57 PM, Andy Lutomirski wrote:
> 
> This particular warning is IMO in a particularly dumb category: GCC
> optimizes some code and then warns about a construct that wasn't there
> in the original code.  In this case, I think it unrolled a loop and
> discovered that one iteration contained a test that was always true.
> Big deal.
> 
> (OTOH, the code in question was buggy, but not all for the reason that
> GCC thought it was.)
> 

		if (syms[sym_vvar_start] > syms[i] + 4096)
			fail("%s underruns begin_vvar\n",
			     required_syms[i].name);

if i == sym_vvar_start then this is at least a valid warning.  It could
easily be quieted by chaning syms[] to an unsigned array.

	-hpa

Re: linux-next: build warning after merge of the tip tree

[Andy Lutomirski]

On Fri, Jul 18, 2014 at 1:05 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/18/2014 12:57 PM, Andy Lutomirski wrote:
>>
>> This particular warning is IMO in a particularly dumb category: GCC
>> optimizes some code and then warns about a construct that wasn't there
>> in the original code.  In this case, I think it unrolled a loop and
>> discovered that one iteration contained a test that was always true.
>> Big deal.
>>
>> (OTOH, the code in question was buggy, but not all for the reason that
>> GCC thought it was.)
>>
>
>                 if (syms[sym_vvar_start] > syms[i] + 4096)
>                         fail("%s underruns begin_vvar\n",
>                              required_syms[i].name);
>
> if i == sym_vvar_start then this is at least a valid warning.  It could
> easily be quieted by chaning syms[] to an unsigned array.

Hah -- fooled you, too :)

i isn't an index in to the syms array at all.  This code is completely
wrong.  See the patch I sent in reply to Stephen's original email.

But, to your earlier point, presumably this could warn:

for (int i = 0; i < 10; i++)
  if (array[i] > array[5] + 1)
    fail();

I think that's absurd.  There's nothing wrong with that code.  A given
test should have to be always true or always false on *all* loop
iterations to be flagged, I think.

--Andy

>
>         -hpa
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: linux-next: build warning after merge of the tip tree

[H. Peter Anvin]

On 07/18/2014 01:08 PM, Andy Lutomirski wrote:
> 
> i isn't an index in to the syms array at all.  This code is completely
> wrong.  See the patch I sent in reply to Stephen's original email.
> 
> But, to your earlier point, presumably this could warn:
> 
> for (int i = 0; i < 10; i++)
>   if (array[i] > array[5] + 1)
>     fail();
> 
> I think that's absurd.  There's nothing wrong with that code.  A given
> test should have to be always true or always false on *all* loop
> iterations to be flagged, I think.
> 

No, the issue is that gcc is telling you that the code will do the wrong
thing in this case.  Yes, only for one iteration, but still.

The reason this is a concern is that: (x > x + n) and its variants is
often used to mean (x > INT_MAX - n) without the type knowledge, but
that is actually invalid standard C because signed types are not
guaranteed to wrap.

	-hpa

Re: linux-next: build warning after merge of the tip tree

[Andy Lutomirski]

On Fri, Jul 18, 2014 at 1:15 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/18/2014 01:08 PM, Andy Lutomirski wrote:
>>
>> i isn't an index in to the syms array at all.  This code is completely
>> wrong.  See the patch I sent in reply to Stephen's original email.
>>
>> But, to your earlier point, presumably this could warn:
>>
>> for (int i = 0; i < 10; i++)
>>   if (array[i] > array[5] + 1)
>>     fail();
>>
>> I think that's absurd.  There's nothing wrong with that code.  A given
>> test should have to be always true or always false on *all* loop
>> iterations to be flagged, I think.
>>
>
> No, the issue is that gcc is telling you that the code will do the wrong
> thing in this case.  Yes, only for one iteration, but still.
>
> The reason this is a concern is that: (x > x + n) and its variants is
> often used to mean (x > INT_MAX - n) without the type knowledge, but
> that is actually invalid standard C because signed types are not
> guaranteed to wrap.

Right, but the constant in this case is *much* less than INT_MAX.
Anyway, this is moot.

I do wonder whether the kind of people who build hardened kernels
should enable -fwrapv, though.

--Andy

>
>         -hpa
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: linux-next: build warning after merge of the tip tree

[H. Peter Anvin]

On 07/18/2014 01:20 PM, Andy Lutomirski wrote:
>>
>> The reason this is a concern is that: (x > x + n) and its variants is
>> often used to mean (x > INT_MAX - n) without the type knowledge, but
>> that is actually invalid standard C because signed types are not
>> guaranteed to wrap.
> 
> Right, but the constant in this case is *much* less than INT_MAX.
> Anyway, this is moot.

It isn't about the constant (n) at all, it is about the value of x.

> I do wonder whether the kind of people who build hardened kernels
> should enable -fwrapv, though.

-fwrapv in gcc makes signed arithmetic strict 2's-complement, which is
what I think we want in the kernel.  Someone would just have to make
sure there isn't some key codepath in the kernel which gets pessimized.

	-hpa