[libvirt] [PATCH 0/2] fix labeling for chardev source path
Pavel Hrdina (2): conf: don't iterate over backcompat console in virDomainChrDefForeach qemu: don't relabel chardev source file if virtlogd is used src/conf/domain_conf.c | 46 ++++++++++++++++++++++++++++++++++++----- src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 +++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 16 ++++++-------- 5 files changed, 62 insertions(+), 19 deletions(-) -- 2.13.0
If the first console is just a copy of the first serial device we don't need to iterate over the same device twice in order to perform actions like security labeling, cgroup configuring, etc. Currently only security SELinux manager was aware of this fact. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 26 +++++++++++++++++++++----- src/security/security_selinux.c | 10 ---------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0ff216e3a3..aa441fae3c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3517,6 +3517,24 @@ void virDomainDeviceInfoClear(virDomainDeviceInfoPtr info) } +static bool +virDomainSkipBackcompatConsole(virDomainDefPtr def, + size_t index, + bool all) +{ + virDomainChrDefPtr console = def->consoles[index]; + + if (!all && index == 0 && + (console->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || + console->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) && + def->os.type == VIR_DOMAIN_OSTYPE_HVM) { + return true; + } + + return false; +} + + static int virDomainDeviceInfoIterateInternal(virDomainDefPtr def, virDomainDeviceInfoCallback cb, @@ -3585,11 +3603,7 @@ virDomainDeviceInfoIterateInternal(virDomainDefPtr def, return -1; } for (i = 0; i < def->nconsoles; i++) { - if (!all && - i == 0 && - (def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL || - def->consoles[i]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE) && - def->os.type == VIR_DOMAIN_OSTYPE_HVM) + if (virDomainSkipBackcompatConsole(def, i, all)) continue; device.data.chr = def->consoles[i]; if (cb(def, &device, &def->consoles[i]->info, opaque) < 0) @@ -25313,6 +25327,8 @@ virDomainChrDefForeach(virDomainDefPtr def, goto done; } for (i = 0; i < def->nconsoles; i++) { + if (virDomainSkipBackcompatConsole(def, i, false)) + continue; if ((iter)(def, def->consoles[i], opaque) < 0) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index df7c96833e..612dbc2a83 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2339,11 +2339,6 @@ virSecuritySELinuxRestoreSecurityChardevCallback(virDomainDefPtr def, { virSecurityManagerPtr mgr = opaque; - /* This is taken care of by processing of def->serials */ - if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && - dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) - return 0; - return virSecuritySELinuxRestoreChardevLabel(mgr, def, dev, dev->source); } @@ -2733,11 +2728,6 @@ virSecuritySELinuxSetSecurityChardevCallback(virDomainDefPtr def, { virSecurityManagerPtr mgr = opaque; - /* This is taken care of by processing of def->serials */ - if (dev->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && - dev->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL) - return 0; - return virSecuritySELinuxSetChardevLabel(mgr, def, dev, dev->source); } -- 2.13.0
On Mon, May 15, 2017 at 04:28:34PM +0200, Pavel Hrdina wrote:
If the first console is just a copy of the first serial device we don't need to iterate over the same device twice in order to perform actions like security labeling, cgroup configuring, etc.
Currently only security SELinux manager was aware of this fact.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 26 +++++++++++++++++++++----- src/security/security_selinux.c | 10 ---------- 2 files changed, 21 insertions(+), 15 deletions(-)
ACK
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, } dest->type = src->type; + dest->skipRelabel = src->skipRelabel; return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0; while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel"); /* PTY path is only parsed from live xml. */ if (!path && (def->type != VIR_DOMAIN_CHR_TYPE_PTY || @@ -10726,6 +10730,17 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, _("Invalid append attribute value '%s'"), append); goto error; } + if (skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE && + (flags & VIR_DOMAIN_DEF_PARSE_STATUS)) { + if (STREQ(skipRelabel, "yes")) { + def->skipRelabel = true; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid 'skipRelabel' attribute value '%s'"), + skipRelabel); + goto error; + } + } if (!path && def->type != VIR_DOMAIN_CHR_TYPE_PTY) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -10902,6 +10917,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, VIR_FREE(logfile); VIR_FREE(haveTLS); VIR_FREE(tlsFromConfig); + VIR_FREE(skipRelabel); return remaining; @@ -22324,6 +22340,10 @@ virDomainChrSourceDefFormat(virBufferPtr buf, def->data.file.append != VIR_TRISTATE_SWITCH_ABSENT) virBufferAsprintf(buf, " append='%s'", virTristateSwitchTypeToString(def->data.file.append)); + if ((flags & VIR_DOMAIN_DEF_FORMAT_STATUS) && + def->type == VIR_DOMAIN_CHR_TYPE_FILE && def->skipRelabel) { + virBufferAddLit(buf, " skipRelabel='yes'"); + } virDomainSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags); } break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 09fb7aada4..329eb90392 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1166,6 +1166,7 @@ struct _virDomainChrSourceDef { } data; char *logfile; int logappend; + bool skipRelabel; }; /* A complete character device, both host and domain views. */ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 813a8515c0..0625075bb2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4998,6 +4998,7 @@ static int qemuBuildChrChardevFileStr(virLogManagerPtr logManager, virCommandPtr cmd, const virDomainDef *def, + virDomainChrSourceDefPtr sourceDef, virBufferPtr buf, const char *filearg, const char *fileval, const char *appendarg, int appendval) @@ -5011,6 +5012,9 @@ qemuBuildChrChardevFileStr(virLogManagerPtr logManager, appendval == VIR_TRISTATE_SWITCH_OFF) flags |= VIR_LOG_MANAGER_PROTOCOL_DOMAIN_OPEN_LOG_FILE_TRUNCATE; + if (sourceDef) + sourceDef->skipRelabel = true; + if ((logfd = virLogManagerDomainOpenLogFile(logManager, "qemu", def->uuid, @@ -5051,7 +5055,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virCommandPtr cmd, virQEMUDriverConfigPtr cfg, const virDomainDef *def, - const virDomainChrSourceDef *dev, + virDomainChrSourceDefPtr dev, const char *alias, virQEMUCapsPtr qemuCaps, bool nowait) @@ -5093,7 +5097,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, goto cleanup; } if (qemuBuildChrChardevFileStr(virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FILE_APPEND) ? - logManager : NULL, cmd, def, &buf, + logManager : NULL, cmd, def, dev, &buf, "path", dev->data.file.path, "append", dev->data.file.append) < 0) goto cleanup; @@ -5209,7 +5213,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("logfile not supported in this QEMU binary")); goto cleanup; } - if (qemuBuildChrChardevFileStr(logManager, cmd, def, &buf, + if (qemuBuildChrChardevFileStr(logManager, cmd, def, NULL, &buf, "logfile", dev->logfile, "logappend", dev->logappend) < 0) goto cleanup; @@ -5573,7 +5577,7 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager, virQEMUDriverConfigPtr cfg, virDomainDefPtr def, virQEMUCapsPtr qemuCaps, - const virDomainChrSourceDef *monitor_chr, + virDomainChrSourceDefPtr monitor_chr, bool monitor_json) { char *chrdev; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 922e484942..a4e02ca8bc 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1196,6 +1196,9 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + if (chr_seclabel && chr_seclabel->label) { if (virParseOwnershipIds(chr_seclabel->label, &user, &group) < 0) return -1; @@ -1276,6 +1279,9 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + switch ((virDomainChrType) dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 612dbc2a83..64ab2795d5 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -2216,6 +2216,9 @@ virSecuritySELinuxSetChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + if (chr_seclabel) imagelabel = chr_seclabel->label; if (!imagelabel) @@ -2289,6 +2292,9 @@ virSecuritySELinuxRestoreChardevLabel(virSecurityManagerPtr mgr, if (chr_seclabel && !chr_seclabel->relabel) return 0; + if (!chr_seclabel && dev_source->skipRelabel) + return 0; + switch (dev_source->type) { case VIR_DOMAIN_CHR_TYPE_DEV: case VIR_DOMAIN_CHR_TYPE_FILE: -- 2.13.0
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute. Either keep this data in private structure or even better, just add the same thing as you would do with: <seclabel relabel="no"/> with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already. I'm open to suggestions, but NACK to random "wannabe private" attributes.
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
What sense would it make in this case? There is only one option when we're running virtlogd and that is no label. If they supply relabel='yes', it won't work with virtlogd unless they change it back to 'no'. So, you've got an option the functionality of which depends on a service running, which is by default on by the way. Since I don't want to be someone who just criticizes other people's ideas without adding anything to the discussion, I was thinking about re-using the qemu config parameter stdioLogD. But that solution is also wrong as it turned out. I haven't come up with anything better yet, though. So, having something in the status XML seems like a plausible approach to me. Erik
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 23, 2017 at 05:13:17PM +0200, Erik Skultety wrote:
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
What sense would it make in this case? There is only one option when we're running virtlogd and that is no label. If they supply relabel='yes', it won't work with virtlogd unless they change it back to 'no'. So, you've got an option the functionality of which depends on a service running, which is by default on by the way.
If that's so clear, why put it in the XML? Why not just set it in the private structure then? Either there is a reason for it to be variable or not. If there is not, then we're effectively talking about something like <domain doNotFailOnStart='yes'> attribute. There is no need for such thing in that case. Sure, I might be wrong. In that case, this commit needs more explanation.
Since I don't want to be someone who just criticizes other people's ideas without adding anything to the discussion, I was thinking about re-using the qemu config parameter stdioLogD. But that solution is also wrong as it turned out. I haven't come up with anything better yet, though. So, having something in the status XML seems like a plausible approach to me.
Erik
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
The use of virtlogd affects many devices in guests. So we should just record at the top level of the QEMU domain status, whether virtlogd was used or not. 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 :|
On Tue, May 23, 2017 at 04:24:13PM +0100, Daniel P. Berrange wrote:
On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
The use of virtlogd affects many devices in guests. So we should just record at the top level of the QEMU domain status, whether virtlogd was used or not.
That would be one possibility, however we need to decide what to do in one specific case: 1. stdio_handler is set to logd and QEMU_CAPS_CHARDEV_FILE_APPEND is supported 2. start a guest with one serial chardev 3. change the stdio_handler to file and restart libvirtd 4. hotplug another serial chardev (this is currently broken [1]) There are to possible solution, we will store the usage of virtlogd domain-wide and virtlogd will be used for that domain even if the config option would change for the rest of that domain's life. Or we need to store the usage of virtlogd per device and it will always depend on the config option. [1] The hotplug of char devices doesn't use virtlogd at all and we don't relabel the files to be accessible by QEMU. Pavel
On Tue, May 23, 2017 at 05:53:46PM +0200, Pavel Hrdina wrote:
On Tue, May 23, 2017 at 04:24:13PM +0100, Daniel P. Berrange wrote:
On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
The use of virtlogd affects many devices in guests. So we should just record at the top level of the QEMU domain status, whether virtlogd was used or not.
That would be one possibility, however we need to decide what to do in one specific case:
1. stdio_handler is set to logd and QEMU_CAPS_CHARDEV_FILE_APPEND is supported
2. start a guest with one serial chardev
3. change the stdio_handler to file and restart libvirtd
4. hotplug another serial chardev (this is currently broken [1])
There are to possible solution, we will store the usage of virtlogd domain-wide and virtlogd will be used for that domain even if the config option would change for the rest of that domain's life. Or we need to store the usage of virtlogd per device and it will always depend on the config option.
Having some devices use virtlogd and some devices not use virtlogd is just a recipe for maximising confusion of developers, users, and support people alike. We should record whether we used virtlogd when we first start the guest, and honour that until QEMU is killed. 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 :|
On Tue, May 23, 2017 at 05:18:01PM +0100, Daniel P. Berrange wrote:
On Tue, May 23, 2017 at 05:53:46PM +0200, Pavel Hrdina wrote:
On Tue, May 23, 2017 at 04:24:13PM +0100, Daniel P. Berrange wrote:
On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
The use of virtlogd affects many devices in guests. So we should just record at the top level of the QEMU domain status, whether virtlogd was used or not.
That would be one possibility, however we need to decide what to do in one specific case:
1. stdio_handler is set to logd and QEMU_CAPS_CHARDEV_FILE_APPEND is supported
2. start a guest with one serial chardev
3. change the stdio_handler to file and restart libvirtd
4. hotplug another serial chardev (this is currently broken [1])
There are to possible solution, we will store the usage of virtlogd domain-wide and virtlogd will be used for that domain even if the config option would change for the rest of that domain's life. Or we need to store the usage of virtlogd per device and it will always depend on the config option.
Having some devices use virtlogd and some devices not use virtlogd is just a recipe for maximising confusion of developers, users, and support people alike.
We should record whether we used virtlogd when we first start the guest, and honour that until QEMU is killed.
This ^^ gets formatted in qemuDomainObjPrivateXMLFormat() which uses qemuDomainObjPrivatePtr; and that's one of the private structures you could keep it in. Just to answer Pavel's question. Also, just for info, is the related crasher in virtlogd fixed already or is someone working on that?
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 :|
On Wed, May 24, 2017 at 12:25:51PM +0200, Martin Kletzander wrote:
On Tue, May 23, 2017 at 05:18:01PM +0100, Daniel P. Berrange wrote:
On Tue, May 23, 2017 at 05:53:46PM +0200, Pavel Hrdina wrote:
On Tue, May 23, 2017 at 04:24:13PM +0100, Daniel P. Berrange wrote:
On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
Either keep this data in private structure or even better, just add the same thing as you would do with:
<seclabel relabel="no"/>
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
The use of virtlogd affects many devices in guests. So we should just record at the top level of the QEMU domain status, whether virtlogd was used or not.
That would be one possibility, however we need to decide what to do in one specific case:
1. stdio_handler is set to logd and QEMU_CAPS_CHARDEV_FILE_APPEND is supported
2. start a guest with one serial chardev
3. change the stdio_handler to file and restart libvirtd
4. hotplug another serial chardev (this is currently broken [1])
There are to possible solution, we will store the usage of virtlogd domain-wide and virtlogd will be used for that domain even if the config option would change for the rest of that domain's life. Or we need to store the usage of virtlogd per device and it will always depend on the config option.
Having some devices use virtlogd and some devices not use virtlogd is just a recipe for maximising confusion of developers, users, and support people alike.
We should record whether we used virtlogd when we first start the guest, and honour that until QEMU is killed.
This ^^ gets formatted in qemuDomainObjPrivateXMLFormat() which uses qemuDomainObjPrivatePtr; and that's one of the private structures you could keep it in. Just to answer Pavel's question.
Now it makes sense what you've meant by private structure, I was confused because we were talking about XML :).
Also, just for info, is the related crasher in virtlogd fixed already or is someone working on that?
The related crasher is not fix to my knowledge and probably no-one is working on it. Pavel
On Tue, May 23, 2017 at 04:25:01PM +0200, Martin Kletzander wrote:
On Mon, May 15, 2017 at 04:28:35PM +0200, Pavel Hrdina wrote:
If libvirt uses virtlogd instead of passing the file path directly to QEMU we shouldn't relabel the chardev source file, otherwise virtlogd will get a permission denied while reloading.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=143098
You missed a digit and I'm too lazy to check 10 bugs for a reproducer ;)
Yep, there should by additional 8 :)
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 12 ++++++++---- src/security/security_dac.c | 6 ++++++ src/security/security_selinux.c | 6 ++++++ 5 files changed, 41 insertions(+), 4 deletions(-)
I see you are changing the parser code, but you are not changing the Relax-NG schema, neither any test.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index aa441fae3c..92f011d3a4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2064,6 +2064,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest, }
dest->type = src->type; + dest->skipRelabel = src->skipRelabel;
return 0; } @@ -10608,6 +10609,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *append = NULL; char *haveTLS = NULL; char *tlsFromConfig = NULL; + char *skipRelabel = NULL; int remaining = 0;
while (cur != NULL) { @@ -10628,6 +10630,8 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, case VIR_DOMAIN_CHR_TYPE_UNIX: if (!append && def->type == VIR_DOMAIN_CHR_TYPE_FILE) append = virXMLPropString(cur, "append"); + if (!skipRelabel && def->type == VIR_DOMAIN_CHR_TYPE_FILE) + skipRelabel = virXMLPropString(cur, "skipRelabel");
I'm guessing you want to keep this away from users, right? You should not be parsing it from the XML then. Or you should add a thing there that the XML supports. Not just a random attribute.
We need to store it in the status XML so we don't lose the information while restarting libvirtd. And I want to keep it hidden from users because they shouldn't care about it. The decision whether we will use virtlogd as stdio handler is made by checking the qemu.conf for "stdio_handler" and whether QEMU supports appending to files.
Either keep this data in private structure or even better, just add the
What do you mean by private structure?
same thing as you would do with:
<seclabel relabel="no"/>
with all the details of course. The user can see it, can supply it, old releases support it, all the stuff is there already.
This for user to specify it, not to store our internal private information that should survive restarting libvirtd.
I'm open to suggestions, but NACK to random "wannabe private" attributes.
The only reason why I used the new private attribute is that we already do it for graphics device (fromConfig and autoGenerated) and for TCP character device (tlsFromConfig). I can store it in the status part of the status XML outside of the domain part, but that would require mapping it to the actual character device. Pavel
participants (4)
-
Daniel P. Berrange -
Erik Skultety -
Martin Kletzander -
Pavel Hrdina