LinuxLists.cc - [PATCH 00/18] uaccess: fix sparse warning on get

2014-12-14 16:52:06

Subject: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

At the moment, if p and x are both tagged as bitwise types,
get_user(x, p) produces a sparse warning on many architectures.
This is because *p on these architectures is loaded into long
(typically using asm), then cast back to typeof(*p).

When typeof(*p) is a bitwise type (which is uncommon), such a cast needs
__force, otherwise sparse produces a warning.

Some architectures already have the __force tag, add it
where it's missing.

Specificlly, vhost 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.

For now, I worked around this by converting vhost code to access_ok +
__get_user, but that's not robust, I'd like to switch to get_user for 3.19.

I tested this on x86 only so far. Since it's just adding __force, should be
trivially safe everywhere?

Please review, and consider for 3.19.

Who can merge this? Arnd?

Michael S. Tsirkin (18):
x86/uaccess: fix sparse errors
alpha/uaccess: fix sparse errors
arm64/uaccess: fix sparse errors
avr32/uaccess: fix sparse errors
blackfin/uaccess: fix sparse errors
cris/uaccess: fix sparse errors
ia64/uaccess: fix sparse errors
m32r/uaccess: fix sparse errors
metag/uaccess: fix sparse errors
microblaze/uaccess: fix sparse errors
openrisc/uaccess: fix sparse errors
parisc/uaccess: fix sparse errors
powerpc/uaccess: fix sparse errors
sh/uaccess: fix sparse errors
sparc/uaccess: fix sparse errors
sparc/uaccess: fix sparse errors
xtensa/uaccess: fix sparse errors
m68k/uaccess: fix sparse errors

arch/alpha/include/asm/uaccess.h | 4 ++--
arch/arm64/include/asm/uaccess.h | 2 +-
arch/avr32/include/asm/uaccess.h | 4 ++--
arch/blackfin/include/asm/uaccess.h | 2 +-
arch/cris/include/asm/uaccess.h | 4 ++--
arch/ia64/include/asm/uaccess.h | 2 +-
arch/m32r/include/asm/uaccess.h | 4 ++--
arch/m68k/include/asm/uaccess_mm.h | 4 ++--
arch/metag/include/asm/uaccess.h | 4 ++--
arch/microblaze/include/asm/uaccess.h | 4 ++--
arch/openrisc/include/asm/uaccess.h | 4 ++--
arch/parisc/include/asm/uaccess.h | 2 +-
arch/powerpc/include/asm/uaccess.h | 6 +++---
arch/sh/include/asm/uaccess.h | 4 ++--
arch/sparc/include/asm/uaccess_32.h | 8 ++++----
arch/sparc/include/asm/uaccess_64.h | 4 ++--
arch/x86/include/asm/uaccess.h | 2 +-
arch/xtensa/include/asm/uaccess.h | 4 ++--
18 files changed, 34 insertions(+), 34 deletions(-)

--
MST

2014-12-14 16:52:19

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 01/18] x86/uaccess: fix sparse errors

2014-12-14 16:52:29

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 02/18] alpha/uaccess: fix sparse errors

2014-12-14 16:52:35

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 04/18] avr32/uaccess: fix sparse errors

2014-12-14 16:52:40

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 05/18] blackfin/uaccess: fix sparse errors

2014-12-14 16:52:57

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 10/18] microblaze/uaccess: fix sparse errors

2014-12-14 16:52:45

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 08/18] m32r/uaccess: fix sparse errors

2014-12-14 16:53:03

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 11/18] openrisc/uaccess: fix sparse errors

2014-12-14 16:53:12

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 06/18] cris/uaccess: fix sparse errors

2014-12-14 16:53:09

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 12/18] parisc/uaccess: fix sparse errors

2014-12-14 16:53:22

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 15/18] sparc/uaccess: fix sparse errors

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.

