On 2/14/25 9:21 AM, Andrea Bolognani wrote:
On Fri, Feb 14, 2025 at 08:49:29AM -0500, Laine Stump wrote:
On 2/14/25 6:03 AM, Andrea Bolognani wrote:
On Thu, Feb 13, 2025 at 01:19:53PM -0500, Laine Stump wrote:
The result is that you can now have:
<interface type='vhostuser'>
<backend type='passt'/>
...
</interface>
Then as long as you also have the following as a subelement of
<domain>:
<memoryBacking>
<access mode='shared'/>
</memoryBacking>
your passt interfaces will benefit from the greatly improved
efficiency of a vhost-user data path
The presence of this element is not validated anywhere though, so if
you leave it out the network interface will just silently be
non-functional. I don't think that makes for a good user experience.
Keeping in mind that this is a pre-existing requirement for a pre-existing
functionality that's been there since 2014...
My assumption has been that whatever type='vhostuser' already does is "good
enough", and at some point during testing of uncompleted code, one of the
unit tests produced an error saying something about "you've selected a
feature that requires shared memory but don't have shared memory turned on;
your configuration may not work correctly" or something like that.
Because I saw that during a failed unit test (which was later fixed) I
figured that must be reported when a vhost-user config doesn't have shared
memory enabled (and didn't think to actually try it), but just now I tried
making a vhost-user config without shared memory enabled, and it didn't
produce any error in libvirt at virsh edit time, nor did either QEMU or
passt log any error (that I can see, althouygh maybe I'm not looking in all
the right places), but it did produce a non-working interface for the guest!
I agree with you that it doesn't "make for a good user experience" and
something should be done about it. I had considered having shared memory
turned on automatically for type='vhostuser', but anything about "shared
memory" sets security red flags ringing (or some other mixed metaphor) so
I've assumed that's why they didn't already do this for traditional
vhost-user interfaces. (I did ask about this online, but sbrivio was the
only one who answered (saying a variation of the above)) (admittedly it was
late in the day for me, so anybody in a further east timezone was probably
already offline (except sbrivio, who seems to not sleep until after it is
sunrise in Australia for some strange reason ;-)).
As a followup, I'd be happy to make a patch that causes a config with a
vhost-user interface but no shared mem enabled to fail validation. That
shouldn't hold up what's here though - as I said above, it's a preexisting
condition (I don't know if you were actually suggesting that, but just want
to "head it off at the pass" if you were :-))
I was pretty much requesting that change be made, actually :)
I have no idea about how things work in the pre-existing vhost-user
scenarios, but vhost-user passt is obviously broken when shared
memory is disabled, so we should simply not allow it. I see no reason
to introduce a new feature with known flaws, especially when adding a
check for this should be completely trivial.
Yeah, sure sure sure, okay :-P
You can restrict the check to just the vhost-user passt case, leaving
other vhost-user cases alone, so that we don't have to worry about
accidentally breaking existing configurations.
I will also send a separate patch that removes the "passt-only" aspect
and does it for vhost-user as well; since validation isn't run on
existing configs, and any existing config that had vhost-user but no
shared memory would have previously failed to function anyway, I don't
see much danger in validating it for plain vhost-user as well.
Let's not even
consider enabling shared memory automatically for now, because as you
say that's potentially opening a big can of worm.
Yeah, it would be convenient, and *might not* even be all that unsafe,
but I certainly can't say that certain thing with any certainty (hey, if
sbrivio can do it so can I!) and I wouldn't want to be the one
responsible for some new exploit.
You need to respin to add the documentation anyway, don't you?
If there weren't any other major problems, I wasn't planning to resend
the entire series for that, just add a "Patch 10/9".
