On Wed, Jan 05, 2022 at 04:06:49PM +0100, Ján Tomko wrote: > Add a new attribute to control the killpriv feature: > > <filesystem> > ... > <binary> > <killpriv v2='off'/> > </binary> > </filesystem> Ewww, ewww, ewww. This is a horrible element & attribute name. Even with the docs I have little clue as to why we would ever need this or what its real effects are. Having now read the libvirt and related QEMU bugzilla, I now see that killpriv v2 is enabled by default in virtiofs because it is faster and more reliable. A limitation in NFS, however, means that some specific syscalls don't work correctly with it, so QEMU folks are saying killpriv v2 should not be used with NFS. At this point I ask why doesn't virtiofsd just do the right thing. It can statfs() the root of the export it is configured with and see that it is NFS. I'm not convinced by a need to expose this knob in libvirt, especially with such an unintelligible name & difficult to understand behavioural impact. > > https://bugzilla.redhat.com/show_bug.cgi?id=1972571 > > Signed-off-by: Ján Tomko <jtomko@xxxxxxxxxx> > --- > docs/formatdomain.rst | 4 ++++ > docs/schemas/domaincommon.rng | 7 +++++++ > src/conf/domain_conf.c | 15 +++++++++++++++ > src/conf/domain_conf.h | 1 + > .../qemuxml2argvdata/vhost-user-fs-fd-memory.xml | 1 + > 5 files changed, 28 insertions(+) > > diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst > index d4f30bb8af..73ff8bce51 100644 > --- a/docs/formatdomain.rst > +++ b/docs/formatdomain.rst > @@ -3313,6 +3313,7 @@ A directory on the host that can be accessed directly from the guest. > <binary path='/usr/libexec/virtiofsd' xattr='on'> > <cache mode='always'/> > <sandbox mode='namespace'/> > + <killpriv v2='on'/> > <lock posix='on' flock='on'/> > </binary> > <source dir='/path'/> > @@ -3447,6 +3448,9 @@ A directory on the host that can be accessed directly from the guest. > ``chroot``, see the > `virtiofsd documentation <https://qemu.readthedocs.io/en/latest/tools/virtiofsd.html>`__ > for more details. ( :since:`Since 7.2.0` ) > + The ``killpriv`` element with the attribute ``v2`` (values: ``on`` or ``off``) > + can be used to disable the killpriv capability which is used to improve performance > + by expecting writes to reset some security attributes. ( :since:`Since 8.0.0` ) > ``source`` > The resource on the host that is being accessed in the guest. The ``name`` > attribute must be used with ``type='template'``, and the ``dir`` attribute > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 7fa5c2b8b5..5701bbe193 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -3057,6 +3057,13 @@ > </optional> > </element> > </optional> > + <optional> > + <element name="killpriv"> > + <attribute name="v2"> > + <ref name="virOnOff"/> > + </attribute> > + </element> > + </optional> > </interleave> > </element> > </define> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 5691b8d2d5..2a1802a3d5 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -9874,6 +9874,7 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, > g_autofree char *sandbox = virXPathString("string(./binary/sandbox/@mode)", ctxt); > g_autofree char *posix_lock = virXPathString("string(./binary/lock/@posix)", ctxt); > g_autofree char *flock = virXPathString("string(./binary/lock/@flock)", ctxt); > + g_autofree char *killpriv_v2 = virXPathString("string(./binary/killpriv/@v2)", ctxt); > int val; > > if (queue_size && virStrToLong_ull(queue_size, NULL, 10, &def->queue_size) < 0) { > @@ -9932,6 +9933,15 @@ virDomainFSDefParseXML(virDomainXMLOption *xmlopt, > } > def->flock = val; > } > + > + if (killpriv_v2) { > + if ((val = virTristateSwitchTypeFromString(killpriv_v2)) <= 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown killpriv_v2 value '%s'"), killpriv_v2); > + goto error; > + } > + def->killpriv_v2 = val; > + } > } > > if (source == NULL && def->type != VIR_DOMAIN_FS_TYPE_RAM > @@ -24197,6 +24207,11 @@ virDomainFSDefFormat(virBuffer *buf, > virTristateSwitchTypeToString(def->flock)); > } > > + if (def->killpriv_v2 != VIR_TRISTATE_SWITCH_ABSENT) { > + virBufferAsprintf(&binaryBuf, "<killpriv v2='%s'/>\n", > + virTristateSwitchTypeToString(def->killpriv_v2)); > + } > + > virXMLFormatElement(&binaryBuf, "lock", &lockAttrBuf, NULL); > } > > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 144ba4dd12..4619fcbfd1 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -892,6 +892,7 @@ struct _virDomainFSDef { > virTristateSwitch posix_lock; > virTristateSwitch flock; > virDomainFSSandboxMode sandbox; > + virTristateSwitch killpriv_v2; > virDomainVirtioOptions *virtio; > virObject *privateData; > }; > diff --git a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml > index abddf0870b..2f44a1593a 100644 > --- a/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml > +++ b/tests/qemuxml2argvdata/vhost-user-fs-fd-memory.xml > @@ -31,6 +31,7 @@ > <binary path='/usr/libexec/virtiofsd' xattr='on'> > <cache mode='always'/> > <sandbox mode='chroot'/> > + <killpriv v2='on'/> > <lock posix='off' flock='off'/> > </binary> > <source dir='/path'/> > -- > 2.31.1 > Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