Signed-off-by: Michael S. Tsirkin <[email protected]>
---
arch/sparc/include/asm/uaccess_32.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/sparc/include/asm/uaccess_32.h b/arch/sparc/include/asm/uaccess_32.h
index 9634d08..8b571a0 100644
--- a/arch/sparc/include/asm/uaccess_32.h
+++ b/arch/sparc/include/asm/uaccess_32.h
@@ -164,7 +164,7 @@ case 2: __get_user_asm(__gu_val,uh,addr,__gu_ret); break; \
case 4: __get_user_asm(__gu_val,,addr,__gu_ret); break; \
case 8: __get_user_asm(__gu_val,d,addr,__gu_ret); break; \
default: __gu_val = 0; __gu_ret = __get_user_bad(); break; \
-} } else { __gu_val = 0; __gu_ret = -EFAULT; } x = (type) __gu_val; __gu_ret; })
+} } else { __gu_val = 0; __gu_ret = -EFAULT; } x = (__force type) __gu_val; __gu_ret; })

#define __get_user_check_ret(x,addr,size,type,retval) ({ \
register unsigned long __gu_val __asm__ ("l1"); \
@@ -175,7 +175,7 @@ case 2: __get_user_asm_ret(__gu_val,uh,addr,retval); break; \
case 4: __get_user_asm_ret(__gu_val,,addr,retval); break; \
case 8: __get_user_asm_ret(__gu_val,d,addr,retval); break; \
default: if (__get_user_bad()) return retval; \
-} x = (type) __gu_val; } else return retval; })
+} x = (__force type) __gu_val; } else return retval; })

#define __get_user_nocheck(x,addr,size,type) ({ \
register int __gu_ret; \
@@ -186,7 +186,7 @@ case 2: __get_user_asm(__gu_val,uh,addr,__gu_ret); break; \
case 4: __get_user_asm(__gu_val,,addr,__gu_ret); break; \
case 8: __get_user_asm(__gu_val,d,addr,__gu_ret); break; \
default: __gu_val = 0; __gu_ret = __get_user_bad(); break; \
-} x = (type) __gu_val; __gu_ret; })
+} x = (__force type) __gu_val; __gu_ret; })

#define __get_user_nocheck_ret(x,addr,size,type,retval) ({ \
register unsigned long __gu_val __asm__ ("l1"); \
@@ -196,7 +196,7 @@ case 2: __get_user_asm_ret(__gu_val,uh,addr,retval); break; \
case 4: __get_user_asm_ret(__gu_val,,addr,retval); break; \
case 8: __get_user_asm_ret(__gu_val,d,addr,retval); break; \
default: if (__get_user_bad()) return retval; \
-} x = (type) __gu_val; })
+} x = (__force type) __gu_val; })

#define __get_user_asm(x,size,addr,ret) \
__asm__ __volatile__( \
--
MST

2014-12-14 16:53:18

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 13/18] powerpc/uaccess: fix sparse errors

2014-12-14 16:53:28

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 18/18] m68k/uaccess: fix sparse errors

2014-12-14 16:53:49

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 16/18] sparc/uaccess: fix sparse errors

2014-12-14 16:54:00

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 03/18] arm64/uaccess: fix sparse errors

2014-12-14 16:54:33

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 17/18] xtensa/uaccess: fix sparse errors

2014-12-14 16:55:23

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 14/18] sh/uaccess: fix sparse errors

2014-12-14 16:52:54

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 09/18] metag/uaccess: fix sparse errors

2014-12-14 16:56:25

by Michael S. Tsirkin

[permalink] [raw]

Subject: [PATCH 07/18] ia64/uaccess: fix sparse errors

2014-12-14 20:36:09

by Hans-Christian Noren Egtvedt

[permalink] [raw]

Subject: Re: [PATCH 04/18] avr32/uaccess: fix sparse errors

Around Sun 14 Dec 2014 18:52:14 +0200 or thereabout, 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.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>

Acked-by: Hans-Christian Egtvedt <[email protected]>

> ---
> arch/avr32/include/asm/uaccess.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

<snipp diff>

--
Hans-Christian Egtvedt

2014-12-15 00:05:37

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

On Sun, 2014-12-14 at 18:52 +0200, 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.

You mean bitfields ? Argh ... we should just remove them from the
compiler and be done with it :-(

Ben.

> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> arch/powerpc/include/asm/uaccess.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 9485b43..a0c071d 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -284,7 +284,7 @@ do { \
> if (!is_kernel_addr((unsigned long)__gu_addr)) \
> might_fault(); \
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
> #endif /* __powerpc64__ */
> @@ -297,7 +297,7 @@ do { \
> might_fault(); \
> if (access_ok(VERIFY_READ, __gu_addr, (size))) \
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
>
> @@ -308,7 +308,7 @@ do { \
> const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
> __chk_user_ptr(ptr); \
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
>

2014-12-15 00:41:14

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Sun, 2014-12-14 at 18:51 +0200, Michael S. Tsirkin wrote:
> At the moment, if p and x are both tagged as bitwise types,
> get_user(x, p) produces a sparse warning on many architectures.
> This is because *p on these architectures is loaded into long
> (typically using asm), then cast back to typeof(*p).
>
> When typeof(*p) is a bitwise type (which is uncommon), such a cast needs
> __force, otherwise sparse produces a warning.

What does __force actually mean? Force the cast even though it's a bitfield? Or
does it mean more than that?

ie. are we loosing the ability to detect any actual errors by adding force?

cheers

2014-12-15 01:38:22

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

On Mon, 2014-12-15 at 11:05 +1100, Benjamin Herrenschmidt wrote:
> On Sun, 2014-12-14 at 18:52 +0200, 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.
>
> You mean bitfields ? Argh ... we should just remove them from the
> compiler and be done with it :-(

Oh... no I suppose you actually meant explicit endian fields, ie, __be32
or __le32 right ? Ack on that. It sucks a bit but I think it's
acceptable.

Cheers,
Ben.

> Ben.
>
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > arch/powerpc/include/asm/uaccess.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 9485b43..a0c071d 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -284,7 +284,7 @@ do { \
> > if (!is_kernel_addr((unsigned long)__gu_addr)) \
> > might_fault(); \
> > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > __gu_err; \
> > })
> > #endif /* __powerpc64__ */
> > @@ -297,7 +297,7 @@ do { \
> > might_fault(); \
> > if (access_ok(VERIFY_READ, __gu_addr, (size))) \
> > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > __gu_err; \
> > })
> >
> > @@ -308,7 +308,7 @@ do { \
> > const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
> > __chk_user_ptr(ptr); \
> > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > __gu_err; \
> > })
> >
>

2014-12-15 10:03:16

by Ley Foon Tan

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Mon, Dec 15, 2014 at 12:51 AM, Michael S. Tsirkin <[email protected]> wrote:
> At the moment, if p and x are both tagged as bitwise types,
> get_user(x, p) produces a sparse warning on many architectures.
> This is because *p on these architectures is loaded into long
> (typically using asm), then cast back to typeof(*p).
>
> When typeof(*p) is a bitwise type (which is uncommon), such a cast needs
> __force, otherwise sparse produces a warning.
>
> Some architectures already have the __force tag, add it
> where it's missing.
>
> Specificlly, vhost 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.
>
> For now, I worked around this by converting vhost code to access_ok +
> __get_user, but that's not robust, I'd like to switch to get_user for 3.19.
>
> I tested this on x86 only so far. Since it's just adding __force, should be
> trivially safe everywhere?
>
> Please review, and consider for 3.19.
>
> Who can merge this? Arnd?
Hi Michael

FYI, there is new arch (arch/nios2) in 3.19. Is your patchset include
the fix for it?
Thanks.

Regards
Ley Foon

2014-12-15 10:17:17

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Mon, Dec 15, 2014 at 06:03:10PM +0800, LF.Tan wrote:
> On Mon, Dec 15, 2014 at 12:51 AM, Michael S. Tsirkin <[email protected]> wrote:
> > At the moment, if p and x are both tagged as bitwise types,
> > get_user(x, p) produces a sparse warning on many architectures.
> > This is because *p on these architectures is loaded into long
> > (typically using asm), then cast back to typeof(*p).
> >
> > When typeof(*p) is a bitwise type (which is uncommon), such a cast needs
> > __force, otherwise sparse produces a warning.
> >
> > Some architectures already have the __force tag, add it
> > where it's missing.
> >
> > Specificlly, vhost 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.
> >
> > For now, I worked around this by converting vhost code to access_ok +
> > __get_user, but that's not robust, I'd like to switch to get_user for 3.19.
> >
> > I tested this on x86 only so far. Since it's just adding __force, should be
> > trivially safe everywhere?
> >
> > Please review, and consider for 3.19.
> >
> > Who can merge this? Arnd?
> Hi Michael
>
> FYI, there is new arch (arch/nios2) in 3.19. Is your patchset include
> the fix for it?
> Thanks.
>
> Regards
> Ley Foon

Thanks for the heads up.
No - it will probably need a similar fix for __get_user and get_user.
It's just a parse warning so it's not a blocker.
Can you fix it yourself or prefer me to?

--
MST

2014-12-15 10:22:44

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Mon, Dec 15, 2014 at 11:40:52AM +1100, Michael Ellerman wrote:
> On Sun, 2014-12-14 at 18:51 +0200, Michael S. Tsirkin wrote:
> > At the moment, if p and x are both tagged as bitwise types,
> > get_user(x, p) produces a sparse warning on many architectures.
> > This is because *p on these architectures is loaded into long
> > (typically using asm), then cast back to typeof(*p).
> >
> > When typeof(*p) is a bitwise type (which is uncommon), such a cast needs
> > __force, otherwise sparse produces a warning.
>
> What does __force actually mean? Force the cast even though it's a bitfield? Or
> does it mean more than that?

It's not a bitfield = it's a bitwise integer.
Once you tag a typedef as bitwise, casts to and from
an untypedefed integer cause sparse warnings.

> ie. are we loosing the ability to detect any actual errors by adding force?
>
> cheers

I think we aren't:
get_user(x, p) should be equivalent to x = *p except it
validates the pointer is to userspace memory,
and can handle pagefaults.

Sparse warnings are triggered because these macros use
an untyped integer internally.

Note that we are casting to typeof(*p) not typeof(x).
Even with the cast, if x and *p are of different types we should get the
warning, so I think we are not loosing the ability to detect any actual
errors.

--
MST

2014-12-15 11:17:29

by Will Deacon

[permalink] [raw]

Subject: Re: [PATCH 03/18] arm64/uaccess: fix sparse errors

On Sun, Dec 14, 2014 at 04:52:09PM +0000, 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.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> arch/arm64/include/asm/uaccess.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 3bf8f4e..8d66bcf 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -147,7 +147,7 @@ do { \
> default: \
> BUILD_BUG(); \
> } \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> } while (0)
>
> #define __get_user(x, ptr) \

Acked-by: Will Deacon <[email protected]>

Will

2014-12-15 11:23:22

by Russell King - ARM Linux

[permalink] [raw]

Subject: Re: [PATCH 03/18] arm64/uaccess: fix sparse errors

On Mon, Dec 15, 2014 at 11:17:16AM +0000, Will Deacon wrote:
> On Sun, Dec 14, 2014 at 04:52:09PM +0000, 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.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > arch/arm64/include/asm/uaccess.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > index 3bf8f4e..8d66bcf 100644
> > --- a/arch/arm64/include/asm/uaccess.h
> > +++ b/arch/arm64/include/asm/uaccess.h
> > @@ -147,7 +147,7 @@ do { \
> > default: \
> > BUILD_BUG(); \
> > } \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > } while (0)
> >
> > #define __get_user(x, ptr) \
>
> Acked-by: Will Deacon <[email protected]>

