Re: [RFCv2 00/12] Introduce host-side virtio queue and CAIF Virtio.

[Rusty Russell <rusty@rustcorp.com.au>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Jan 17, 2013 at 12:40:29PM +1030, Rusty Russell wrote:
>> I resist sprinkling __user everywhere because it's *not* always user
>> addresses, and it's deeply misleading to anyone reading it.  I'd rather
>> have it in one place with a big comment.
>> I can try using a union of kvec and iovec, since they are the same
>> layout in practice AFAICT.
>
> I suggest the following easy fix: as you say, it's
> in one place with a bug comment.
>
> /* On the host side we often communicate to untrusted
>  * entities over virtio, so set __user tag on addresses
>  * we get helps make sure we don't directly dereference the addresses,
>  * while making it possible to pass the addresses in iovec arrays
>  * without casts.
>  */
> #define __virtio __user
>
> /* A helper to discard __virtio tag - only call when
>  * you are communicating to a trusted entity.
>  */
> static inline void *virtio_raw_addr(__virtio void *addr)
> {
> 	return (__force void *)addr;
> }
>
> Hmm?

The two problems are iovec, which contains a __user address, and that
gets exposed via the API, and vring, which *doesn't* use __user
addresses.

This is ugly, but works:

vringh: use vringh_kiov for _kern functions, and internally.

This makes user of vringh perfectly __user-clean, and removes an
internal cast.

This only works because of -fno-strict-aliasing!

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index ab10da8..2ba087d 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -65,9 +65,9 @@ static inline int __vringh_get_head(const struct vringh *vrh,
 }
 
 /* Copy some bytes to/from the iovec.  Returns num copied. */
-static inline ssize_t vringh_iov_xfer(struct vringh_iov *iov,
+static inline ssize_t vringh_iov_xfer(struct vringh_kiov *iov,
 				      void *ptr, size_t len,
-				      int (*xfer)(void __user *addr, void *ptr,
+				      int (*xfer)(void *addr, void *ptr,
 						  size_t len))
 {
 	int err, done = 0;
@@ -149,9 +149,9 @@ static int move_to_indirect(int *up_next, u16 *i, void *addr,
 	return 0;
 }
 
-static int resize_iovec(struct vringh_iov *iov, gfp_t gfp)
+static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp)
 {
-	struct iovec *new;
+	struct kvec *new;
 	unsigned int new_num = iov->max * 2;
 
 	if (new_num < 8)
@@ -186,8 +186,8 @@ static u16 __cold return_from_indirect(const struct vringh *vrh, int *up_next,
 
 static inline int
 __vringh_iov(struct vringh *vrh, u16 i,
-	     struct vringh_iov *riov,
-	     struct vringh_iov *wiov,
+	     struct vringh_kiov *riov,
+	     struct vringh_kiov *wiov,
 	     bool (*getrange)(u64 addr, struct vringh_range *r),
 	     gfp_t gfp,
 	     int (*getdesc)(struct vring_desc *dst, const struct vring_desc *s))
@@ -204,7 +204,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
 	riov->i = wiov->i = 0;
 	for (;;) {
 		void *addr;
-		struct vringh_iov *iov;
+		struct vringh_kiov *iov;
 
 		err = getdesc(&desc, &descs[i]);
 		if (unlikely(err))
@@ -249,7 +249,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
 				goto fail;
 		}
 
-		iov->iov[iov->i].iov_base = (__force __user void *)addr;
+		iov->iov[iov->i].iov_base = addr;
 		iov->iov[iov->i].iov_len = desc.len;
 		iov->i++;
 
@@ -438,7 +438,7 @@ static inline void __vringh_notify_disable(struct vringh *vrh,
 	}
 }
 
-/* Userspace access helpers. */
+/* Userspace access helpers: in this case, addresses are really userspace. */
 static inline int getu16_user(u16 *val, const u16 *p)
 {
 	return get_user(*val, (__force u16 __user *)p);
@@ -452,27 +452,27 @@ static inline int putu16_user(u16 *p, u16 val)
 static inline int getdesc_user(struct vring_desc *dst,
 			       const struct vring_desc *src)
 {
-	return copy_from_user(dst, (__force void *)src, sizeof(*dst)) == 0 ? 0 :
-		-EFAULT;
+	return copy_from_user(dst, (__force void __user *)src, sizeof(*dst)) ?
+		-EFAULT : 0;
 }
 
 static inline int putused_user(struct vring_used_elem *dst,
 			       const struct vring_used_elem *s)
 {
-	return copy_to_user((__force void __user *)dst, s, sizeof(*dst)) == 0
-		? 0 : -EFAULT;
+	return copy_to_user((__force void __user *)dst, s, sizeof(*dst)) ?
+		-EFAULT : 0;
 }
 
 static inline int xfer_from_user(void *src, void *dst, size_t len)
 {
-	return copy_from_user(dst, (__force void *)src, len) == 0 ? 0 :
-		-EFAULT;
+	return copy_from_user(dst, (__force void __user *)src, len) ?
+		-EFAULT : 0;
 }
 
 static inline int xfer_to_user(void *dst, void *src, size_t len)
 {
-	return copy_to_user((__force void *)dst, src, len) == 0 ? 0 :
-		-EFAULT;
+	return copy_to_user((__force void __user *)dst, src, len) ?
+		-EFAULT : 0;
 }
 
 /**
@@ -506,6 +506,7 @@ int vringh_init_user(struct vringh *vrh, u32 features,
 	vrh->last_avail_idx = 0;
 	vrh->last_used_idx = 0;
 	vrh->vring.num = num;
+	/* vring expects kernel addresses, but only used via accessors. */
 	vrh->vring.desc = (__force struct vring_desc *)desc;
 	vrh->vring.avail = (__force struct vring_avail *)avail;
 	vrh->vring.used = (__force struct vring_used *)used;
@@ -543,8 +544,30 @@ int vringh_getdesc_user(struct vringh *vrh,
 	if (err == vrh->vring.num)
 		return 0;
 
+	/* We need the layouts to be the indentical for this to work */
+	BUILD_BUG_ON(sizeof(struct vringh_kiov) != sizeof(struct vringh_iov));
+	BUILD_BUG_ON(offsetof(struct vringh_kiov, iov) !=
+		     offsetof(struct vringh_iov, iov));
+	BUILD_BUG_ON(offsetof(struct vringh_kiov, i) !=
+		     offsetof(struct vringh_iov, i));
+	BUILD_BUG_ON(offsetof(struct vringh_kiov, max) !=
+		     offsetof(struct vringh_iov, max));
+	BUILD_BUG_ON(offsetof(struct vringh_kiov, allocated) !=
+		     offsetof(struct vringh_iov, allocated));
+	BUILD_BUG_ON(sizeof(struct iovec) != sizeof(struct kvec));
+	BUILD_BUG_ON(offsetof(struct iovec, iov_base) !=
+		     offsetof(struct kvec, iov_base));
+	BUILD_BUG_ON(offsetof(struct iovec, iov_len) !=
+		     offsetof(struct kvec, iov_len));
+	BUILD_BUG_ON(sizeof(((struct iovec *)NULL)->iov_base)
+		     != sizeof(((struct kvec *)NULL)->iov_base));
+	BUILD_BUG_ON(sizeof(((struct iovec *)NULL)->iov_len)
+		     != sizeof(((struct kvec *)NULL)->iov_len));
+
 	*head = err;
-	err = __vringh_iov(vrh, *head, riov, wiov, getrange, gfp, getdesc_user);
+	err = __vringh_iov(vrh, *head, (struct vringh_kiov *)riov,
+			   (struct vringh_kiov *)wiov,
+			   getrange, gfp, getdesc_user);
 	if (err)
 		return err;
 
@@ -561,7 +584,8 @@ int vringh_getdesc_user(struct vringh *vrh,
  */
 ssize_t vringh_iov_pull_user(struct vringh_iov *riov, void *dst, size_t len)
 {
-	return vringh_iov_xfer(riov, dst, len, xfer_from_user);
+	return vringh_iov_xfer((struct vringh_kiov *)riov,
+			       dst, len, xfer_from_user);
 }
 
 /**
@@ -575,7 +599,8 @@ ssize_t vringh_iov_pull_user(struct vringh_iov *riov, void *dst, size_t len)
 ssize_t vringh_iov_push_user(struct vringh_iov *wiov,
 			     const void *src, size_t len)
 {
-	return vringh_iov_xfer(wiov, (void *)src, len, xfer_to_user);
+	return vringh_iov_xfer((struct vringh_kiov *)wiov,
+			       (void *)src, len, xfer_to_user);
 }
 
 /**
@@ -734,8 +759,8 @@ int vringh_init_kern(struct vringh *vrh, u32 features,
  * have to kfree riov->iov and wiov->iov respectively.
  */
 int vringh_getdesc_kern(struct vringh *vrh,
-			struct vringh_iov *riov,
-			struct vringh_iov *wiov,
+			struct vringh_kiov *riov,
+			struct vringh_kiov *wiov,
 			u16 *head,
 			gfp_t gfp)
 {
@@ -766,7 +791,7 @@ int vringh_getdesc_kern(struct vringh *vrh,
  *
  * Returns the bytes copied <= len or a negative errno.
  */
-ssize_t vringh_iov_pull_kern(struct vringh_iov *riov, void *dst, size_t len)
+ssize_t vringh_iov_pull_kern(struct vringh_kiov *riov, void *dst, size_t len)
 {
 	return vringh_iov_xfer(riov, dst, len, xfer_kern);
 }
@@ -779,7 +804,7 @@ ssize_t vringh_iov_pull_kern(struct vringh_iov *riov, void *dst, size_t len)
  *
  * Returns the bytes copied <= len or a negative errno.
  */
-ssize_t vringh_iov_push_kern(struct vringh_iov *wiov,
+ssize_t vringh_iov_push_kern(struct vringh_kiov *wiov,
 			     const void *src, size_t len)
 {
 	return vringh_iov_xfer(wiov, (void *)src, len, xfer_kern);
diff --git a/include/linux/vringh.h b/include/linux/vringh.h
index 9df86e9..b3345e9 100644
--- a/include/linux/vringh.h
+++ b/include/linux/vringh.h
@@ -24,7 +24,7 @@
 #ifndef _LINUX_VRINGH_H
 #define _LINUX_VRINGH_H
 #include <uapi/linux/virtio_ring.h>
-#include <uapi/linux/uio.h>
+#include <linux/uio.h>
 #include <asm/barrier.h>
 
 /* virtio_ring with information needed for host access. */
@@ -64,6 +64,13 @@ struct vringh_iov {
 	bool allocated;
 };
 
+/* All the information about a kvec. */
+struct vringh_kiov {
+	struct kvec *iov;
+	unsigned i, max;
+	bool allocated;
+};
+
 /* Helpers for userspace vrings. */
 int vringh_init_user(struct vringh *vrh, u32 features,
 		     unsigned int num, bool weak_barriers,
@@ -103,13 +110,13 @@ int vringh_init_kern(struct vringh *vrh, u32 features,
 		     struct vring_used *used);
 
 int vringh_getdesc_kern(struct vringh *vrh,
-			struct vringh_iov *riov,
-			struct vringh_iov *wiov,
+			struct vringh_kiov *riov,
+			struct vringh_kiov *wiov,
 			u16 *head,
 			gfp_t gfp);
 
-ssize_t vringh_iov_pull_kern(struct vringh_iov *riov, void *dst, size_t len);
-ssize_t vringh_iov_push_user(struct vringh_iov *wiov,
+ssize_t vringh_iov_pull_kern(struct vringh_kiov *riov, void *dst, size_t len);
+ssize_t vringh_iov_push_kern(struct vringh_kiov *wiov,
 			     const void *src, size_t len);
 void vringh_abandon_kern(struct vringh *vrh, unsigned int num);
 int vringh_complete_kern(struct vringh *vrh, u16 head, u32 len);