Michael S. Tsirkin
2022-Aug-08 12:45 UTC
IOTLB support for vhost/vsock breaks crosvm on Android
On Mon, Aug 08, 2022 at 11:18:50AM +0100, Will Deacon wrote:> Hi Michael, > > On Sun, Aug 07, 2022 at 09:14:43AM -0400, Michael S. Tsirkin wrote: > > Will, thanks very much for the analysis and the writeup! > > No problem, and thanks for following up. > > > On Fri, Aug 05, 2022 at 07:11:06PM +0100, Will Deacon wrote: > > > So how should we fix this? One possibility is for us to hack crosvm to > > > clear the VIRTIO_F_ACCESS_PLATFORM flag when setting the vhost features, > > > but others here have reasonably pointed out that they didn't expect a > > > kernel change to break userspace. On the flip side, the offending commit > > > in the kernel isn't exactly new (it's from the end of 2020!) and so it's > > > likely that others (e.g. QEMU) are using this feature. > > > > Exactly, that's the problem. > > > > vhost is reusing the virtio bits and it's only natural that > > what you are doing would happen. > > > > To be precise, this is what we expected people to do (and what QEMU does): > > > > > > #define QEMU_VHOST_FEATURES ((1 << VIRTIO_F_VERSION_1) | > > (1 << VIRTIO_NET_F_RX_MRG) | .... ) > > > > VHOST_GET_FEATURES(... &host_features); > > host_features &= QEMU_VHOST_FEATURES > > VHOST_SET_FEATURES(host_features & guest_features) > > > > > > Here QEMU_VHOST_FEATURES are the bits userspace knows about. > > > > Our assumption was that whatever userspace enables, it > > knows what the effect on vhost is going to be. > > > > But yes, I understand absolutely how someone would instead just use the > > guest features. It is unfortunate that we did not catch this in time. > > > > > > In hindsight, we should have just created vhost level macros > > instead of reusing virtio ones. Would address the concern > > about naming: PLATFORM_ACCESS makes sense for the > > guest since there it means "whatever access rules platform has", > > but for vhost a better name would be VHOST_F_IOTLB. > > We should have also taken greater pains to document what > > we expect userspace to do. I remember now how I thought about something > > like this but after coding this up in QEMU I forgot to document this :( > > Also, I suspect given the history the GET/SET features ioctl and burned > > wrt extending it and we have to use a new when we add new features. > > All this we can do going forward. > > Makes sense. The crosvm developers are also pretty friendly in my > experience, so I'm sure they wouldn't mind being involved in discussions > around any future ABI extensions. Just be aware that they _very_ recently > moved their mailing lists, so I think it lives here now: > > https://groups.google.com/a/chromium.org/g/crosvm-dev > > > But what can we do about the specific issue? > > I am not 100% sure since as Will points out, QEMU and other > > userspace already rely on the current behaviour. > > > > Looking at QEMU specifically, it always sends some translations at > > startup, this in order to handle device rings. > > > > So, *maybe* we can get away with assuming that if no IOTLB ioctl was > > ever invoked then this userspace does not know about IOTLB and > > translation should ignore IOTLB completely. > > There was a similar suggestion from Stefano: > > https://lore.kernel.org/r/20220806105225.crkui6nw53kbm5ge at sgarzare-redhat > > about spotting the backend ioctl for IOTLB and using that to enable > the negotiation of F_ACCESS_PLATFORM. Would that work for qemu?Hmm I would worry that this disables the feature for old QEMU :(> > I am a bit nervous about breaking some *other* userspace which actually > > wants device to be blocked from accessing memory until IOTLB > > has been setup. If we get it wrong we are making guest > > and possibly even host vulnerable. > > And of course just revering is not an option either since there > > are now whole stacks depending on the feature. > > Absolutely, I'm not seriously suggesting the revert. I just did it locally > to confirm the issue I was seeing. > > > Will I'd like your input on whether you feel a hack in the kernel > > is justified here. > > If we can come up with something that we have confidence in and won't be a > pig to maintain, then I think we should do it, but otherwise we can go ahead > and change crosvm to mask out this feature flag on the vhost side for now. > We mainly wanted to raise the issue to illustrate that this flag continues > to attract problems in the hope that it might inform further usage and/or > spec work in this area. > > In any case, I'm happy to test any kernel patches with our setup if you > want to give it a shot.Thanks! I'm a bit concerned that the trick I proposed changes the configuration where iotlb was not set up from "access to memory not allowed" to "access to all memory allowed". This just might have security implications if some application assumed the former. And the one Stefano proposed disables IOTLB for old QEMU versions.> > Also yes, I think it's a good idea to change crosvm anyway. While the > > work around I describe might make sense upstream I don't think it's a > > reasonable thing to do in stable kernels. > > I think I'll prepare a patch documenting the legal vhost features > > as a 1st step even though crosvm is rust so it's not importing > > the header directly, right? > > Documentation is a good idea regardless, so thanks for that. Even though > crosvm has its own bindings for the vhost ioctl()s, the documentation > can be reference or duplicated once it's available in the kernel headers. > > WillSo for crosvm change, I will post the documentation change and you guys can discuss? -- MST
On Mon, Aug 8, 2022 at 8:45 PM Michael S. Tsirkin <mst at redhat.com> wrote:> > On Mon, Aug 08, 2022 at 11:18:50AM +0100, Will Deacon wrote: > > Hi Michael, > > > > On Sun, Aug 07, 2022 at 09:14:43AM -0400, Michael S. Tsirkin wrote: > > > Will, thanks very much for the analysis and the writeup! > > > > No problem, and thanks for following up. > > > > > On Fri, Aug 05, 2022 at 07:11:06PM +0100, Will Deacon wrote: > > > > So how should we fix this? One possibility is for us to hack crosvm to > > > > clear the VIRTIO_F_ACCESS_PLATFORM flag when setting the vhost features, > > > > but others here have reasonably pointed out that they didn't expect a > > > > kernel change to break userspace. On the flip side, the offending commit > > > > in the kernel isn't exactly new (it's from the end of 2020!) and so it's > > > > likely that others (e.g. QEMU) are using this feature. > > > > > > Exactly, that's the problem. > > > > > > vhost is reusing the virtio bits and it's only natural that > > > what you are doing would happen. > > > > > > To be precise, this is what we expected people to do (and what QEMU does): > > > > > > > > > #define QEMU_VHOST_FEATURES ((1 << VIRTIO_F_VERSION_1) | > > > (1 << VIRTIO_NET_F_RX_MRG) | .... ) > > > > > > VHOST_GET_FEATURES(... &host_features); > > > host_features &= QEMU_VHOST_FEATURES > > > VHOST_SET_FEATURES(host_features & guest_features) > > > > > > > > > Here QEMU_VHOST_FEATURES are the bits userspace knows about. > > > > > > Our assumption was that whatever userspace enables, it > > > knows what the effect on vhost is going to be. > > > > > > But yes, I understand absolutely how someone would instead just use the > > > guest features. It is unfortunate that we did not catch this in time. > > > > > > > > > In hindsight, we should have just created vhost level macros > > > instead of reusing virtio ones. Would address the concern > > > about naming: PLATFORM_ACCESS makes sense for the > > > guest since there it means "whatever access rules platform has", > > > but for vhost a better name would be VHOST_F_IOTLB. > > > We should have also taken greater pains to document what > > > we expect userspace to do. I remember now how I thought about something > > > like this but after coding this up in QEMU I forgot to document this :( > > > Also, I suspect given the history the GET/SET features ioctl and burned > > > wrt extending it and we have to use a new when we add new features. > > > All this we can do going forward. > > > > Makes sense. The crosvm developers are also pretty friendly in my > > experience, so I'm sure they wouldn't mind being involved in discussions > > around any future ABI extensions. Just be aware that they _very_ recently > > moved their mailing lists, so I think it lives here now: > > > > https://groups.google.com/a/chromium.org/g/crosvm-dev > > > > > But what can we do about the specific issue? > > > I am not 100% sure since as Will points out, QEMU and other > > > userspace already rely on the current behaviour. > > > > > > Looking at QEMU specifically, it always sends some translations at > > > startup, this in order to handle device rings. > > > > > > So, *maybe* we can get away with assuming that if no IOTLB ioctl was > > > ever invoked then this userspace does not know about IOTLB and > > > translation should ignore IOTLB completely. > > > > There was a similar suggestion from Stefano: > > > > https://lore.kernel.org/r/20220806105225.crkui6nw53kbm5ge at sgarzare-redhat > > > > about spotting the backend ioctl for IOTLB and using that to enable > > the negotiation of F_ACCESS_PLATFORM. Would that work for qemu? > > Hmm I would worry that this disables the feature for old QEMU :( > > > > > I am a bit nervous about breaking some *other* userspace which actually > > > wants device to be blocked from accessing memory until IOTLB > > > has been setup. If we get it wrong we are making guest > > > and possibly even host vulnerable. > > > And of course just revering is not an option either since there > > > are now whole stacks depending on the feature. > > > > Absolutely, I'm not seriously suggesting the revert. I just did it locally > > to confirm the issue I was seeing. > > > > > Will I'd like your input on whether you feel a hack in the kernel > > > is justified here. > > > > If we can come up with something that we have confidence in and won't be a > > pig to maintain, then I think we should do it, but otherwise we can go ahead > > and change crosvm to mask out this feature flag on the vhost side for now. > > We mainly wanted to raise the issue to illustrate that this flag continues > > to attract problems in the hope that it might inform further usage and/or > > spec work in this area. > > > > In any case, I'm happy to test any kernel patches with our setup if you > > want to give it a shot. > > Thanks! > I'm a bit concerned that the trick I proposed changes the configuration > where iotlb was not set up from "access to memory not allowed" to > "access to all memory allowed". This just might have security > implications if some application assumed the former. > And the one Stefano proposed disables IOTLB for old QEMU versions.Yes, if there is no choice, having some known cases that are broken is better than silently breaking unknown setups. Thanks> > > > > > Also yes, I think it's a good idea to change crosvm anyway. While the > > > work around I describe might make sense upstream I don't think it's a > > > reasonable thing to do in stable kernels. > > > I think I'll prepare a patch documenting the legal vhost features > > > as a 1st step even though crosvm is rust so it's not importing > > > the header directly, right? > > > > Documentation is a good idea regardless, so thanks for that. Even though > > crosvm has its own bindings for the vhost ioctl()s, the documentation > > can be reference or duplicated once it's available in the kernel headers. > > > > Will > > So for crosvm change, I will post the documentation change and > you guys can discuss? > > -- > MST >
On Mon, Aug 08, 2022 at 08:45:48AM -0400, Michael S. Tsirkin wrote:> > > Also yes, I think it's a good idea to change crosvm anyway. While the > > > work around I describe might make sense upstream I don't think it's a > > > reasonable thing to do in stable kernels. > > > I think I'll prepare a patch documenting the legal vhost features > > > as a 1st step even though crosvm is rust so it's not importing > > > the header directly, right? > > > > Documentation is a good idea regardless, so thanks for that. Even though > > crosvm has its own bindings for the vhost ioctl()s, the documentation > > can be reference or duplicated once it's available in the kernel headers. > > > So for crosvm change, I will post the documentation change and > you guys can discuss?FYI, the crosvm patch is merged here: https://github.com/google/crosvm/commit/4e7d00be2e135b0a2d964320ea4276e5d896f426 Will