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

From: Michael S. Tsirkin
Date: Wed Dec 31 2014 - 13:38:52 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/