9:06 a.m.
Ján Tomko (2):
conf: add killpriv v2 attribute for virtiofs
qemu: virtiofs: add (no_)killpriv_v2 support
docs/formatdomain.rst | 4 ++++
docs/schemas/domaincommon.rng | 7 +++++++
src/conf/domain_conf.c | 15 +++++++++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_virtiofs.c | 5 +++++
.../qemuxml2argvdata/vhost-user-fs-fd-memory.xml | 1 +
6 files changed, 33 insertions(+)
--
2.31.1
9:06 a.m.
New subject: [libvirt PATCH 1/2] conf: add killpriv v2 attribute for virtiofs
Add a new attribute to control the killpriv feature:
<filesystem>
...
<binary>
<killpriv v2='off'/>
</binary>
</filesystem>
https://bugzilla.redhat.com/show_bug.cgi?id=1972571
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
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
9:30 a.m.
New subject: [libvirt PATCH 1/2] conf: add killpriv v2 attribute for virtiofs
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(a)redhat.com>
---
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 :|
9:27 a.m.
New subject: [libvirt PATCH 1/2] conf: add killpriv v2 attribute for virtiofs
On a Wednesday in 2022, Daniel P. Berrangé wrote:
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.
Thank you for the short summary of the bug, I did not realize it was
private :(
Jano
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(a)redhat.com>
> ---
> 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(+)
9:06 a.m.
New subject: [libvirt PATCH 2/2] qemu: virtiofs: add (no_)killpriv_v2 support
https://bugzilla.redhat.com/show_bug.cgi?id=1972571
Signed-off-by: Ján Tomko <jtomko(a)redhat.com>
---
src/qemu/qemu_virtiofs.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 1b853a5a59..c89da76c27 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -162,6 +162,11 @@ qemuVirtioFSBuildCommandLine(virQEMUDriverConfig *cfg,
else if (fs->posix_lock == VIR_TRISTATE_SWITCH_OFF)
virBufferAddLit(&opts, ",no_posix_lock");
+ if (fs->killpriv_v2 == VIR_TRISTATE_SWITCH_ON)
+ virBufferAddLit(&opts, ",killpriv_v2");
+ else if (fs->killpriv_v2 == VIR_TRISTATE_SWITCH_OFF)
+ virBufferAddLit(&opts, ",no_killpriv_v2");
+
virCommandAddArgBuffer(cmd, &opts);
if (cfg->virtiofsdDebug)
virCommandAddArg(cmd, "-d");
--
2.31.1
1051
days inactive
1052
days old
4 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Ján Tomko