vhost is currently broken on the some ARM configs.
The reason is that the ring element addresses are passed between
components with different alignments assumptions. Thus, if
guest selects a pointer and host then gets and dereferences
it, then alignment assumed by the host's compiler might be
greater than the actual alignment of the pointer.
compiler on the host from assuming pointer is aligned.
This actually triggers on ARM with -mabi=apcs-gnu - which is a
deprecated configuration. With this OABI, compiler assumes that
all structures are 4 byte aligned - which is stronger than
virtio guarantees for available and used rings, which are
merely 2 bytes. Thus a guest without -mabi=apcs-gnu running
on top of host with -mabi=apcs-gnu will be broken.
The correct fix is to force alignment of structures - however
that is an intrusive fix that's best deferred until the next release.
We didn't previously support such ancient systems at all - this surfaced
after vdpa support prompted removing dependency of vhost on
VIRTULIZATION. So for now, let's just add something along the lines of
depends on !ARM || AEABI
to the virtio Kconfig declaration, and add a comment that it has to do
with struct member alignment.
Note: we can't make VHOST and VHOST_RING themselves have
a dependency since these are selected. Add a new symbol for that.
We should be able to drop this dependency down the road.
Fixes: 20c384f1ea1a0bc7 ("vhost: refine vhost and vringh kconfig")
Suggested-by: Ard Biesheuvel <ardb at kernel.org>
Suggested-by: Richard Earnshaw <Richard.Earnshaw at arm.com>
Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
---
changes from v3:
update commit log clarifying the motivation and that
it's a temporary fix.
suggested by Christoph Hellwig
drivers/misc/mic/Kconfig | 2 +-
drivers/net/caif/Kconfig | 2 +-
drivers/vdpa/Kconfig | 2 +-
drivers/vhost/Kconfig | 17 +++++++++++++----
4 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/mic/Kconfig b/drivers/misc/mic/Kconfig
index 8f201d019f5a..3bfe72c59864 100644
--- a/drivers/misc/mic/Kconfig
+++ b/drivers/misc/mic/Kconfig
@@ -116,7 +116,7 @@ config MIC_COSM
config VOP
tristate "VOP Driver"
- depends on VOP_BUS
+ depends on VOP_BUS && VHOST_DPN
select VHOST_RING
select VIRTIO
help
diff --git a/drivers/net/caif/Kconfig b/drivers/net/caif/Kconfig
index 9db0570c5beb..661c25eb1c46 100644
--- a/drivers/net/caif/Kconfig
+++ b/drivers/net/caif/Kconfig
@@ -50,7 +50,7 @@ config CAIF_HSI
config CAIF_VIRTIO
tristate "CAIF virtio transport driver"
- depends on CAIF && HAS_DMA
+ depends on CAIF && HAS_DMA && VHOST_DPN
select VHOST_RING
select VIRTIO
select GENERIC_ALLOCATOR
diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig
index 3e1ceb8e9f2b..e8140065c8a5 100644
--- a/drivers/vdpa/Kconfig
+++ b/drivers/vdpa/Kconfig
@@ -10,7 +10,7 @@ if VDPA
config VDPA_SIM
tristate "vDPA device simulator"
- depends on RUNTIME_TESTING_MENU && HAS_DMA
+ depends on RUNTIME_TESTING_MENU && HAS_DMA && VHOST_DPN
select VHOST_RING
default n
help
diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
index 2c75d164b827..c4f273793595 100644
--- a/drivers/vhost/Kconfig
+++ b/drivers/vhost/Kconfig
@@ -13,6 +13,15 @@ config VHOST_RING
This option is selected by any driver which needs to access
the host side of a virtio ring.
+config VHOST_DPN
+ bool
+ depends on !ARM || AEABI
+ default y
+ help
+ Anything selecting VHOST or VHOST_RING must depend on VHOST_DPN.
+ This excludes the deprecated ARM ABI since that forces a 4 byte
+ alignment on all structs - incompatible with virtio spec requirements.
+
config VHOST
tristate
select VHOST_IOTLB
@@ -28,7 +37,7 @@ if VHOST_MENU
config VHOST_NET
tristate "Host kernel accelerator for virtio net"
- depends on NET && EVENTFD && (TUN || !TUN) && (TAP ||
!TAP)
+ depends on NET && EVENTFD && (TUN || !TUN) && (TAP ||
!TAP) && VHOST_DPN
select VHOST
---help---
This kernel module can be loaded in host kernel to accelerate
@@ -40,7 +49,7 @@ config VHOST_NET
config VHOST_SCSI
tristate "VHOST_SCSI TCM fabric driver"
- depends on TARGET_CORE && EVENTFD
+ depends on TARGET_CORE && EVENTFD && VHOST_DPN
select VHOST
default n
---help---
@@ -49,7 +58,7 @@ config VHOST_SCSI
config VHOST_VSOCK
tristate "vhost virtio-vsock driver"
- depends on VSOCKETS && EVENTFD
+ depends on VSOCKETS && EVENTFD && VHOST_DPN
select VHOST
select VIRTIO_VSOCKETS_COMMON
default n
@@ -63,7 +72,7 @@ config VHOST_VSOCK
config VHOST_VDPA
tristate "Vhost driver for vDPA-based backend"
- depends on EVENTFD
+ depends on EVENTFD && VHOST_DPN
select VHOST
depends on VDPA
help
--
MST
On 2020/4/20 ??10:34, Michael S. Tsirkin wrote:> vhost is currently broken on the some ARM configs. > > The reason is that the ring element addresses are passed between > components with different alignments assumptions. Thus, if > guest selects a pointer and host then gets and dereferences > it, then alignment assumed by the host's compiler might be > greater than the actual alignment of the pointer. > compiler on the host from assuming pointer is aligned. > > This actually triggers on ARM with -mabi=apcs-gnu - which is a > deprecated configuration. With this OABI, compiler assumes that > all structures are 4 byte aligned - which is stronger than > virtio guarantees for available and used rings, which are > merely 2 bytes. Thus a guest without -mabi=apcs-gnu running > on top of host with -mabi=apcs-gnu will be broken. > > The correct fix is to force alignment of structures - however > that is an intrusive fix that's best deferred until the next release. > > We didn't previously support such ancient systems at all - this surfaced > after vdpa support prompted removing dependency of vhost on > VIRTULIZATION. So for now, let's just add something along the lines of > > depends on !ARM || AEABI > > to the virtio Kconfig declaration, and add a comment that it has to do > with struct member alignment. > > Note: we can't make VHOST and VHOST_RING themselves have > a dependency since these are selected. Add a new symbol for that. > > We should be able to drop this dependency down the road. > > Fixes: 20c384f1ea1a0bc7 ("vhost: refine vhost and vringh kconfig") > Suggested-by: Ard Biesheuvel <ardb at kernel.org> > Suggested-by: Richard Earnshaw <Richard.Earnshaw at arm.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > --- > > changes from v3: > update commit log clarifying the motivation and that > it's a temporary fix. > > suggested by Christoph Hellwig > > drivers/misc/mic/Kconfig | 2 +- > drivers/net/caif/Kconfig | 2 +- > drivers/vdpa/Kconfig | 2 +- > drivers/vhost/Kconfig | 17 +++++++++++++---- > 4 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/misc/mic/Kconfig b/drivers/misc/mic/Kconfig > index 8f201d019f5a..3bfe72c59864 100644 > --- a/drivers/misc/mic/Kconfig > +++ b/drivers/misc/mic/Kconfig > @@ -116,7 +116,7 @@ config MIC_COSM > > config VOP > tristate "VOP Driver" > - depends on VOP_BUS > + depends on VOP_BUS && VHOST_DPN > select VHOST_RING > select VIRTIO > help > diff --git a/drivers/net/caif/Kconfig b/drivers/net/caif/Kconfig > index 9db0570c5beb..661c25eb1c46 100644 > --- a/drivers/net/caif/Kconfig > +++ b/drivers/net/caif/Kconfig > @@ -50,7 +50,7 @@ config CAIF_HSI > > config CAIF_VIRTIO > tristate "CAIF virtio transport driver" > - depends on CAIF && HAS_DMA > + depends on CAIF && HAS_DMA && VHOST_DPN > select VHOST_RING > select VIRTIO > select GENERIC_ALLOCATOR > diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig > index 3e1ceb8e9f2b..e8140065c8a5 100644 > --- a/drivers/vdpa/Kconfig > +++ b/drivers/vdpa/Kconfig > @@ -10,7 +10,7 @@ if VDPA > > config VDPA_SIM > tristate "vDPA device simulator" > - depends on RUNTIME_TESTING_MENU && HAS_DMA > + depends on RUNTIME_TESTING_MENU && HAS_DMA && VHOST_DPN > select VHOST_RING > default n > help > diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig > index 2c75d164b827..c4f273793595 100644 > --- a/drivers/vhost/Kconfig > +++ b/drivers/vhost/Kconfig > @@ -13,6 +13,15 @@ config VHOST_RING > This option is selected by any driver which needs to access > the host side of a virtio ring. > > +config VHOST_DPN > + bool > + depends on !ARM || AEABI > + default y > + help > + Anything selecting VHOST or VHOST_RING must depend on VHOST_DPN. > + This excludes the deprecated ARM ABI since that forces a 4 byte > + alignment on all structs - incompatible with virtio spec requirements. > + > config VHOST > tristate > select VHOST_IOTLB > @@ -28,7 +37,7 @@ if VHOST_MENU > > config VHOST_NET > tristate "Host kernel accelerator for virtio net" > - depends on NET && EVENTFD && (TUN || !TUN) && (TAP || !TAP) > + depends on NET && EVENTFD && (TUN || !TUN) && (TAP || !TAP) && VHOST_DPN > select VHOST > ---help--- > This kernel module can be loaded in host kernel to accelerate > @@ -40,7 +49,7 @@ config VHOST_NET > > config VHOST_SCSI > tristate "VHOST_SCSI TCM fabric driver" > - depends on TARGET_CORE && EVENTFD > + depends on TARGET_CORE && EVENTFD && VHOST_DPN > select VHOST > default n > ---help--- > @@ -49,7 +58,7 @@ config VHOST_SCSI > > config VHOST_VSOCK > tristate "vhost virtio-vsock driver" > - depends on VSOCKETS && EVENTFD > + depends on VSOCKETS && EVENTFD && VHOST_DPN > select VHOST > select VIRTIO_VSOCKETS_COMMON > default n > @@ -63,7 +72,7 @@ config VHOST_VSOCK > > config VHOST_VDPA > tristate "Vhost driver for vDPA-based backend" > - depends on EVENTFD > + depends on EVENTFD && VHOST_DPN > select VHOST > depends on VDPA > helpAcked-by: Jason Wang <jasowang at redhat.com> Thanks
Hi Michael, Thanks for your patch! On Mon, Apr 20, 2020 at 5:13 PM Michael S. Tsirkin <mst at redhat.com> wrote:> vhost is currently broken on the some ARM configs. > > The reason is that the ring element addresses are passed between > components with different alignments assumptions. Thus, if > guest selects a pointer and host then gets and dereferences > it, then alignment assumed by the host's compiler might be > greater than the actual alignment of the pointer. > compiler on the host from assuming pointer is aligned. > > This actually triggers on ARM with -mabi=apcs-gnu - which is a > deprecated configuration. With this OABI, compiler assumes that > all structures are 4 byte aligned - which is stronger than > virtio guarantees for available and used rings, which are > merely 2 bytes. Thus a guest without -mabi=apcs-gnu running > on top of host with -mabi=apcs-gnu will be broken. > > The correct fix is to force alignment of structures - however > that is an intrusive fix that's best deferred until the next release. > > We didn't previously support such ancient systems at all - this surfaced > after vdpa support prompted removing dependency of vhost on > VIRTULIZATION. So for now, let's just add something along the lines of > > depends on !ARM || AEABI > > to the virtio Kconfig declaration, and add a comment that it has to do > with struct member alignment. > > Note: we can't make VHOST and VHOST_RING themselves have > a dependency since these are selected. Add a new symbol for that.Adding the dependencies to VHOST and VHOST_RING themselves is indeed not sufficient. But IMHO you should still add VHOST_DPN dependencies t these two symbols, so any driver selecting them without fulfilling the VHOST_DPN dependency will trigger a Kconfig warning. Else the issue will be ignored silently.> We should be able to drop this dependency down the road. > > Fixes: 20c384f1ea1a0bc7 ("vhost: refine vhost and vringh kconfig") > Suggested-by: Ard Biesheuvel <ardb at kernel.org> > Suggested-by: Richard Earnshaw <Richard.Earnshaw at arm.com> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Apr 27, 2020 at 08:45:22AM +0200, Geert Uytterhoeven wrote:> Hi Michael, > > Thanks for your patch! > > On Mon, Apr 20, 2020 at 5:13 PM Michael S. Tsirkin <mst at redhat.com> wrote: > > vhost is currently broken on the some ARM configs. > > > > The reason is that the ring element addresses are passed between > > components with different alignments assumptions. Thus, if > > guest selects a pointer and host then gets and dereferences > > it, then alignment assumed by the host's compiler might be > > greater than the actual alignment of the pointer. > > compiler on the host from assuming pointer is aligned. > > > > This actually triggers on ARM with -mabi=apcs-gnu - which is a > > deprecated configuration. With this OABI, compiler assumes that > > all structures are 4 byte aligned - which is stronger than > > virtio guarantees for available and used rings, which are > > merely 2 bytes. Thus a guest without -mabi=apcs-gnu running > > on top of host with -mabi=apcs-gnu will be broken. > > > > The correct fix is to force alignment of structures - however > > that is an intrusive fix that's best deferred until the next release. > > > > We didn't previously support such ancient systems at all - this surfaced > > after vdpa support prompted removing dependency of vhost on > > VIRTULIZATION. So for now, let's just add something along the lines of > > > > depends on !ARM || AEABI > > > > to the virtio Kconfig declaration, and add a comment that it has to do > > with struct member alignment. > > > > Note: we can't make VHOST and VHOST_RING themselves have > > a dependency since these are selected. Add a new symbol for that. > > Adding the dependencies to VHOST and VHOST_RING themselves is indeed not > sufficient. But IMHO you should still add VHOST_DPN dependencies t > these two symbols, so any driver selecting them without fulfilling the > VHOST_DPN dependency will trigger a Kconfig warning. Else the > issue will be ignored silently.Good point. For now I'm trying to just get rid of this work around. If I can't I will add the suggested change. Thanks!> > We should be able to drop this dependency down the road. > > > > Fixes: 20c384f1ea1a0bc7 ("vhost: refine vhost and vringh kconfig") > > Suggested-by: Ard Biesheuvel <ardb at kernel.org> > > Suggested-by: Richard Earnshaw <Richard.Earnshaw at arm.com> > > Signed-off-by: Michael S. Tsirkin <mst at redhat.com> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds