Hello, I reproduced the symptoms (no output from designated pipe, guest hang after a while) with commit 05e2fc51, and with commit c7d1f598 they are gone. I also noticed libvirt needs to be built with --with-qemu-user=<user> --with-qemu-group=<group> to reproduce the symptoms, with older code, or the qemu process will be running as root and can write to un-chown'ed pipe. On 09/28/2011 03:01 PM, Daniel Veillard wrote:
On Tue, Sep 27, 2011 at 09:00:15PM -0400, Laine Stump wrote:
(v2: change to only relabel the uni-direction fifo pair if they're found, otherwise only relabel the bidirectional fifo, per comment in BZ by Dan Berrange)
This patch fixes the regression with using named pipes for qemu serial devices noted in:
https://bugzilla.redhat.com/show_bug.cgi?id=740478
The problem was that, while new code in libvirt looks for a single bidirectional fifo of the name given in the config, then relabels that and continues without looking for / relabelling the two unidirectional fifos named ${name}.in and ${name}.out, qemu looks in the opposite order. So if the user had naively created all three fifos, libvirt would relabel the bidirectional fifo to allow qemu access, but qemu would attempt to use the two unidirectional fifos and fail (because it didn't have proper permissions/rights).
This patch changes the order that libvirt looks for the fifos to match what qemu does - first it looks for the dual fifos, then it looks for the single bidirectional fifo. If it finds the dual unidirectional fifos first, it labels/chowns them and ignores any possible bidirectional fifo.
(Note commit d37c6a3a (which first appeared in libvirt-0.9.2) added the code that checked for a bidirectional fifo. Prior to that commit, bidirectional fifos fdor serial devices didn't work unless the fifo was pre-owned/labelled such that qemu could access it.) --- src/security/security_dac.c | 30 ++++++++++++++++++------------ src/security/security_selinux.c | 29 +++++++++++++++++------------ 2 files changed, 35 insertions(+), 24 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c index af02236..ac79e70 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -406,18 +406,19 @@ virSecurityDACSetChardevLabel(virSecurityManagerPtr mgr, break;
case VIR_DOMAIN_CHR_TYPE_PIPE: - if (virFileExists(dev->data.file.path)) { - if (virSecurityDACSetOwnership(dev->data.file.path, priv->user, priv->group)< 0) - goto done; - } else { - if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || - (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { - virReportOOMError(); - goto done; - } + if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || + (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { + virReportOOMError(); + goto done; + } + if (virFileExists(in) || virFileExists(out)) {
Actually the qemu code of qemu_chr_open_pipe() does if (fd_in< 0 || fd_out< 0) { close either if they opened and try to open the main file so the logic in libvirt should be
if (virFileExists(in)&& virFileExists(out)) {
if ((virSecurityDACSetOwnership(in, priv->user, priv->group)< 0) || - (virSecurityDACSetOwnership(out, priv->user, priv->group)< 0)) + (virSecurityDACSetOwnership(out, priv->user, priv->group)< 0)) { goto done; + } + } else if (virSecurityDACSetOwnership(dev->data.file.path, + priv->user, priv->group)< 0) { + goto done; } ret = 0; break; @@ -452,9 +453,14 @@ virSecurityDACRestoreChardevLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virReportOOMError(); goto done; } - if ((virSecurityDACRestoreSecurityFileLabel(out)< 0) || - (virSecurityDACRestoreSecurityFileLabel(in)< 0)) + if (virFileExists(in) || virFileExists(out)) {
Same
+ if ((virSecurityDACRestoreSecurityFileLabel(out)< 0) || + (virSecurityDACRestoreSecurityFileLabel(in)< 0)) { goto done; + } + } else if (virSecurityDACRestoreSecurityFileLabel(dev->data.file.path)< 0) { + goto done; + } ret = 0; break;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 0807a34..9e5b668 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -806,18 +806,18 @@ SELinuxSetSecurityChardevLabel(virDomainObjPtr vm, break;
case VIR_DOMAIN_CHR_TYPE_PIPE: - if (virFileExists(dev->data.file.path)) { - if (SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel)< 0) - goto done; - } else { - if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || - (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { - virReportOOMError(); - goto done; - } + if ((virAsprintf(&in, "%s.in", dev->data.file.path)< 0) || + (virAsprintf(&out, "%s.out", dev->data.file.path)< 0)) { + virReportOOMError(); + goto done; + } + if (virFileExists(in) || virFileExists(out)) {
Same
if ((SELinuxSetFilecon(in, secdef->imagelabel)< 0) || - (SELinuxSetFilecon(out, secdef->imagelabel)< 0)) + (SELinuxSetFilecon(out, secdef->imagelabel)< 0)) { goto done; + } + } else if (SELinuxSetFilecon(dev->data.file.path, secdef->imagelabel)< 0) { + goto done; } ret = 0; break; @@ -858,9 +858,14 @@ SELinuxRestoreSecurityChardevLabel(virDomainObjPtr vm, virReportOOMError(); goto done; } - if ((SELinuxRestoreSecurityFileLabel(out)< 0) || - (SELinuxRestoreSecurityFileLabel(in)< 0)) + if (virFileExists(in) || virFileExists(out)) {
And same,
+ if ((SELinuxRestoreSecurityFileLabel(out)< 0) || + (SELinuxRestoreSecurityFileLabel(in)< 0)) { + goto done; + } + } else if (SELinuxRestoreSecurityFileLabel(dev->data.file.path)< 0) { goto done; + } ret = 0; break;
That bug got me a bit crazy, I would like to get it fixed :)
ACK conditionned on the above four || replaced by&&
Daniel
-- Thanks. Hong Xiang