Re: [PATCH v3] virtio: force spec specified alignment on types

[Michael S. Tsirkin]

On Tue, Apr 21, 2020 at 10:39:19AM +0800, Jason Wang wrote:
> 
> On 2020/4/21 äå4:46, Michael S. Tsirkin wrote:
> > The ring element addresses are passed between components with different
> > alignments assumptions. Thus, if guest/userspace selects a pointer and
> > host then gets and dereferences it, we might need to decrease the
> > compiler-selected alignment to prevent compiler on the host from
> > assuming pointer is aligned.
> > 
> > This actually triggers on ARM with -mabi=apcs-gnu - which is a
> > deprecated configuration, but it seems safer to handle this
> > generally.
> > 
> > Note that userspace that allocates the memory is actually OK and does
> > not need to be fixed, but userspace that gets it from guest or another
> > process does need to be fixed. The later doesn't generally talk to the
> > kernel so while it might be buggy it's not talking to the kernel in the
> > buggy way - it's just using the header in the buggy way - so fixing
> > header and asking userspace to recompile is the best we can do.
> > 
> > I verified that the produced kernel binary on x86 is exactly identical
> > before and after the change.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > ---
> > 
> > changes from v2:
> > 	add vring_used_elem_t to ensure alignment for substructures
> > changes from v1:
> > 	swicth all __user to the new typedefs
> > 
> >   drivers/vhost/vhost.c            |  8 +++---
> >   drivers/vhost/vhost.h            |  6 ++---
> >   drivers/vhost/vringh.c           |  6 ++---
> >   include/linux/vringh.h           |  6 ++---
> >   include/uapi/linux/virtio_ring.h | 43 ++++++++++++++++++++++++--------
> >   5 files changed, 45 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index d450e16c5c25..bc77b0f465fd 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1244,9 +1244,9 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access)
> >   }
> >   static bool vq_access_ok(struct vhost_virtqueue *vq, unsigned int num,
> > -			 struct vring_desc __user *desc,
> > -			 struct vring_avail __user *avail,
> > -			 struct vring_used __user *used)
> > +			 vring_desc_t __user *desc,
> > +			 vring_avail_t __user *avail,
> > +			 vring_used_t __user *used)
> >   {
> >   	return access_ok(desc, vhost_get_desc_size(vq, num)) &&
> > @@ -2301,7 +2301,7 @@ static int __vhost_add_used_n(struct vhost_virtqueue *vq,
> >   			    struct vring_used_elem *heads,
> >   			    unsigned count)
> >   {
> > -	struct vring_used_elem __user *used;
> > +	vring_used_elem_t __user *used;
> >   	u16 old, new;
> >   	int start;
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index f8403bd46b85..60cab4c78229 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -67,9 +67,9 @@ struct vhost_virtqueue {
> >   	/* The actual ring of buffers. */
> >   	struct mutex mutex;
> >   	unsigned int num;
> > -	struct vring_desc __user *desc;
> > -	struct vring_avail __user *avail;
> > -	struct vring_used __user *used;
> > +	vring_desc_t __user *desc;
> > +	vring_avail_t __user *avail;
> > +	vring_used_t __user *used;
> >   	const struct vhost_iotlb_map *meta_iotlb[VHOST_NUM_ADDRS];
> >   	struct file *kick;
> >   	struct eventfd_ctx *call_ctx;
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index ba8e0d6cfd97..e059a9a47cdf 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -620,9 +620,9 @@ static inline int xfer_to_user(const struct vringh *vrh,
> >    */
> >   int vringh_init_user(struct vringh *vrh, u64 features,
> >   		     unsigned int num, bool weak_barriers,
> > -		     struct vring_desc __user *desc,
> > -		     struct vring_avail __user *avail,
> > -		     struct vring_used __user *used)
> > +		     vring_desc_t __user *desc,
> > +		     vring_avail_t __user *avail,
> > +		     vring_used_t __user *used)
> >   {
> >   	/* Sane power of 2 please! */
> >   	if (!num || num > 0xffff || (num & (num - 1))) {
> > diff --git a/include/linux/vringh.h b/include/linux/vringh.h
> > index 9e2763d7c159..59bd50f99291 100644
> > --- a/include/linux/vringh.h
> > +++ b/include/linux/vringh.h
> > @@ -105,9 +105,9 @@ struct vringh_kiov {
> >   /* Helpers for userspace vrings. */
> >   int vringh_init_user(struct vringh *vrh, u64 features,
> >   		     unsigned int num, bool weak_barriers,
> > -		     struct vring_desc __user *desc,
> > -		     struct vring_avail __user *avail,
> > -		     struct vring_used __user *used);
> > +		     vring_desc_t __user *desc,
> > +		     vring_avail_t __user *avail,
> > +		     vring_used_t __user *used);
> >   static inline void vringh_iov_init(struct vringh_iov *iov,
> >   				   struct iovec *iovec, unsigned num)
> > diff --git a/include/uapi/linux/virtio_ring.h b/include/uapi/linux/virtio_ring.h
> > index 9223c3a5c46a..b2c20f794472 100644
> > --- a/include/uapi/linux/virtio_ring.h
> > +++ b/include/uapi/linux/virtio_ring.h
> > @@ -86,6 +86,13 @@
> >    * at the end of the used ring. Guest should ignore the used->flags field. */
> >   #define VIRTIO_RING_F_EVENT_IDX		29
> > +/* Alignment requirements for vring elements.
> > + * When using pre-virtio 1.0 layout, these fall out naturally.
> > + */
> > +#define VRING_AVAIL_ALIGN_SIZE 2
> > +#define VRING_USED_ALIGN_SIZE 4
> > +#define VRING_DESC_ALIGN_SIZE 16
> > +
> >   /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
> >   struct vring_desc {
> >   	/* Address (guest-physical). */
> > @@ -112,29 +119,43 @@ struct vring_used_elem {
> >   	__virtio32 len;
> >   };
> > +typedef struct vring_used_elem __aligned(VRING_USED_ALIGN_SIZE)
> > +	vring_used_elem_t;
> > +
> >   struct vring_used {
> >   	__virtio16 flags;
> >   	__virtio16 idx;
> > -	struct vring_used_elem ring[];
> > +	vring_used_elem_t ring[];
> >   };
> > +/*
> > + * The ring element addresses are passed between components with different
> > + * alignments assumptions. Thus, we might need to decrease the compiler-selected
> > + * alignment, and so must use a typedef to make sure the __aligned attribute
> > + * actually takes hold:
> > + *
> > + * https://gcc.gnu.org/onlinedocs//gcc/Common-Type-Attributes.html#Common-Type-Attributes
> > + *
> > + * When used on a struct, or struct member, the aligned attribute can only
> > + * increase the alignment; in order to decrease it, the packed attribute must
> > + * be specified as well. When used as part of a typedef, the aligned attribute
> > + * can both increase and decrease alignment, and specifying the packed
> > + * attribute generates a warning.
> > + */
> > +typedef struct vring_desc __aligned(VRING_DESC_ALIGN_SIZE) vring_desc_t;
> > +typedef struct vring_avail __aligned(VRING_AVAIL_ALIGN_SIZE) vring_avail_t;
> > +typedef struct vring_used __aligned(VRING_USED_ALIGN_SIZE) vring_used_t;
> 
> 
> I wonder whether we can simply use __attribute__(packed) instead?
> 
> Thanks

Packed is 1 byte alignment. As such generates very bad code for
accesses.


> 
> > +
> >   struct vring {
> >   	unsigned int num;
> > -	struct vring_desc *desc;
> > +	vring_desc_t *desc;
> > -	struct vring_avail *avail;
> > +	vring_avail_t *avail;
> > -	struct vring_used *used;
> > +	vring_used_t *used;
> >   };
> > -/* Alignment requirements for vring elements.
> > - * When using pre-virtio 1.0 layout, these fall out naturally.
> > - */
> > -#define VRING_AVAIL_ALIGN_SIZE 2
> > -#define VRING_USED_ALIGN_SIZE 4
> > -#define VRING_DESC_ALIGN_SIZE 16
> > -
> >   #ifndef VIRTIO_RING_NO_LEGACY
> >   /* The standard layout for the ring is a continuous chunk of memory which looks