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".