On Thu, Jun 15, 2017 at 04:40:49PM +0200, Pavel Hrdina wrote: > On Thu, Jun 15, 2017 at 07:57:18AM -0400, John Ferlan wrote: > > > > > > On 06/15/2017 03:11 AM, Pavel Hrdina wrote: > > > On Tue, Jun 13, 2017 at 08:00:41PM -0400, John Ferlan wrote: > > >> > > >> > > >> On 05/29/2017 10:31 AM, Pavel Hrdina wrote: > > >>> In the case that virtlogd is used as stdio handler we pass to QEMU > > >>> only FD to a PIPE connected to virtlogd instead of the file itself. > > >>> > > >>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1430988 > > >>> > > >>> Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx> > > >>> --- > > >>> > > >>> Notes: > > >>> new in v2 > > >>> > > >>> src/lxc/lxc_process.c | 6 ++--- > > >>> src/qemu/qemu_security.c | 9 +++++-- > > >>> src/security/security_apparmor.c | 7 ++++-- > > >>> src/security/security_dac.c | 54 +++++++++++++++++++++++++++++++--------- > > >>> src/security/security_driver.h | 6 +++-- > > >>> src/security/security_manager.c | 12 ++++++--- > > >>> src/security/security_manager.h | 6 +++-- > > >>> src/security/security_nop.c | 6 +++-- > > >>> src/security/security_selinux.c | 53 ++++++++++++++++++++++++++++++--------- > > >>> src/security/security_stack.c | 12 ++++++--- > > >>> tests/securityselinuxlabeltest.c | 2 +- > > >>> 11 files changed, 127 insertions(+), 46 deletions(-) > > >>> > > >> > > >> Why is it a (!chr_seclabel && chardevStdioLogd)? More to the point why > > >> is (!chr_seclabel) even matter? > > > > > > If you configure the label we shouldn't ignore it in some cases, that's > > > just wrong. If the label is set for the char device we will configure > > > it every time even if it will fail to start the guest, it's a > > > responsibility of the user to configure proper label if it is provided. > > > > > > > When email doesn't convey the question... Ugh... I'm also trying to > > speed learn an area of the code and review at the same time. > > > > If I go back to commit id 'f8b08d0e' where the original implementation > > to add labels for chardevs was done (but has been modified for patch 1 > > to change where the label is stored), I get the impression that a label > > should be added either from something specifically supplied for the > > <chardev> or to use the per domain one: > > > > "The source element may contain an optional seclabel to override the way > > that labelling is done on the socket path. If this element is not > > present, the security label is inherited from the per-domain setting." > > > > So if I look at the condition "(!chr_seclabel && chardevStdioLogd)" > > added by this patch to decide whether or not to supply the label, I'm > > left with the impression that if for this particular chardev a label > > doesn't exist *and* the configuration option chardevStdioLogd is true, > > then we're going to return happy status *and* not inherit the per-domain > > setting. > > > > So the bug is then that applying the default domain label for a chardev > > configured to use a stdio handle is incorrect? Perhaps I didn't get > > that from reading bz or the patch. > > Yes, that's the bug. If virtlogd is used to handle stdio for char > devices we shouldn't relabel the @path to the default labels, the > @path doesn't have to be accessible by the qemu process, it has to be > accessible by the virtlogd process. > > > >> IIUC, whether or not someone set the label for the chardev, for this > > >> particular issue/config where the chardev has a <log file=$path>, we > > >> don't want to set (or restore) the label. I feel like I'm missing > > >> something subtle. Maybe a bit more explanation of the adjustment would > > >> help me... > > > > > > This is not for the <log file=$path/> but for the <source path=$path/>. > > > We don't relabel $path for <log file=$path/> at all. > > > > > > > hmm.. ah, right... I kept scrolling back and forth in the bz and the > > docs, but missed this in the bz: > > > > 3) Check the virtlogd.log: > > error : virRotatingFileWriterEntryNew:113 : Unable to open file: > > /var/log/libvirt/qemu/log: Permission denied > > > > I guess I got lost in the power of suggestion of reading the docs > > regarding the "optional log file" that can be associated paragraph and > > trying to learn on the fly so that you at least get a review in a > > somewhat timely manner ;-) > > > > >> Wouldn't these changes end up selecting "any" chardev if > > >> chardevStdioLogd ended up being set regardless of whether they were > > >> actually using the log file? > > > > > > I don't know what you mean by this sentence? > > > > > > > Well let's see, chardevStdioLogd is set to true when meeting the two > > conditions a qemu.conf global "cfg->stdioLogd" && a per domain or > > emulator image capability "QEMU_CAPS_CHARDEV_FILE_APPEND". > > > > So, conceivably chardevStdioLogd could be true for *any* domain as long > > as those conditions are met, right? > > Yes, the two conditions are checked while starting a new domain in > qemuProcessPrepareDomain() and stored in the private date of that > domain. > > > If you have a domain that has chardev's which are not configured to use > > the stdio handler, then the chardevStdioLogd could still be true, right? > > No, if the @chardevStdioLogd is true all char devices for that domain > will use virtlogd. > > The only issue I've just found out is that the code path for chardev > hot-plug isn't updated to use virtlogd when it should be so for > hot-plugged char devices we pass the path directly to QEMU. > > With this patch applied the hot-plug fails if virtlogd is used because > we don't relabel the path but we pass it directly to QEMU, this needs to > be fixed to not introduce a regression, sigh. Ok, I've tried it without this patch and it still fails with permission denied so there is no regression, we just don't relabel the hot-plugged chardev at all. Still it should be fixed. > Pavel > > > If that's the case and the chardev doesn't have a label associated, then > > we just return happy status and we do not inherit the per domain > > setting. Wouldn't that be incorrect? > > > > My concern is more we're making a change in a (mostly) common set of > > functions for a (very) specific problem. > > > > > > >> As an aside, I think there's an "oddity" when it comes to the Restore, > > >> but I'm not sure how to put it into words exactly. If a guest is running > > >> code prior to this set of changes, would it have successfully run a Set? > > >> If so, then after applying this change and restarting, the label > > >> wouldn't be reset, right? What happens at guest shutdown - does the > > >> label not get unset now? Of course this is all "interaction" with > > >> virtlogd restart that really throws a monkey wrench into things. > > > > > > No, that's not correct. The @chardevStdioLogd is stored in the status > > > XML (the one stored in /var/run/libvirt/qemu/$domain_name.xml). So when > > > the libvirtd is stopped and started with this patch applied the status > > > XML doesn't have the @chardevStdioLogd stored in it so it will be false > > > and we will reset the label. The @chardevStdioLogd is updated only when > > > the domain is started and we will store the value in the status XML > > > only with new libvirtd and only in that case we will not set/restore > > > the label. > > > > > > > hmmm.. Reading the bz indicates the 'virtlogd' daemon restarting... This > > is where all this gets a bit "odd" for me. Like I said it was a weird > > thing to even try and explain, but I think you talked me off the ledge > > of concern. > > > > John > > > > >> Also, why is the Smartcard chardev handling not using this > > > > > > The smartcard doesn't ever use virtlogd as stdio handler. > > > > > > Pavel > > > > > > > -- > > libvir-list mailing list > > libvir-list@xxxxxxxxxx > > https://www.redhat.com/mailman/listinfo/libvir-list > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