This also means you can do stuff like:

u32 *p;
__le32 v;

err = get_user(p, v);

which is not right. Both the dereferenced pointer type and the destination
type should be compatible, and if one is a bitwise type but the other isn't,
that seems like a valid case to warn.

I don't see any use of get_user() in drivers/virtio in mainline, so I can't
check further.

--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

2014-12-15 11:31:23

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 03/18] arm64/uaccess: fix sparse errors

On Mon, Dec 15, 2014 at 11:23:11AM +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 15, 2014 at 11:17:16AM +0000, Will Deacon wrote:
> > On Sun, Dec 14, 2014 at 04:52:09PM +0000, 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.
> > >
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > ---
> > > arch/arm64/include/asm/uaccess.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > > index 3bf8f4e..8d66bcf 100644
> > > --- a/arch/arm64/include/asm/uaccess.h
> > > +++ b/arch/arm64/include/asm/uaccess.h
> > > @@ -147,7 +147,7 @@ do { \
> > > default: \
> > > BUILD_BUG(); \
> > > } \
> > > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > > } while (0)
> > >
> > > #define __get_user(x, ptr) \
> >
> > Acked-by: Will Deacon <[email protected]>
>
> This also means you can do stuff like:
>
> u32 *p;
> __le32 v;
>
> err = get_user(p, v);
>
> which is not right.

