[PATCH repost 12/16] parisc/uaccess: fix sparse errors

[PATCH repost 12/16] parisc/uaccess: fix sparse errors

[Michael S. Tsirkin]

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 <mst@redhat.com>
---
 arch/parisc/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
index a5cb070..3a20da6 100644
--- a/arch/parisc/include/asm/uaccess.h
+++ b/arch/parisc/include/asm/uaccess.h
@@ -104,7 +104,7 @@ struct exception_data {
 	    }                                           \
 	}                                               \
 							\
-	(x) = (__typeof__(*(ptr))) __gu_val;            \
+	(x) = (__force __typeof__(*(ptr))) __gu_val;            \
 	__gu_err;                                       \
 })
 
-- 
MST

Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

[Helge Deller]

Hi Michael,

On 12/25/2014 10:29 AM, Michael S. Tsirkin wrote:
> virtio wants to read bitwise types from userspace using get_user.  At the

I don't know the virtio code much yet, but does it makes sense to read bitwise types?
Will virtio then get possible troubles because of endianess correct as well?

Do you have a code example, or the sparse error message ?

Helge

> moment this triggers sparse errors, since the value is passed through an
> integer.
>
> Fix that up using __force.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   arch/parisc/include/asm/uaccess.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
> index a5cb070..3a20da6 100644
> --- a/arch/parisc/include/asm/uaccess.h
> +++ b/arch/parisc/include/asm/uaccess.h
> @@ -104,7 +104,7 @@ struct exception_data {
>   	    }                                           \
>   	}                                               \
>   							\
> -	(x) = (__typeof__(*(ptr))) __gu_val;            \
> +	(x) = (__force __typeof__(*(ptr))) __gu_val;            \
>   	__gu_err;                                       \
>   })
>
>

Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

[Michael S. Tsirkin]

On Thu, Dec 25, 2014 at 11:37:45PM +0100, Helge Deller wrote:
> Hi Michael,
> 
> On 12/25/2014 10:29 AM, Michael S. Tsirkin wrote:
> >virtio wants to read bitwise types from userspace using get_user.  At the
> 
> I don't know the virtio code much yet, but does it makes sense to read bitwise types?
> Will virtio then get possible troubles because of endianess correct as well?

There's no conversion: we are reading from __virtio16 __user *
pointer into __virtio16 v value.

> Do you have a code example, or the sparse error message ?
> 
> Helge

Sure. the code is upstream now.
The warning is below.

sparse warnings: (new ones prefixed by >>)

>> drivers/vhost/vringh.c:554:18: sparse: cast to restricted __virtio16

vim +554 drivers/vhost/vringh.c

   538                                                           __virtio16 *p, u16 val))
   539  {
   540          if (!vrh->event_indices) {
   541                  /* Old-school; update flags. */
   542                  if (putu16(vrh, &vrh->vring.used->flags,
   543                             VRING_USED_F_NO_NOTIFY)) {
   544                          vringh_bad("Setting used flags %p",
   545                                     &vrh->vring.used->flags);
   546                  }
   547          }
   548  }
   549
   550  /* Userspace access helpers: in this case, addresses are really userspace. */
   551  static inline int getu16_user(const struct vringh *vrh, u16 *val, const __virtio16 *p)
   552  {
   553          __virtio16 v = 0;
 > 554          int rc = get_user(v, (__force __virtio16 __user *)p);
   555          *val = vringh16_to_cpu(vrh, v);
   556          return rc;
   557  }
   558
   559  static inline int putu16_user(const struct vringh *vrh, __virtio16 *p, u16 val)
   560  {
   561          __virtio16 v = cpu_to_vringh16(vrh, val);
   562          return put_user(v, (__force __virtio16 __user *)p);



> 
> >moment this triggers sparse errors, since the value is passed through an
> >integer.
> >
> >Fix that up using __force.
> >
> >Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >---
> >  arch/parisc/include/asm/uaccess.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
> >index a5cb070..3a20da6 100644
> >--- a/arch/parisc/include/asm/uaccess.h
> >+++ b/arch/parisc/include/asm/uaccess.h
> >@@ -104,7 +104,7 @@ struct exception_data {
> >  	    }                                           \
> >  	}                                               \
> >  							\
> >-	(x) = (__typeof__(*(ptr))) __gu_val;            \
> >+	(x) = (__force __typeof__(*(ptr))) __gu_val;            \
> >  	__gu_err;                                       \
> >  })
> >
> >

Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

