| Message ID | 1419499661-8566-13-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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; \ > }) > > -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; \ > > }) > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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 -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; \ })
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(-)