Are you sure? We are casting to __typeof__(*(ptr))),
then assigning to x. So if *p and v are different sparse will
still warn.

> Both the dereferenced pointer type and the destination
> type should be compatible, and if one is a bitwise type but the other isn't,
> that seems like a valid case to warn.
>
> I don't see any use of get_user() in drivers/virtio in mainline, so I can't
> check further.

It's not in drivers/virtio.
We have a work around upstream (uses __get_user + access_ok).
Try this patch reverting the work-around, you will see the sparse
warning:

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ed71b53..4c83be8 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1047,9 +1047,7 @@ int vhost_init_used(struct vhost_virtqueue *vq)
if (r)
return r;
vq->signalled_used_valid = false;
- if (!access_ok(VERIFY_READ, &vq->used->idx, sizeof vq->used->idx))
- return -EFAULT;
- r = __get_user(last_used_idx, &vq->used->idx);
+ r = get_user(last_used_idx, &vq->used->idx);
if (r)
return r;
vq->last_used_idx = vhost16_to_cpu(vq, last_used_idx);

> --
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.

2014-12-15 12:55:12

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 03/18] arm64/uaccess: fix sparse errors

On Mon, Dec 15, 2014 at 11:23:11AM +0000, Russell King - ARM Linux wrote:
> On Mon, Dec 15, 2014 at 11:17:16AM +0000, Will Deacon wrote:
> > On Sun, Dec 14, 2014 at 04:52:09PM +0000, 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.
> > >
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > ---
> > > arch/arm64/include/asm/uaccess.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> > > index 3bf8f4e..8d66bcf 100644
> > > --- a/arch/arm64/include/asm/uaccess.h
> > > +++ b/arch/arm64/include/asm/uaccess.h
> > > @@ -147,7 +147,7 @@ do { \
> > > default: \
> > > BUILD_BUG(); \
> > > } \
> > > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > > } while (0)
> > >
> > > #define __get_user(x, ptr) \
> >
> > Acked-by: Will Deacon <[email protected]>
>
> This also means you can do stuff like:
>
> u32 *p;
> __le32 v;
>
> err = get_user(p, v);
>
> which is not right. Both the dereferenced pointer type and the destination
> type should be compatible, and if one is a bitwise type but the other isn't,
> that seems like a valid case to warn.

I just verified this case:
#define __force __attribute__((force))
#define __bitwise__ __attribute__((bitwise))
#define get_user(x, ptr) \
do {\
unsigned long __gu_val = 0; \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
} while (0)

typedef unsigned u32;
typedef u32 __bitwise__ __le32;

static u32 *p;
static __le32 v;

int main(int argc, char **argv)
{
get_user(v, p);
return 0;
}

Produces a warning as expected.

So I think the above comment is a result of a mistake.
Can you confirm please?

> I don't see any use of get_user() in drivers/virtio in mainline, so I can't
> check further.

this is an example of the case which I'm fixing:

#define __force __attribute__((force))
#define __bitwise__ __attribute__((bitwise))
#define get_user(x, ptr) \
do {\
unsigned long __gu_val = 0; \
(x) = (__typeof__(*(ptr)))__gu_val; \
} while (0)

typedef unsigned u32;
typedef u32 __bitwise__ __le32;

static __le32 *p;
static __le32 v;

int main(int argc, char **argv)
{
get_user(v, p);
return 0;
}

