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 ---