[uml-devel] Real fix for rejected patch uaccess-warning

[uml-devel] Real fix for rejected patch uaccess-warning

[Blaisorblade]

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

About
http://user-mode-linux.sourceforge.net/work/current/2.6/2.6.16-rc4/patches/uaccess-warning

I read the patch which was rejected and noticed that you put there style 
changes to get_user. They should be kept while the rest of the patch thrown 
away, obviously, as discussed, but reading that made me wonder at this 
(changes are just whitespace fixups):

-        const __typeof__(ptr) __private_ptr = ptr; \
-        __typeof__(*(__private_ptr)) __private_val; \
[...]
-	if (__copy_from_user(&__private_val, (__private_ptr), \
-	    sizeof(*(__private_ptr))) == 0) {\

Given that __private_ptr is a const pointer, __private_val is a const value, 
where we store something with __copy_from_user.
That's likely why GCC complains.

To fix that, we should remove some excess constness. Try the attached patch (I 
haven't GCC 4.1 installed).

Also, I wonder why all this complicate and extra type-checked calls are 
needed.

The i386 source use typeof only in 1 case, for put_user:
when I put_user((short)b, (unsigned long *) c) I must first cast (i.e. 
zero-extend) b to a long and only after write it to c; writing the 4/8 bytes 
pointed to by &b would be meaningless.

All this IMHO, obviously.
-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

[-- Attachment #2: uml-fix-const-warns-gcc-4_1 --]
[-- Type: text/x-diff, Size: 874 bytes --]

Index: linux-2.6.git/include/asm-um/uaccess.h
===================================================================
--- linux-2.6.git.orig/include/asm-um/uaccess.h
+++ linux-2.6.git/include/asm-um/uaccess.h
@@ -42,7 +42,7 @@
 #define __get_user(x, ptr) \
 ({ \
         const __typeof__(ptr) __private_ptr = ptr; \
-        __typeof__(*(__private_ptr)) __private_val; \
+        __typeof__(*(ptr)) __private_val; \
         int __private_ret = -EFAULT; \
         (x) = (__typeof__(*(__private_ptr)))0; \
 	if (__copy_from_user(&__private_val, (__private_ptr), \
@@ -55,7 +55,7 @@
 
 #define get_user(x, ptr) \
 ({ \
-        const __typeof__((*(ptr))) __user *private_ptr = (ptr); \
+        __typeof__((*(ptr))) __user *private_ptr = (ptr); \
         (access_ok(VERIFY_READ, private_ptr, sizeof(*private_ptr)) ? \
 	 __get_user(x, private_ptr) : ((x) = 0, -EFAULT)); \
 })

Re: [uml-devel] Real fix for rejected patch uaccess-warning

[Jeff Dike]

On Tue, Mar 07, 2006 at 08:31:15PM +0100, Blaisorblade wrote:
> To fix that, we should remove some excess constness. Try the attached patch

That's certainly a lot simpler than what I had.  I'll drop it in my tree
and see what it does.

				Jeff


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Real fix for rejected patch uaccess-warning

[Blaisorblade]

On Thursday 09 March 2006 17:57, Jeff Dike wrote:
> On Tue, Mar 07, 2006 at 08:31:15PM +0100, Blaisorblade wrote:
> > To fix that, we should remove some excess constness. Try the attached
> > patch

> That's certainly a lot simpler than what I had.  I'll drop it in my tree
> and see what it does.

I've now seen that while that may fix the problem on 4.1 (and please, do test 
that), on my 3.4 it causes new warnings like:

fs/proc/proc_misc.c: In function `write_sysrq_trigger':
fs/proc/proc_misc.c:686: warning: initialization discards qualifiers from 
pointer target type

686:		if (get_user(c, buf))
where "buf" is a "const char __user *".

It's complaining because "const" was lost.

Ultimately, I think we should remove all this copying of pointers and do like 
i386 in this regard - there is only one cast to do, in put_user, as I 
mentioned in last mail.
-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Real fix for rejected patch uaccess-warning

[Jeff Dike]

On Fri, Mar 10, 2006 at 04:22:41PM +0100, Blaisorblade wrote:
> 686:		if (get_user(c, buf))
> where "buf" is a "const char __user *".
> 
> It's complaining because "const" was lost.
> 
> Ultimately, I think we should remove all this copying of pointers and do like
> i386 in this regard - there is only one cast to do, in put_user, as I 
> mentioned in last mail.

OK, my first patch was insane, try this one:

Index: linux-2.6.15/include/asm-um/uaccess.h
===================================================================
--- linux-2.6.15.orig/include/asm-um/uaccess.h	2006-03-23 15:41:15.000000000 -0500
+++ linux-2.6.15/include/asm-um/uaccess.h	2006-03-23 15:48:52.000000000 -0500
@@ -42,10 +42,10 @@
 #define __get_user(x, ptr) \
 ({ \
         const __typeof__(ptr) __private_ptr = ptr; \
-        __typeof__(*(__private_ptr)) __private_val; \
+        __typeof__(x) __private_val; \
         int __private_ret = -EFAULT; \
         (x) = (__typeof__(*(__private_ptr)))0; \
-	if (__copy_from_user(&__private_val, (__private_ptr), \
+	if (__copy_from_user((void *) &__private_val, (__private_ptr),	\
 	    sizeof(*(__private_ptr))) == 0) {\
         	(x) = (__typeof__(*(__private_ptr))) __private_val; \
 		__private_ret = 0; \

The (void *) fixes the warnings all by itself, but I changed the
typeof because that's more correct and fixes some of the warnings by
itself.

What it can't fix is things like (from include/linux/pagemap.h):

	volatile char c;
	ret = __get_user(c, uaddr);

We unavoidably lose the volatile because we can't know it was there,
so the (void *) cast seems necessary in this case.

Looking at this some more, it seems like most of the complexity could
go away.  I think I was avoiding any assignment to the target variable
until I knew that the copy_from_user had succeeded.  I think that was
being too paranoid, and just copy_from_user straight into x should be
fine.

				Jeff


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Real fix for rejected patch uaccess-warning

[Blaisorblade]

On Thursday 23 March 2006 21:58, Jeff Dike wrote:
> On Fri, Mar 10, 2006 at 04:22:41PM +0100, Blaisorblade wrote:
> > 686:		if (get_user(c, buf))
> > where "buf" is a "const char __user *".
> >
> > It's complaining because "const" was lost.
> >
> > Ultimately, I think we should remove all this copying of pointers and do
> > like i386 in this regard - there is only one cast to do, in put_user, as
> > I mentioned in last mail.
>
> OK, my first patch was insane, try this one:
>
> Index: linux-2.6.15/include/asm-um/uaccess.h
> ===================================================================
> --- linux-2.6.15.orig/include/asm-um/uaccess.h	2006-03-23
> 15:41:15.000000000 -0500 +++
> linux-2.6.15/include/asm-um/uaccess.h	2006-03-23 15:48:52.000000000 -0500
> @@ -42,10 +42,10 @@
>  #define __get_user(x, ptr) \
>  ({ \
>          const __typeof__(ptr) __private_ptr = ptr; \
> -        __typeof__(*(__private_ptr)) __private_val; \
> +        __typeof__(x) __private_val; \
>          int __private_ret = -EFAULT; \
>          (x) = (__typeof__(*(__private_ptr)))0; \
> -	if (__copy_from_user(&__private_val, (__private_ptr), \
> +	if (__copy_from_user((void *) &__private_val, (__private_ptr),	\
>  	    sizeof(*(__private_ptr))) == 0) {\
>          	(x) = (__typeof__(*(__private_ptr))) __private_val; \
>  		__private_ret = 0; \

> The (void *) fixes the warnings all by itself, but I changed the
> typeof because that's more correct and fixes some of the warnings by
> itself.

> What it can't fix is things like (from include/linux/pagemap.h):

> 	volatile char c;
> 	ret = __get_user(c, uaddr);

> We unavoidably lose the volatile because we can't know it was there,
> so the (void *) cast seems necessary in this case.

We could even do without __private_ptr, and that's surely easier to do. 
Actually I think that it could be done like for i386:

> Looking at this some more, it seems like most of the complexity could
> go away.  I think I was avoiding any assignment to the target variable
> until I knew that the copy_from_user had succeeded.  I think that was
> being too paranoid, and just copy_from_user straight into x should be
> fine.

If we think to a userspace address crossing a page bound, we can see the 
problem.

However, I think that get_user doesn't guarantee a "full copy" semantic - 
include/asm-i386/uaccess.h says it zeroes the dest var on error, so we're 
safe.
-- 
Inform me of my mistakes, so I can keep imitating Homer Simpson's "Doh!".
Paolo Giarrusso, aka Blaisorblade (Skype ID "PaoloGiarrusso", ICQ 215621894)
http://www.user-mode-linux.org/~blaisorblade

	

	
		
___________________________________ 
Yahoo! Mail: gratis 1GB per i messaggi e allegati da 10MB 
http://mail.yahoo.it



-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] Real fix for rejected patch uaccess-warning

[Jeff Dike]

On Fri, Mar 24, 2006 at 12:52:09AM +0100, Blaisorblade wrote:
> We could even do without __private_ptr, and that's surely easier to do. 
> Actually I think that it could be done like for i386:

We can, but I'm looking for a safe patch to quiet the warnings.  I did
strip down __get_user and __put_user with no apparent ill effects, but
I'm not happy sending something like that off to Andrew with 5 minutes
of testing.  I'll put the cleanup patch in my development tree and if
I don't see any problems, then I'll send it off.

> However, I think that get_user doesn't guarantee a "full copy" semantic - 
> include/asm-i386/uaccess.h says it zeroes the dest var on error, so we're 
> safe.

Yeah, I think so.  In which case, these become pretty thin wrappers
around copy_{to,from}_user.

				Jeff


-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language
that extends applications into web and mobile media. Attend the live webcast
and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel