[PATCH] bogus cast in bio.c

[PATCH] bogus cast in bio.c

[viro]

<qualifier> void * is not the same as void <qualifier> *...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
----
diff -urN RC13-git8-base/fs/bio.c current/fs/bio.c
--- RC13-git8-base/fs/bio.c	2005-09-08 10:17:39.000000000 -0400
+++ current/fs/bio.c	2005-09-08 23:53:33.000000000 -0400
@@ -683,7 +683,7 @@
 {
 	struct sg_iovec iov;
 
-	iov.iov_base = (__user void *)uaddr;
+	iov.iov_base = (void __user *)uaddr;
 	iov.iov_len = len;
 
 	return bio_map_user_iov(q, bdev, &iov, 1, write_to_vm);

Re: [PATCH] bogus cast in bio.c

[Andreas Schwab]

viro@ZenIV.linux.org.uk writes:

> <qualifier> void * is not the same as void <qualifier> *...

IMHO it should.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] bogus cast in bio.c

[viro]

On Fri, Sep 09, 2005 at 06:18:14PM +0200, Andreas Schwab wrote:
> viro@ZenIV.linux.org.uk writes:
> 
> > <qualifier> void * is not the same as void <qualifier> *...
> 
> IMHO it should.

... and indeed it should.  My apologies - that's a combination of
sparse bug and me being very low on coffee...

FWIW, looks like we are losing address_space in the first form: with current
sparse we get
fs/bio.c:686:15: warning: incorrect type in assignment (different address spaces)
fs/bio.c:686:15:    expected void [noderef] *iov_base<asn:1>
fs/bio.c:686:15:    got void [noderef] *<noident>
from the first form (cast to __user void *).  Lovely...

OK, I think I know what's going on there, will fix.

[sparse fix] (was Re: [PATCH] bogus cast in bio.c)

[viro]

> fs/bio.c:686:15: warning: incorrect type in assignment (different address spaces)
> fs/bio.c:686:15:    expected void [noderef] *iov_base<asn:1>
> fs/bio.c:686:15:    got void [noderef] *<noident>
> from the first form (cast to __user void *).  Lovely...
> 
> OK, I think I know what's going on there, will fix.

What happens is actually pretty simple - we get address_space(1) handled
in declaration_specifiers(), which sets ctype->as to 1.  Then we see
"void" and eventually get to
                        ctype->base_type = type;
                }

                check_modifiers(&token->pos, s, ctype->modifiers);
                apply_ctype(token->pos, &thistype, ctype);
with thistype coming from lookup for "void".  And that, of course, has
zero ->as.  Now apply_ctype merrily buggers ctype->as and we have 0...

So AFAICS proper fix for sparse should be to check thistype->as to see
if it really has any intention to change ->as.  ACK?

--- parse.c	2005-09-07 17:03:13.000000000 -0400
+++ parse.c.new	2005-09-09 13:25:01.000000000 -0400
@@ -636,7 +636,8 @@
 		ctype->alignment = thistype->alignment;
 
 	/* Address space */
-	ctype->as = thistype->as;
+	if (thistype->as)
+		ctype->as = thistype->as;
 }
 
 static void check_modifiers(struct position *pos, struct symbol *s, unsigned long mod)

Re: [sparse fix] (was Re: [PATCH] bogus cast in bio.c)

[viro]

On Fri, Sep 09, 2005 at 06:29:38PM +0100, viro@ZenIV.linux.org.uk wrote:
> > fs/bio.c:686:15: warning: incorrect type in assignment (different address spaces)
> > fs/bio.c:686:15:    expected void [noderef] *iov_base<asn:1>
> > fs/bio.c:686:15:    got void [noderef] *<noident>
> > from the first form (cast to __user void *).  Lovely...
> > 
> > OK, I think I know what's going on there, will fix.
> 
> What happens is actually pretty simple - we get address_space(1) handled
> in declaration_specifiers(), which sets ctype->as to 1.  Then we see
> "void" and eventually get to
>                         ctype->base_type = type;
>                 }
> 
>                 check_modifiers(&token->pos, s, ctype->modifiers);
>                 apply_ctype(token->pos, &thistype, ctype);
> with thistype coming from lookup for "void".  And that, of course, has
> zero ->as.  Now apply_ctype merrily buggers ctype->as and we have 0...
> 
> So AFAICS proper fix for sparse should be to check thistype->as to see
> if it really has any intention to change ->as.  ACK?

PS: obvious testcase for that one:

#define X __attribute__((address_space(1)))    
void X *p;
void X *q;
void foo(unsigned long n)
{
        p = (void X *)n;
        q = (X void *)n;
}

Re: [sparse fix] (was Re: [PATCH] bogus cast in bio.c)

[Linus Torvalds]

On Fri, 9 Sep 2005 viro@ZenIV.linux.org.uk wrote:
> 
> So AFAICS proper fix for sparse should be to check thistype->as to see
> if it really has any intention to change ->as.  ACK?

Ack. Applied,

		Linus

Re: [sparse fix] (was Re: [PATCH] bogus cast in bio.c)

[Linus Torvalds]

On Fri, 9 Sep 2005, Linus Torvalds wrote:
> 
> Ack. Applied,

Btw, I also just committed the fix to not warn for

	#if defined(TOKEN) && TOKEN > 1

when TOKEN is undefined and -Wundef is enabled. Implemented exactly the
way you suggested.

		Linus

Re: [sparse fix] (was Re: [PATCH] bogus cast in bio.c)

[viro]

On Fri, Sep 09, 2005 at 11:15:41AM -0700, Linus Torvalds wrote:
> 
> 
> On Fri, 9 Sep 2005, Linus Torvalds wrote:
> > 
> > Ack. Applied,
> 
> Btw, I also just committed the fix to not warn for
> 
> 	#if defined(TOKEN) && TOKEN > 1
> 
> when TOKEN is undefined and -Wundef is enabled. Implemented exactly the
> way you suggested.

Cool...  Speaking of sparse patches:

* generates a warning when we cast _between_ address spaces (e.g. cast from
__user to __iomem).
* optional (on -Wcast-to-as) warning when casting _TO_ address space (e.g.
when normal pointer is cast to __iomem one - that caught a lot of crap in
drivers).  casts from unsigned long are still OK, so's cast from 0, so's
__force cast, of course.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
----
diff --git a/evaluate.c b/evaluate.c
--- a/evaluate.c
+++ b/evaluate.c
@@ -2049,11 +2049,27 @@ static int get_as(struct symbol *sym)
 	return as;
 }
 
