Re: [PATCH repost 09/16] metag/uaccess: fix sparse errors
On Mon, Jan 05, 2015 at 02:47:19PM +, James Hogan wrote:
On 5 January 2015 at 13:00, Michael S. Tsirkin m...@redhat.com wrote:
On Mon, Jan 05, 2015 at 09:44:14AM +, James Hogan wrote:
On 04/01/15 10:52, Michael S. Tsirkin wrote:
On Fri, Jan 02, 2015 at 03:41:02PM +, James Hogan wrote:
Hi,
On 25/12/14 09:29, Michael S. Tsirkin wrote:
virtio wants to read bitwise types from userspace using get_user. At
the
moment this triggers sparse errors, since the value is passed through
an
integer.
Fix that up using __force.
I still see these sparse warnings with metag even with your patch:
CHECK drivers/vhost/vhost.c
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
Which something like the following hunk fixes in a similar way to yours:
diff --git a/arch/metag/include/asm/uaccess.h
b/arch/metag/include/asm/uaccess.h
index 0748b0a97986..594497053748 100644
--- a/arch/metag/include/asm/uaccess.h
+++ b/arch/metag/include/asm/uaccess.h
@@ -112,13 +112,17 @@ do {
\
retval = 0; \
switch (size) { \
case 1: \
- retval = __put_user_asm_b((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_b((__force unsigned int)x, ptr); \
+ break; \
case 2: \
- retval = __put_user_asm_w((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_w((__force unsigned int)x, ptr); \
+ break; \
case 4: \
- retval = __put_user_asm_d((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_d((__force unsigned int)x, ptr); \
+ break; \
case 8: \
- retval = __put_user_asm_l((unsigned long long)x, ptr);
break; \
+ retval = __put_user_asm_l((__force unsigned long long)x,
ptr); \
+ break; \
default:\
__put_user_bad(); \
}
As far as I understand it, using __force on the value (as opposed to the
pointer) is safe here, in the sense of not masking any genuine defects.
Do you agree? Do you want to apply that hunk with your patch too?
what about this code:
u16 v = 0;
int rc = get_user(v, (__force __le16 __user *)p);
it should trigger a warning, does it?
No, but it didn't previously either, and doesn't seem to with x86_64
either.
I tried __be16 too, since I presume you're referring to a discrepancy
between the native byte order of v (u16) and the foreign byte order of
*p (__be16)?
Cheers
James
Yes.
It certainly does for me.
Did you define __CHECK_ENDIAN__?
Documentation/sparse.txt says:
To perform endianness
checks, you may define __CHECK_ENDIAN__:
make C=2 CF=-D__CHECK_ENDIAN__
These checks are disabled by default as they generate a host of warnings.
Or just try
Re: [PATCH repost 09/16] metag/uaccess: fix sparse errors
On Mon, Jan 05, 2015 at 09:44:14AM +, James Hogan wrote:
On 04/01/15 10:52, Michael S. Tsirkin wrote:
On Fri, Jan 02, 2015 at 03:41:02PM +, James Hogan wrote:
Hi,
On 25/12/14 09:29, Michael S. Tsirkin wrote:
virtio wants to read bitwise types from userspace using get_user. At the
moment this triggers sparse errors, since the value is passed through an
integer.
Fix that up using __force.
I still see these sparse warnings with metag even with your patch:
CHECK drivers/vhost/vhost.c
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
Which something like the following hunk fixes in a similar way to yours:
diff --git a/arch/metag/include/asm/uaccess.h
b/arch/metag/include/asm/uaccess.h
index 0748b0a97986..594497053748 100644
--- a/arch/metag/include/asm/uaccess.h
+++ b/arch/metag/include/asm/uaccess.h
@@ -112,13 +112,17 @@ do {
\
retval = 0; \
switch (size) { \
case 1: \
- retval = __put_user_asm_b((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_b((__force unsigned int)x, ptr); \
+ break; \
case 2: \
- retval = __put_user_asm_w((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_w((__force unsigned int)x, ptr); \
+ break; \
case 4: \
- retval = __put_user_asm_d((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_d((__force unsigned int)x, ptr); \
+ break; \
case 8: \
- retval = __put_user_asm_l((unsigned long long)x, ptr); break; \
+ retval = __put_user_asm_l((__force unsigned long long)x, ptr); \
+ break; \
default:\
__put_user_bad(); \
}
As far as I understand it, using __force on the value (as opposed to the
pointer) is safe here, in the sense of not masking any genuine defects.
Do you agree? Do you want to apply that hunk with your patch too?
what about this code:
u16 v = 0;
int rc = get_user(v, (__force __le16 __user *)p);
it should trigger a warning, does it?
No, but it didn't previously either, and doesn't seem to with x86_64 either.
I tried __be16 too, since I presume you're referring to a discrepancy
between the native byte order of v (u16) and the foreign byte order of
*p (__be16)?
Cheers
James
Yes.
It certainly does for me.
Did you define __CHECK_ENDIAN__?
Documentation/sparse.txt says:
To perform endianness
checks, you may define __CHECK_ENDIAN__:
make C=2 CF=-D__CHECK_ENDIAN__
These checks are disabled by default as they generate a host of warnings.
Or just try with __virtio16 instead of __le16 - I think these are
enabled unconditionally.
Note, this change also suppresses warnings for writing a pointer to
userland due to the casts to unsigned int / unsigned long long, such as
the following
Re: [PATCH repost 09/16] metag/uaccess: fix sparse errors
Hi,
On 25/12/14 09:29, Michael S. Tsirkin wrote:
virtio wants to read bitwise types from userspace using get_user. At the
moment this triggers sparse errors, since the value is passed through an
integer.
Fix that up using __force.
I still see these sparse warnings with metag even with your patch:
CHECK drivers/vhost/vhost.c
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1004:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1022:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1373:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1377:21: warning: cast from restricted __virtio32
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
drivers/vhost/vhost.c:1425:13: warning: cast from restricted __virtio16
Which something like the following hunk fixes in a similar way to yours:
diff --git a/arch/metag/include/asm/uaccess.h b/arch/metag/include/asm/uaccess.h
index 0748b0a97986..594497053748 100644
--- a/arch/metag/include/asm/uaccess.h
+++ b/arch/metag/include/asm/uaccess.h
@@ -112,13 +112,17 @@ do {
\
retval = 0; \
switch (size) { \
case 1: \
- retval = __put_user_asm_b((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_b((__force unsigned int)x, ptr); \
+ break; \
case 2: \
- retval = __put_user_asm_w((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_w((__force unsigned int)x, ptr); \
+ break; \
case 4: \
- retval = __put_user_asm_d((unsigned int)x, ptr); break; \
+ retval = __put_user_asm_d((__force unsigned int)x, ptr); \
+ break; \
case 8: \
- retval = __put_user_asm_l((unsigned long long)x, ptr); break; \
+ retval = __put_user_asm_l((__force unsigned long long)x, ptr); \
+ break; \
default:\
__put_user_bad(); \
}
As far as I understand it, using __force on the value (as opposed to the
pointer) is safe here, in the sense of not masking any genuine defects.
Do you agree? Do you want to apply that hunk with your patch too?
Note, this change also suppresses warnings for writing a pointer to
userland due to the casts to unsigned int / unsigned long long, such as
the following (each 4 times due to 4 cases above):
kernel/signal.c:2740:25: warning: cast removes address space of expression
kernel/signal.c:2747:24: warning: cast removes address space of expression
kernel/signal.c:2760:24: warning: cast removes address space of expression
kernel/signal.c:2761:24: warning: cast removes address space of expression
kernel/signal.c:2775:24: warning: cast removes address space of expression
kernel/signal.c:2779:24: warning: cast removes address space of expression
kernel/signal.c:3202:25: warning: cast removes address space of expression
kernel/signal.c:3225:17: warning: cast removes address space of expression
kernel/futex.c:2769:16: warning: cast removes address space of expression
Signed-off-by: Michael S. Tsirkin m...@redhat.com
---
arch/metag/include/asm/uaccess.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/metag/include/asm/uaccess.h
b/arch/metag/include/asm/uaccess.h
index 0748b0a..c314b45 100644
---