the code is correct but produces a warning:
a.c:18:9: warning: cast to restricted __le32

The cast near __typeof__ above needs __force,
this is what my patch does.

> --
> FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
> according to speedtest.net.

2014-12-16 06:06:54

by Ley Foon Tan

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Mon, Dec 15, 2014 at 6:17 PM, Michael S. Tsirkin <[email protected]> wrote:
> Thanks for the heads up.
> No - it will probably need a similar fix for __get_user and get_user.
> It's just a parse warning so it's not a blocker.
> Can you fix it yourself or prefer me to?
Do you have newer revision of this series?
If yes, then it is great if you can include it. Otherwise, I can fix it.

Regards

Ley Foon

2014-12-16 06:57:48

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Tue, Dec 16, 2014 at 02:06:50PM +0800, Ley Foon Tan wrote:
> On Mon, Dec 15, 2014 at 6:17 PM, Michael S. Tsirkin <[email protected]> wrote:
> > Thanks for the heads up.
> > No - it will probably need a similar fix for __get_user and get_user.
> > It's just a parse warning so it's not a blocker.
> > Can you fix it yourself or prefer me to?
> Do you have newer revision of this series?

I didn't find a reason to create a newer revision yet.

> If yes, then it is great if you can include it. Otherwise, I can fix it.
>
> Regards
>
> Ley Foon

2014-12-16 16:45:40

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Tue, Dec 16, 2014 at 02:06:50PM +0800, Ley Foon Tan wrote:
> On Mon, Dec 15, 2014 at 6:17 PM, Michael S. Tsirkin <[email protected]> wrote:
> > Thanks for the heads up.
> > No - it will probably need a similar fix for __get_user and get_user.
> > It's just a parse warning so it's not a blocker.
> > Can you fix it yourself or prefer me to?
> Do you have newer revision of this series?
> If yes, then it is great if you can include it. Otherwise, I can fix it.
>
> Regards
>
> Ley Foon

Arnd said he'll merge whatever is not fixed in -rc1 through his tree for
3.20.
So it's up to you - fix it up in your tree before -rc1 or wait for
mer to fix it after rc1 and have Arnd merge it for 3.20.

--
MST

2014-12-16 16:47:30

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 01/18] x86/uaccess: fix sparse errors

On Sun, Dec 14, 2014 at 06:51:57PM +0200, 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.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---

Ping.
Do x86 maintainers consider fixing sparse errors
applicable for 3.19?
If yes, can you pls merge this patch?

> arch/x86/include/asm/uaccess.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 0d592e0..8ce1af8 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -179,7 +179,7 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> asm volatile("call __get_user_%P3" \
> : "=a" (__ret_gu), "=r" (__val_gu) \
> : "0" (ptr), "i" (sizeof(*(ptr)))); \
> - (x) = (__typeof__(*(ptr))) __val_gu; \
> + (x) = (__force __typeof__(*(ptr))) __val_gu; \
> __ret_gu; \
> })
>
> --
> MST
>

2014-12-16 16:48:08

by Michael S. Tsirkin

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

On Sun, Dec 14, 2014 at 06:52:51PM +0200, 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.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>

Ping.
Do powerpc maintainers consider fixing sparse errors
applicable for 3.19?
If yes, can you pls merge this patch?