+static void cast_to_as(struct expression *e, int as)
+{
+	struct expression *v = e->cast_expression;
+
+	if (!Wcast_to_address_space)
+		return;
+
+	/* cast from constant 0 to pointer is OK */
+	if (v->type == EXPR_VALUE && is_int_type(v->ctype) && !v->value)
+		return;
+
+	warning(e->pos, "cast adds address space to expression (<asn:%d>)", as);
+}
+
 static struct symbol *evaluate_cast(struct expression *expr)
 {
 	struct expression *target = expr->cast_expression;
 	struct symbol *ctype = examine_symbol_type(expr->cast_type);
-	enum type type;
+	struct symbol *t1, *t2;
+	enum type type1, type2;
+	int as1, as2;
 
 	if (!target)
 		return NULL;
@@ -2092,51 +2108,54 @@ static struct symbol *evaluate_cast(stru
 	evaluate_expression(target);
 	degenerate(target);
 
+	t1 = ctype;
+	if (t1->type == SYM_NODE)
+		t1 = t1->ctype.base_type;
+	if (t1->type == SYM_ENUM)
+		t1 = t1->ctype.base_type;
+
 	/*
 	 * You can always throw a value away by casting to
 	 * "void" - that's an implicit "force". Note that
 	 * the same is _not_ true of "void *".
 	 */
-	if (ctype == &void_ctype)
+	if (t1 == &void_ctype)
 		goto out;
 
-	type = ctype->type;
-	if (type == SYM_NODE) {
-		type = ctype->ctype.base_type->type;
-		if (ctype->ctype.base_type == &void_ctype)
-			goto out;
-	}
-	if (type == SYM_ARRAY || type == SYM_UNION || type == SYM_STRUCT)
+	type1 = t1->type;
+	if (type1 == SYM_ARRAY || type1 == SYM_UNION || type1 == SYM_STRUCT)
 		warning(expr->pos, "cast to non-scalar");
 
-	if (!target->ctype) {
+	t2 = target->ctype;
+	if (!t2) {
 		warning(expr->pos, "cast from unknown type");
 		goto out;
 	}
+	if (t2->type == SYM_NODE)
+		t2 = t2->ctype.base_type;
+	if (t2->type == SYM_ENUM)
+		t2 = t2->ctype.base_type;
 
-	type = target->ctype->type;
-	if (type == SYM_NODE)
-		type = target->ctype->ctype.base_type->type;
-	if (type == SYM_ARRAY || type == SYM_UNION || type == SYM_STRUCT)
+	type2 = t2->type;
+	if (type2 == SYM_ARRAY || type2 == SYM_UNION || type2 == SYM_STRUCT)
 		warning(expr->pos, "cast from non-scalar");
 
-	if (!get_as(ctype) && get_as(target->ctype) > 0)
-		warning(expr->pos, "cast removes address space of expression");
-
-	if (!(ctype->ctype.modifiers & MOD_FORCE)) {
-		struct symbol *t1 = ctype, *t2 = target->ctype;
-		if (t1->type == SYM_NODE)
-			t1 = t1->ctype.base_type;
-		if (t2->type == SYM_NODE)
-			t2 = t2->ctype.base_type;
-		if (t1 != t2) {
-			if (t1->type == SYM_RESTRICT)
-				warning(expr->pos, "cast to restricted type");
-			if (t2->type == SYM_RESTRICT)
-				warning(expr->pos, "cast from restricted type");
-		}
+	if (!(ctype->ctype.modifiers & MOD_FORCE) && t1 != t2) {
+		if (t1->type == SYM_RESTRICT)
+			warning(expr->pos, "cast to restricted type");
+		if (t2->type == SYM_RESTRICT)
+			warning(expr->pos, "cast from restricted type");
 	}
 
+	as1 = get_as(ctype);
+	as2 = get_as(target->ctype);
+	if (!as1 && as2 > 0)
+		warning(expr->pos, "cast removes address space of expression");
+	if (as1 > 0 && as2 > 0 && as1 != as2)
+		warning(expr->pos, "cast between address spaces (<asn:%d>-><asn:%d>)", as2, as1);
+	if (as1 > 0 && !as2)
+		cast_to_as(expr, as1);
+
 	/*
 	 * Casts of constant values are special: they
 	 * can be NULL, and thus need to be simplified
diff --git a/lib.c b/lib.c
--- a/lib.c
+++ b/lib.c
@@ -170,6 +170,7 @@ int Wtypesign = 0;
 int Wcontext = 0;
 int Wundefined_preprocessor = 0;
 int Wptr_subtraction_blows = 0;
+int Wcast_to_address_space = 0;
 int Wtransparent_union = 1;
 int preprocess_only;
 char *include;
@@ -295,6 +296,7 @@ static const struct warning {
 	const char *name;
 	int *flag;
 } warnings[] = {
+	{ "cast-to-as", &Wcast_to_address_space },
 	{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
 	{ "default-bitfield-sign", &Wdefault_bitfield_sign },
 	{ "undef", &Wundefined_preprocessor },
diff --git a/lib.h b/lib.h
--- a/lib.h
+++ b/lib.h
@@ -79,6 +79,7 @@ extern int Wdefault_bitfield_sign;
 extern int Wundefined_preprocessor;
 extern int Wbitwise, Wtypesign, Wcontext;
 extern int Wtransparent_union;
+extern int Wcast_to_address_space;
 
 extern void declare_builtin_functions(void);
 extern void create_builtin_stream(void);