From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
Subject: Re: [PATCH v3] virtio: force spec specified alignment on types
Date: Tue, 21 Apr 2020 09:25:15 -0400 [thread overview]
Message-ID: <20200421092418-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <a4939aeb-ed9d-a6af-1c70-c6c2513e86e2@redhat.com>
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@redhat.com>
> > ---
> >
> > 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
prev parent reply other threads:[~2020-04-21 13:25 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-20 20:46 [PATCH v3] virtio: force spec specified alignment on types Michael S. Tsirkin
2020-04-21 2:39 ` Jason Wang
2020-04-21 13:25 ` Michael S. Tsirkin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200421092418-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=jasowang@redhat.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).