> ---
> arch/powerpc/include/asm/uaccess.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 9485b43..a0c071d 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -284,7 +284,7 @@ do { \
> if (!is_kernel_addr((unsigned long)__gu_addr)) \
> might_fault(); \
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
> #endif /* __powerpc64__ */
> @@ -297,7 +297,7 @@ do { \
> might_fault(); \
> if (access_ok(VERIFY_READ, __gu_addr, (size))) \
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
>
> @@ -308,7 +308,7 @@ do { \
> const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
> __chk_user_ptr(ptr); \
> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> - (x) = (__typeof__(*(ptr)))__gu_val; \
> + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> __gu_err; \
> })
>
> --
> MST
>

2014-12-16 23:50:18

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Mon, 2014-12-15 at 12:22 +0200, Michael S. Tsirkin wrote:
> On Mon, Dec 15, 2014 at 11:40:52AM +1100, Michael Ellerman wrote:
> > On Sun, 2014-12-14 at 18:51 +0200, Michael S. Tsirkin wrote:
> > > At the moment, if p and x are both tagged as bitwise types,
> > > get_user(x, p) produces a sparse warning on many architectures.
> > > This is because *p on these architectures is loaded into long
> > > (typically using asm), then cast back to typeof(*p).
> > >
> > > When typeof(*p) is a bitwise type (which is uncommon), such a cast needs
> > > __force, otherwise sparse produces a warning.
> >
> > What does __force actually mean? Force the cast even though it's a bitfield? Or
> > does it mean more than that?
>
> It's not a bitfield = it's a bitwise integer.

OK. I didn't know what that meant, but Documentation/sparse.txt taught me.

So it's a sparse only thing.

> Once you tag a typedef as bitwise, casts to and from
> an untypedefed integer cause sparse warnings.
>
> > ie. are we loosing the ability to detect any actual errors by adding force?
> >
> > cheers
>
> I think we aren't:
> get_user(x, p) should be equivalent to x = *p except it
> validates the pointer is to userspace memory,
> and can handle pagefaults.
>
> Sparse warnings are triggered because these macros use
> an untyped integer internally.
>
> Note that we are casting to typeof(*p) not typeof(x).
> Even with the cast, if x and *p are of different types we should get the
> warning, so I think we are not loosing the ability to detect any actual
> errors.

OK. Sounds good then.

cheers

2014-12-16 23:50:49

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

2014-12-17 00:53:10

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

On Tue, 2014-12-16 at 18:47 +0200, Michael S. Tsirkin wrote:
> On Sun, Dec 14, 2014 at 06:52:51PM +0200, 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.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
>
> Ping.
> Do powerpc maintainers consider fixing sparse errors
> applicable for 3.19?
> If yes, can you pls merge this patch?

Relax :-) Our patches are tracked in Patchwork and such fixes aren't
necessarily constrained by the merge window. Michael will probably
pick it up but don't expect systematic replies to patches in 2 days ...

Also, when sending a series like that where one of us only gets
CCed on one of the patch, it helps to make it clear whether you
only expect an ack or whether you expect us to take the patch.

Cheers,
Ben.
> > ---
> > arch/powerpc/include/asm/uaccess.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 9485b43..a0c071d 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -284,7 +284,7 @@ do { \
> > if (!is_kernel_addr((unsigned long)__gu_addr)) \
> > might_fault(); \
> > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > __gu_err; \
> > })
> > #endif /* __powerpc64__ */
> > @@ -297,7 +297,7 @@ do { \
> > might_fault(); \
> > if (access_ok(VERIFY_READ, __gu_addr, (size))) \
> > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > __gu_err; \
> > })
> >
> > @@ -308,7 +308,7 @@ do { \
> > const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
> > __chk_user_ptr(ptr); \
> > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
> > - (x) = (__typeof__(*(ptr)))__gu_val; \
> > + (x) = (__force __typeof__(*(ptr)))__gu_val; \
> > __gu_err; \
> > })
> >
> > --
> > MST
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2014-12-17 10:54:16

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

On Wednesday 17 December 2014 11:52:36 Benjamin Herrenschmidt wrote:
> On Tue, 2014-12-16 at 18:47 +0200, Michael S. Tsirkin wrote:
> > On Sun, Dec 14, 2014 at 06:52:51PM +0200, 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.
> > >
> > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> >
> > Ping.
> > Do powerpc maintainers consider fixing sparse errors
> > applicable for 3.19?
> > If yes, can you pls merge this patch?
>
> Relax Our patches are tracked in Patchwork and such fixes aren't
> necessarily constrained by the merge window. Michael will probably
> pick it up but don't expect systematic replies to patches in 2 days ...
>
> Also, when sending a series like that where one of us only gets
> CCed on one of the patch, it helps to make it clear whether you
> only expect an ack or whether you expect us to take the patch.

Michael initially asked how these patches should merged, and as I
discussed with him on IRC, I wouldn't take them through the asm-generic
tree for 3.19 at this point, but I offered to take the ones that
are not picked up by arch maintainers through that tree for 3.20.

