Re: [PATCH v2 23/40] tile: enable sparse checks for get/put_user

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 12, 2015 at 07:08:36PM -0500, Chris Metcalf wrote:
> On 1/6/2015 10:44 AM, Michael S. Tsirkin wrote:
> >Tile currently does not trigger sparse warnings when get_user
> >causes an illegal assignment across bitwise types.
> >
> >For example:
> >
> >__le32 __user *p;
> >__u32 x;
> >put_user(x, p);
> >
> >violates endian-ness rules, but currently does not trigger sparse
> >warning on tile.
> >
> >Fix this by adding some dead code.
> >
> >Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >---
> >  arch/tile/include/asm/uaccess.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> >diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
> >index 22cffa1..03d905c 100644
> >--- a/arch/tile/include/asm/uaccess.h
> >+++ b/arch/tile/include/asm/uaccess.h
> >@@ -218,6 +218,8 @@ extern int __get_user_bad(void)
> >  		case 8: __get_user_8(x, ptr, __ret); break;		\
> >  		default: __ret = __get_user_bad(); break;		\
> >  		}							\
> >+		if (0)							\
> >+			x = *(__force typeof(*ptr) *)(ptr);		\
> >  		__ret;							\
> >  	})
> 
> The "if (0)" suggestion is cute, but pretty hacky :-)
> 
> Here's a revised change that may fix both of your sparse concerns with
> tile, without having to give up the optimized 64-bit get/put on tilepro.
> Would this fix both the issue discussed in this change, as well as the
> issue in the previous change (where you removed the typeof(x-x) cast)?

The patches look good, thanks! they are almost there, however, they do
not completely resolve the issue that my patch 1 tried (incorrectly) to
fix: namely x being a bitwise type.  Comments below.

> The key changes are to copy the x86 definition of __inttype(), and then to
> arrange to use an intermediate integral type that gets assigned to or from
> the actual typed value so as to expose any sparse issues.
> 
> If this works for you, I'm happy to queue it in the tile tree, or I can
> provide a proper git commit for you to include in your series, whichever
> works better for you.

Please queue it up for 3.20. Extra __force is needed in a couple of
places - would you like to fix this up yourself, or do you want me to
write a patch on top?

> --- a/arch/tile/include/asm/uaccess.h
> +++ b/arch/tile/include/asm/uaccess.h
> @@ -114,14 +114,14 @@ struct exception_table_entry {
>  extern int fixup_exception(struct pt_regs *regs);
> 
>  /*
> + * This is a type: either unsigned long, if the argument fits into
> + * that type, or otherwise unsigned long long.
> + */
> +#define __inttype(x) \
> +       __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> +
> +/*
>   * Support macros for __get_user().
> - *
> - * Implementation note: The "case 8" logic of casting to the type of
> - * the result of subtracting the value from itself is basically a way
> - * of keeping all integer types the same, but casting any pointers to
> - * ptrdiff_t, i.e. also an integer type.  This way there are no
> - * questionable casts seen by the compiler on an ILP32 platform.
> - *
>   * Note that __get_user() and __put_user() assume proper alignment.
>   */
> 
> @@ -178,7 +178,7 @@ extern int fixup_exception(struct pt_regs *regs);
>                              "9:"                                       \
>                              : "=r" (ret), "=r" (__a), "=&r" (__b)      \
>                              : "r" (ptr), "i" (-EFAULT));               \
> -               (x) = (__typeof(x))(__typeof((x)-(x)))                  \
> +               (x) = (__typeof(x))(__inttype(x))                       \
>                         (((u64)__hi32(__a, __b) << 32) |                \
>                          __lo32(__a, __b));                             \
>         })

This cast to __typeof(x) needs to be done with __force, otherwise
there will be a warning with bitwise types.


> @@ -210,14 +210,16 @@ extern int __get_user_bad(void)
>  #define __get_user(x, ptr)                                             \
>         ({                                                              \
>                 int __ret;                                              \
> +               __inttype(*(ptr)) __x;                                  \
>                 __chk_user_ptr(ptr);                                    \
>                 switch (sizeof(*(ptr))) {                               \
> -               case 1: __get_user_1(x, ptr, __ret); break;             \
> -               case 2: __get_user_2(x, ptr, __ret); break;             \
> -               case 4: __get_user_4(x, ptr, __ret); break;             \
> -               case 8: __get_user_8(x, ptr, __ret); break;             \
> +               case 1: __get_user_1(__x, ptr, __ret); break;           \
> +               case 2: __get_user_2(__x, ptr, __ret); break;           \
> +               case 4: __get_user_4(__x, ptr, __ret); break;           \
> +               case 8: __get_user_8(__x, ptr, __ret); break;           \
>                 default: __ret = __get_user_bad(); break;               \
>                 }                                                       \
> +               (x) = (typeof(x))__x;                                   \

And this one too.

>                 __ret;                                                  \
>         })
> 
> @@ -246,7 +248,7 @@ extern int __get_user_bad(void)
>  #define __put_user_4(x, ptr, ret) __put_user_asm(sw, x, ptr, ret)
>  #define __put_user_8(x, ptr, ret)                                      \
>         ({                                                              \
> -               u64 __x = (__typeof((x)-(x)))(x);                       \
> +               u64 __x = (__inttype(x))(x);                            \

And this cast to __inttype too.


>                 int __lo = (int) __x, __hi = (int) (__x >> 32);         \
>                 asm volatile("1: { sw %1, %2; addi %0, %1, 4 }\n"       \
>                              "2: { sw %0, %3; movei %0, 0 }\n"          \
> @@ -289,12 +291,13 @@ extern int __put_user_bad(void)
>  #define __put_user(x, ptr)                                             \
>  ({                                                                     \
>         int __ret;                                                      \
> +       typeof(*(ptr)) __x = (x);                                       \
>         __chk_user_ptr(ptr);                                            \
>         switch (sizeof(*(ptr))) {                                       \
> -       case 1: __put_user_1(x, ptr, __ret); break;                     \
> -       case 2: __put_user_2(x, ptr, __ret); break;                     \
> -       case 4: __put_user_4(x, ptr, __ret); break;                     \
> -       case 8: __put_user_8(x, ptr, __ret); break;                     \
> +       case 1: __put_user_1(__x, ptr, __ret); break;                   \
> +       case 2: __put_user_2(__x, ptr, __ret); break;                   \
> +       case 4: __put_user_4(__x, ptr, __ret); break;                   \
> +       case 8: __put_user_8(__x, ptr, __ret); break;                   \
>         default: __ret = __put_user_bad(); break;                       \
>         }                                                               \
>         __ret;                                                          \
> 
> -- 
> Chris Metcalf, EZChip Semiconductor
> http://www.ezchip.com