[James Bottomley]

On Sat, 2014-12-27 at 18:14 +0200, Michael S. Tsirkin wrote:
> On Thu, Dec 25, 2014 at 11:37:45PM +0100, Helge Deller wrote:
> > Hi Michael,
> > 
> > On 12/25/2014 10:29 AM, Michael S. Tsirkin wrote:
> > >virtio wants to read bitwise types from userspace using get_user.  At the
> > 
> > I don't know the virtio code much yet, but does it makes sense to read bitwise types?
> > Will virtio then get possible troubles because of endianess correct as well?
> 
> There's no conversion: we are reading from __virtio16 __user *
> pointer into __virtio16 v value.
> 
> > Do you have a code example, or the sparse error message ?
> > 
> > Helge
> 
> Sure. the code is upstream now.
> The warning is below.
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/vhost/vringh.c:554:18: sparse: cast to restricted __virtio16
> 
> vim +554 drivers/vhost/vringh.c
> 
>    538                                                           __virtio16 *p, u16 val))
>    539  {
>    540          if (!vrh->event_indices) {
>    541                  /* Old-school; update flags. */
>    542                  if (putu16(vrh, &vrh->vring.used->flags,
>    543                             VRING_USED_F_NO_NOTIFY)) {
>    544                          vringh_bad("Setting used flags %p",
>    545                                     &vrh->vring.used->flags);
>    546                  }
>    547          }
>    548  }
>    549
>    550  /* Userspace access helpers: in this case, addresses are really userspace. */
>    551  static inline int getu16_user(const struct vringh *vrh, u16 *val, const __virtio16 *p)
>    552  {
>    553          __virtio16 v = 0;
>  > 554          int rc = get_user(v, (__force __virtio16 __user *)p);
>    555          *val = vringh16_to_cpu(vrh, v);
>    556          return rc;
>    557  }
>    558
>    559  static inline int putu16_user(const struct vringh *vrh, __virtio16 *p, u16 val)
>    560  {
>    561          __virtio16 v = cpu_to_vringh16(vrh, val);
>    562          return put_user(v, (__force __virtio16 __user *)p);

OK, parisc developers still being dense, but this does look like an
abuse of the bitwise type.  bitwise is supposed to be consumed by endian
specific accessors.  get/put_user have no endian tags because they
really can't do this ... the potential for width mismatch between the
user and kernel address spaces could cause havoc if people get this
wrong, so the warning looks correct to me.

If we take your proposed patch we lose the type checking on all
accessors because of the __force.  Why not, instead, alter your code to
tell the kernel you know what you're doing:

        __u16 v = 0;
        int rc = get_user(v, (__force __u16 __user *)p);
        *val = vringh16_to_cpu(vrh, (__force __virtio16)v);
        return rc;

That way the accessors still warn if anyone else tries this but your
warning is gone and the code basically says you knew the u16 was really
an endianness specific virtio quantity?

James

Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

[Michael S. Tsirkin]

On Wed, Dec 31, 2014 at 09:17:20AM -0800, James Bottomley wrote:
> On Sat, 2014-12-27 at 18:14 +0200, Michael S. Tsirkin wrote:
> > On Thu, Dec 25, 2014 at 11:37:45PM +0100, Helge Deller wrote:
> > > Hi Michael,
> > > 
> > > On 12/25/2014 10:29 AM, Michael S. Tsirkin wrote:
> > > >virtio wants to read bitwise types from userspace using get_user.  At the
> > > 
> > > I don't know the virtio code much yet, but does it makes sense to read bitwise types?
> > > Will virtio then get possible troubles because of endianess correct as well?
> > 
> > There's no conversion: we are reading from __virtio16 __user *
> > pointer into __virtio16 v value.
> > 
> > > Do you have a code example, or the sparse error message ?
> > > 
> > > Helge
> > 
> > Sure. the code is upstream now.
> > The warning is below.
> > 
> > sparse warnings: (new ones prefixed by >>)
> > 
> > >> drivers/vhost/vringh.c:554:18: sparse: cast to restricted __virtio16
> > 
> > vim +554 drivers/vhost/vringh.c
> > 
> >    538                                                           __virtio16 *p, u16 val))
> >    539  {
> >    540          if (!vrh->event_indices) {
> >    541                  /* Old-school; update flags. */
> >    542                  if (putu16(vrh, &vrh->vring.used->flags,
> >    543                             VRING_USED_F_NO_NOTIFY)) {
> >    544                          vringh_bad("Setting used flags %p",
> >    545                                     &vrh->vring.used->flags);
> >    546                  }
> >    547          }
> >    548  }
> >    549
> >    550  /* Userspace access helpers: in this case, addresses are really userspace. */
> >    551  static inline int getu16_user(const struct vringh *vrh, u16 *val, const __virtio16 *p)
> >    552  {
> >    553          __virtio16 v = 0;
> >  > 554          int rc = get_user(v, (__force __virtio16 __user *)p);
> >    555          *val = vringh16_to_cpu(vrh, v);
> >    556          return rc;
> >    557  }
> >    558
> >    559  static inline int putu16_user(const struct vringh *vrh, __virtio16 *p, u16 val)
> >    560  {
> >    561          __virtio16 v = cpu_to_vringh16(vrh, val);
> >    562          return put_user(v, (__force __virtio16 __user *)p);
> 
> OK, parisc developers still being dense, but this does look like an
> abuse of the bitwise type.

To give you another example:

	__le16 __user *p;
	__le16 foo;
	int rc = get_user(v, p);

really should be fine, ATM this gives a warning.


>  bitwise is supposed to be consumed by endian
> specific accessors.

Surely, assignment is OK too? get_user is exactly that.

vringh16_to_cpu is an endian specific accessor.
Look up it's definition please. The reason for that __force is
because we are adding __user.
It's a decision Rusty made to reduce code duplication:
we have some code that handles both kernel and userspace pointers.

>  get/put_user have no endian tags because they
> really can't do this ... the potential for width mismatch between the
> user and kernel address spaces could cause havoc if people get this
> wrong, so the warning looks correct to me.


I'm sorry I don't understand.

Why is

	access_ok
	__get_user

safer than

	get_user

?

It does not trigger the warning, because
__get_user does not have the cast to long internally.

Also, on some architectures get_user does not cast to long
internally so there's no warning.

> If we take your proposed patch we lose the type checking on all
> accessors because of the __force.


Did you try?  In my testing, this is not at all true.

For example with my patch:


             u16 v = 0;
             int rc = get_user(v, (__force __virtio16 __user *)p);

correctly triggers a warning.



>  Why not, instead, alter your code to
> tell the kernel you know what you're doing:
> 
>         __u16 v = 0;
>         int rc = get_user(v, (__force __u16 __user *)p);
>         *val = vringh16_to_cpu(vrh, (__force __virtio16)v);
>         return rc;
> 
> That way the accessors still warn if anyone else tries this

Hmm I don't understand, sorry. Tries what?
Can you please show me an invalid use of get_user that
produces a warning currently but won't with my patch?

> but your
> warning is gone and the code basically says you knew the u16 was really
> an endianness specific virtio quantity?
> 
> James
> 


(__force __virtio16 __user *)
tells get_user exactly that pointer is to type __virtio16.
It does not get any more explicit.

What you are proposing is really discarding type
information by a bunch of __force calls.

I am very reluctant to do this.
In fact, because of the static checking I added,
conversion to virtio 1.0 went so smoothly:
most drivers worked right away after the conversion.
I'm very sure without static checking, or with
__force thrown around liberally, I would have



vringh specifically has one __force cast anyway because
it's mixing userspace and kernel pointers.

But, I also have an out of tree patch that use structures
like this:

	struct foo {
		__virtio16 bar;
	};


Now with my patches I can do:

       __virtio16 v = 0;
	struct foo __user *p;
       int rc = get_user(v, &p->bar);


-- 
MST

Re: [PATCH repost 12/16] parisc/uaccess: fix sparse errors

[James Bottomley]

On Wed, 2014-12-31 at 20:38 +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 31, 2014 at 09:17:20AM -0800, James Bottomley wrote:
[...]
> > OK, parisc developers still being dense, but this does look like an
> > abuse of the bitwise type.
> 
> To give you another example:
> 
> 	__le16 __user *p;
> 	__le16 foo;
> 	int rc = get_user(v, p);
> 
> really should be fine, ATM this gives a warning.

OK, I think I've figured it out.  You're saying that casting __gu_val to
a bitwise annotated type is an automatic sparse failure because it has
to be a long in our assembly code to receive the load/store as a
register.  However, this is required for sparse to do the correct lvalue
type = rvalue type check in the assignment to x.  We were all thinking
the __force just killed these sparse type checks.

In that case, I think parisc is fine with this.

James