I also recommend to him to clarify this with maintainers of the
architectures he cares about most so they can decide whether to pick
it up or not, which triggered the message above.

Arnd

2014-12-17 11:05:35

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [PATCH 13/18] powerpc/uaccess: fix sparse errors

On Wed, 2014-12-17 at 11:53 +0100, Arnd Bergmann wrote:
> On Wednesday 17 December 2014 11:52:36 Benjamin Herrenschmidt wrote:
> > On Tue, 2014-12-16 at 18:47 +0200, Michael S. Tsirkin wrote:
> > > On Sun, Dec 14, 2014 at 06:52:51PM +0200, 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.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > >
> > > Ping.
> > > Do powerpc maintainers consider fixing sparse errors
> > > applicable for 3.19?
> > > If yes, can you pls merge this patch?
> >
> > Relax Our patches are tracked in Patchwork and such fixes aren't
> > necessarily constrained by the merge window. Michael will probably
> > pick it up but don't expect systematic replies to patches in 2 days ...
> >
> > Also, when sending a series like that where one of us only gets
> > CCed on one of the patch, it helps to make it clear whether you
> > only expect an ack or whether you expect us to take the patch.
>
> Michael initially asked how these patches should merged, and as I
> discussed with him on IRC, I wouldn't take them through the asm-generic
> tree for 3.19 at this point, but I offered to take the ones that
> are not picked up by arch maintainers through that tree for 3.20.
>
> I also recommend to him to clarify this with maintainers of the
> architectures he cares about most so they can decide whether to pick
> it up or not, which triggered the message above.

Ok, I incorrectly assumed the above was a nag for not looking at his
patch yet :)

I don't have any objection, but I'm leaving the merging for now to
Michael (and possibly for ever, we'll see ... :) as I'm on vacation
until end of January.

Cheers,
Ben.

2014-12-18 03:11:35

by Steven Miao

[permalink] [raw]

Subject: Re: [PATCH 05/18] blackfin/uaccess: fix sparse errors

On Mon, Dec 15, 2014 at 12:52 AM, Michael S. Tsirkin <[email protected]> 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.
>
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
> arch/blackfin/include/asm/uaccess.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/blackfin/include/asm/uaccess.h b/arch/blackfin/include/asm/uaccess.h
> index 57701c3..6a46e7d 100644
> --- a/arch/blackfin/include/asm/uaccess.h
> +++ b/arch/blackfin/include/asm/uaccess.h
> @@ -147,7 +147,7 @@ static inline int bad_user_access_length(void)
> } \
> } else \
> _err = -EFAULT; \
> - x = (typeof(*(ptr)))_val; \
> + x = (__force typeof(*(ptr)))_val; \
> _err; \
> })
>
> --
> MST
>
Acked-by: Steven Miao <[email protected]>

2014-12-18 06:54:21

by Ley Foon Tan

[permalink] [raw]

Subject: Re: [PATCH 00/18] uaccess: fix sparse warning on get_user for bitwise types

On Wed, Dec 17, 2014 at 12:45 AM, Michael S. Tsirkin <[email protected]> wrote:
> On Tue, Dec 16, 2014 at 02:06:50PM +0800, Ley Foon Tan wrote:
>> On Mon, Dec 15, 2014 at 6:17 PM, Michael S. Tsirkin <[email protected]> wrote:
>> > Thanks for the heads up.
>> > No - it will probably need a similar fix for __get_user and get_user.
>> > It's just a parse warning so it's not a blocker.
>> > Can you fix it yourself or prefer me to?
>> Do you have newer revision of this series?
>> If yes, then it is great if you can include it. Otherwise, I can fix it.
>>
>> Regards
>>
>> Ley Foon
>
> Arnd said he'll merge whatever is not fixed in -rc1 through his tree for
> 3.20.
> So it's up to you - fix it up in your tree before -rc1 or wait for
> mer to fix it after rc1 and have Arnd merge it for 3.20.
I will fix it in nios2 tree.

Thanks.

Regards
Ley Foon