Re: Unsigned widening casts of binary "not" operations..

* Re: Unsigned widening casts of binary "not" operations..
       [not found] <CA+55aFwpLu0qNms=hkQr43yqD0K9DgESNm91OEWKT1ZuT8MU6Q@mail.gmail.com>
@ 2013-04-23  0:23 ` Linus Torvalds
  2013-04-23  8:59   ` David Laight
  2013-04-23  0:32 ` H. Peter Anvin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 41+ messages in thread
From: Linus Torvalds @ 2013-04-23  0:23 UTC (permalink / raw)
  To: Ingo Molnar, H. Peter Anvin, Thomas Gleixner, David Miller,
	Theodore Ts'o
  Cc: Linux Kernel Mailing List, the arch/x86 maintainers,
	Network Development, linux-ext4

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

[ Ugh, resending because I had mistakenly set the html bit and all the
mailing lists just refused the original... ]

So I was playing with sparse again, because the MIPS people had hit a
bug with the fact that they had made PAGE_MASK an *unsigned* type, and
then doing the ~ (binary not) operation on it does the wrong thing
when you operate on bigger types, like hardware 36-bit physical
addresses.

See commit 3b5e50edaf50 (and commit c17a6554 that it reverts).

So the issue is that let's say that you have a constant (or variable,
for that matter) that is of type "unsigned int", and then you use that
to mask a variable of a larger size (say it's an "unsigned long" on a
64-bit arch, or it's a phys_addr_t on a 32-bit arch with PAE). So the
code looks basically like a variation of something like this:

     u64 value = ...;

     value &= ~bitmask;

What happens?

What happens is that the "~bitmask" is done in the *narrower* type,
and then - because the narrower type is unsigned - the cast to the
wider type is done as an *unsigned* cast, so what you *think* happens
is that it clears the bits that are set in "bitmask", but what
*actually* happens is that yes, you clear the bits that are set in
:bitmask", but you *also* clear the upper bits of value.

Now, why am I posting about this MIPS-specific small detail on to x86 people?

Because I hacked up a sparse patch that looks for the pattern of
unsigned casts of a (bit) negation to a wider type, and there's a fair
number of them.

Now, it may well be that my sparse hack (and it really is a hack) is a
broken piece of crap, but from a quick look the warnings it gives look
reasonable.

And there's quite a lot of them. Even in my (fairly small) config I
use on my desktop. And the first warnings I see are in x86 code:

     arch/x86/kernel/traps.c:405:16: warning: implicit unsigned
widening cast of a '~' expression
     arch/x86/kernel/cpu/perf_event_p4.c:912:15: warning: implicit
unsigned widening cast of a '~' expression

that particular one is something that has an *explicit* cast to "u64",
and then it does binary 'and' operations with these things that are of
a 32-bit unsigned type with a binary not in front of them. IOW, the
types in that expression are *very* confused.

Here's a ext4 code snippet that looks like an actual bug (but seems to
only hit read-ahead):

     ext4_fsblk_t b, block;

     b = block & ~(EXT4_SB(sb)->s_inode_readahead_blks-1);

where "b" actually ends up having the upper bits cleared, because the
s_inode_readahead_blks thing is an unsigned int, so you're masking off
not just the low bits, but the high bits too. Ted? Of course, it's
just read-ahead, so it probably doesn't matter, but.

We have a number of generic code examples where this kind of thing
seems harmless:

                 *vm_flags &= ~VM_MERGEABLE;

(we only have flags in the low 32 bits, and the only reason we get
warnings for VM_MERGEABLE is because 0x80000000 is an implicitly
unsigned constant, while 0x40000000 is *not*), or

     kernel/trace/trace.c:2910:32: warning: implicit unsigned widening
cast of a '~' expression

where "trace_flags" is "unsigned long" and "mask" is "unsigned int",
and the expression

     trace_flags &= ~mask;

actually clears the upper 32 bits too, but presumably they are always
clear anyway so we don't *really care. But it's an example of code
that is potentially very subtly dangerous.

Now, a lot of the other warnings I get seem to be more benign - the
networking code results in a lot of these because of code like this:

   #define NLMSG_ALIGNTO      4U
   #define NLMSG_ALIGN(len) ( ((len)+NLMSG_ALIGNTO-1) & ~(NLMSG_ALIGNTO-1) )

which technically has the same problem if "len" is 64-bit (which
sizeof() is on x86-64), but we don't really *care* because it turns
out that the sizeof values will always have the high bits clear
*anyway*. So I'm not sure the hacky sparse warning is useful, because
my code isn't smart enough to figure out when this kind of widening
cast is a problem, and when it isn't.

That said, I'm cc'ing David and netdev too, just in case. There is
likely some *reason* why it uses an "unsigned int" for a constant that
is then commonly expanded with a binary "not" and the upper bits end
up being surprising. So this thing doesn't look like a bug, but it
does cause these warnings:

     net/netlink/af_netlink.c:1889:38: warning: implicit unsigned
widening cast of a '~' expression

and maybe the networking people care about this and maybe they don't.

(It turns out that any use of "UINT_MAX" for a "long" value also
results in this warning, because we define it as "~0ul", so there are
other cases of spurious things where we *intentionally* drop the high
bits).

ANYWAY.

Only a few of the warnings looked like they might be bugs, but I'm
attaching my sparse patch here in case somebody wants to play with the
hacky thing..

                           Linus

[-- Attachment #2: sparse.diff --]
[-- Type: application/octet-stream, Size: 3804 bytes --]

 compile-i386.c | 10 ----------
 evaluate.c     | 30 +++++++++++++++++++++++++-----
 show-parse.c   |  9 ---------
 symbol.h       |  1 +
 4 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/compile-i386.c b/compile-i386.c
index b47095252053..31d3dbca1060 100644
--- a/compile-i386.c
+++ b/compile-i386.c
@@ -176,7 +176,6 @@ static const char *current_section;
 static void emit_comment(const char * fmt, ...) FORMAT_ATTR(1);
 static void emit_move(struct storage *src, struct storage *dest,
 		      struct symbol *ctype, const char *comment);
-static int type_is_signed(struct symbol *sym);
 static struct storage *x86_address_gen(struct expression *expr);
 static struct storage *x86_symbol_expr(struct symbol *sym);
 static void x86_symbol(struct symbol *sym);
@@ -2250,15 +2249,6 @@ static void x86_symbol_init(struct symbol *sym)
 	priv->addr = new;
 }
 
-static int type_is_signed(struct symbol *sym)
-{
-	if (sym->type == SYM_NODE)
-		sym = sym->ctype.base_type;
-	if (sym->type == SYM_PTR)
-		return 0;
-	return !(sym->ctype.modifiers & MOD_UNSIGNED);
-}
-
 static struct storage *x86_label_expr(struct expression *expr)
 {
 	struct storage *new = stack_alloc(4);
diff --git a/evaluate.c b/evaluate.c
index 99dedb5c5bde..5c49c9c92a69 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -31,6 +31,15 @@ struct symbol *current_fn;
 static struct symbol *degenerate(struct expression *expr);
 static struct symbol *evaluate_symbol(struct symbol *sym);
 
+int type_is_signed(struct symbol *sym)
+{
+	if (sym->type == SYM_NODE)
+		sym = sym->ctype.base_type;
+	if (sym->type == SYM_PTR)
+		return 0;
+	return !(sym->ctype.modifiers & MOD_UNSIGNED);
+}
+
 static struct symbol *evaluate_symbol_expression(struct expression *expr)
 {
 	struct expression *addr;
@@ -282,12 +291,18 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
 	 */
 	switch (old->type) {
 	case EXPR_PREOP:
-		if (old->ctype->bit_size < type->bit_size)
+		if (old->op == '~' || old->op == '-') {
+			if (old->ctype->bit_size > type->bit_size) {
+				old->ctype = type;
+				old->unop = cast_to(old->unop, type);
+				return old;
+			}
+			if (old->ctype->bit_size == type->bit_size)
+				break;
+			if (type_is_signed(old->ctype))
+				break;
+			warning(old->pos, "implicit unsigned widening cast of a '%c' expression", old->op);
 			break;
-		if (old->op == '~') {
-			old->ctype = type;
-			old->unop = cast_to(old->unop, type);
-			return old;
 		}
 		break;
 
@@ -310,6 +325,7 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
 		/* nothing */;
 	}
 
+
 	expr = alloc_expression(old->pos, EXPR_IMPLIED_CAST);
 	expr->flags = old->flags;
 	expr->ctype = type;
@@ -1757,6 +1773,10 @@ Normal:
 	}
 	if (expr->op == '+')
 		*expr = *expr->unop;
+	else if (expr->unop->type == EXPR_PREOP && expr->unop->op == expr->op) {
+		if (expr->unop->ctype == ctype)
+			*expr = *expr->unop->unop;
+	}
 	expr->ctype = ctype;
 	return ctype;
 Restr:
diff --git a/show-parse.c b/show-parse.c
index 1333e301c151..c8e864476fac 100644
--- a/show-parse.c
+++ b/show-parse.c
@@ -933,15 +933,6 @@ static int show_symbol_init(struct symbol *sym)
 	return 0;
 }
 
-static int type_is_signed(struct symbol *sym)
-{
-	if (sym->type == SYM_NODE)
-		sym = sym->ctype.base_type;
-	if (sym->type == SYM_PTR)
-		return 0;
-	return !(sym->ctype.modifiers & MOD_UNSIGNED);
-}
-
 static int show_cast_expr(struct expression *expr)
 {
 	struct symbol *old_type, *new_type;
diff --git a/symbol.h b/symbol.h
index 1e745799a8d0..6928faf4fec6 100644
--- a/symbol.h
+++ b/symbol.h
@@ -389,5 +389,6 @@ extern int is_ptr_type(struct symbol *);
 
 void create_fouled(struct symbol *type);
 struct symbol *befoul(struct symbol *type);
+int type_is_signed(struct symbol *sym);
 
 #endif /* SYMBOL_H */

^ permalink raw reply related	[flat|nested] 41+ messages in thread