Re: [libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs
On Tue, 2017-10-17 at 09:04 +0200, Christian Ehrhardt wrote:
> On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik m>
> wrote:
>
> > On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
> > > If users only specified vendor&product (the common case) then
> > > parsing
> > > the xml via virDomainHostdevSubsysUSBDefParseXML would only set
> > > these.
> > > Bus and Device would much later be added when the devices are
> > > prepared
> > > to be added.
> > >
> > > Due to that a hot-add of a usb hostdev works as the device is
> > > prepared
> > > and virt-aa-helper processes the new internal xml. But on an
> > > initial
> > > guest start at the time virt-aa-helper renders the apparmor rules
> > > the
> > > bus/device id's are not set yet:
> > >
> > > p ctl->def->hostdevs[0]->source.subsys.u.usb
> > > $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921,
> > > product
> > > = 21888}
> > >
> > > That causes rules to be wrong:
> > > "/dev/bus/usb/000/000" rw,
> > >
> > > The fix calls virHostdevFindUSBDevice after reading the XML from
> > > irt-aa-helper to only add apparmor rules for devices that could
> > > be found
> > > and now are fully known to be able to write the rule correctly.
> > >
> > > It uncondtionally sets virHostdevFindUSBDevice mandatory
> > > attribute as
> > > adding an apparmor rule for a device not found makes no sense no
> > > matter
> > > what startup policy it has set.
> > >
> > > Signed-off-by: Christian Ehrhardt > > om>
> > > ---
> > > src/security/virt-aa-helper.c | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/src/security/virt-aa-helper.c
> >
> > b/src/security/virt-aa-helper.c
> > > index 7944dc1..d1518ea 100644
> > > --- a/src/security/virt-aa-helper.c
> > > +++ b/src/security/virt-aa-helper.c
> > > @@ -55,6 +55,7 @@
> > > #include "virrandom.h"
> > > #include "virstring.h"
> > > #include "virgettext.h"
> > > +#include "virhostdev.h"
> > >
> > > #include "storage/storage_source.h"
> > >
> > > @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
> > > if (usb == NULL)
> > > continue;
> > >
> > > +if (virHostdevFindUSBDevice(dev, true, &usb) <
> > > 0)
> > > +continue;
> > > +
> >
> > Shouldn't we rather fail in this case? Or, what happens if
> > startupPolicy
> > of the device is set to 'optional'? I think we need to error out
> > here
> > (although, we've probably errored out earlier in the process).
> >
>
> Hi,
> sorry for the late reply, but I was finally getting some time off for
> a few
> days.
> I intentionally decided not to error out to avoid a new "source" of
> issues.
> Compare the two options we have:
> 1. continue if not finding the device
> 1.1 likely case we found it, rule will be correct - good
> 1.2 we don't find it (for whatever reason) - we are "as bad" as
> before
> this fix, but not worse
> 2. error out if not finding the device
> 2.1 likely case we found it, rule will be correct - good
> 2.2 we don't find it (for whatever reason) - we are now failing
> completely
>
> What I don't like about 2.2 is that there might be cases things would
> have
> been kind of ok, depsite whatever dark usb magic hit some special
> setup.
> In those cases if we error out we add a new chance to fail.
> And as there are often too many unknowns, so I chose the safer
> option.
>
I agree with your assessment and simply continuing if not found rather
than erroring. Patch LGTM. +1
Thanks for chasing this one down.
--
Jamie Strandboge | http://www.canonical.com
signature.asc
Description: This is a digitally signed message part
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs
On Fri, Sep 29, 2017 at 4:58 PM, Michal Privoznik
wrote:
> On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
> > If users only specified vendor&product (the common case) then parsing
> > the xml via virDomainHostdevSubsysUSBDefParseXML would only set these.
> > Bus and Device would much later be added when the devices are prepared
> > to be added.
> >
> > Due to that a hot-add of a usb hostdev works as the device is prepared
> > and virt-aa-helper processes the new internal xml. But on an initial
> > guest start at the time virt-aa-helper renders the apparmor rules the
> > bus/device id's are not set yet:
> >
> > p ctl->def->hostdevs[0]->source.subsys.u.usb
> > $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product
> > = 21888}
> >
> > That causes rules to be wrong:
> > "/dev/bus/usb/000/000" rw,
> >
> > The fix calls virHostdevFindUSBDevice after reading the XML from
> > irt-aa-helper to only add apparmor rules for devices that could be found
> > and now are fully known to be able to write the rule correctly.
> >
> > It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as
> > adding an apparmor rule for a device not found makes no sense no matter
> > what startup policy it has set.
> >
> > Signed-off-by: Christian Ehrhardt
> > ---
> > src/security/virt-aa-helper.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/src/security/virt-aa-helper.c
> b/src/security/virt-aa-helper.c
> > index 7944dc1..d1518ea 100644
> > --- a/src/security/virt-aa-helper.c
> > +++ b/src/security/virt-aa-helper.c
> > @@ -55,6 +55,7 @@
> > #include "virrandom.h"
> > #include "virstring.h"
> > #include "virgettext.h"
> > +#include "virhostdev.h"
> >
> > #include "storage/storage_source.h"
> >
> > @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
> > if (usb == NULL)
> > continue;
> >
> > +if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
> > +continue;
> > +
>
> Shouldn't we rather fail in this case? Or, what happens if startupPolicy
> of the device is set to 'optional'? I think we need to error out here
> (although, we've probably errored out earlier in the process).
>
Hi,
sorry for the late reply, but I was finally getting some time off for a few
days.
I intentionally decided not to error out to avoid a new "source" of issues.
Compare the two options we have:
1. continue if not finding the device
1.1 likely case we found it, rule will be correct - good
1.2 we don't find it (for whatever reason) - we are "as bad" as before
this fix, but not worse
2. error out if not finding the device
2.1 likely case we found it, rule will be correct - good
2.2 we don't find it (for whatever reason) - we are now failing completely
What I don't like about 2.2 is that there might be cases things would have
been kind of ok, depsite whatever dark usb magic hit some special setup.
In those cases if we error out we add a new chance to fail.
And as there are often too many unknowns, so I chose the safer option.
> ACK to the rest of the patches (after some typo clean up, esp. in the
> commit messages).
>
Thanks,
do you want me to clean up commit messages or will somebody do (to his
preferred style) on accepting the commit?
> Michal
>
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs
On 09/20/2017 04:59 PM, Christian Ehrhardt wrote:
> If users only specified vendor&product (the common case) then parsing
> the xml via virDomainHostdevSubsysUSBDefParseXML would only set these.
> Bus and Device would much later be added when the devices are prepared
> to be added.
>
> Due to that a hot-add of a usb hostdev works as the device is prepared
> and virt-aa-helper processes the new internal xml. But on an initial
> guest start at the time virt-aa-helper renders the apparmor rules the
> bus/device id's are not set yet:
>
> p ctl->def->hostdevs[0]->source.subsys.u.usb
> $12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product
> = 21888}
>
> That causes rules to be wrong:
> "/dev/bus/usb/000/000" rw,
>
> The fix calls virHostdevFindUSBDevice after reading the XML from
> irt-aa-helper to only add apparmor rules for devices that could be found
> and now are fully known to be able to write the rule correctly.
>
> It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as
> adding an apparmor rule for a device not found makes no sense no matter
> what startup policy it has set.
>
> Signed-off-by: Christian Ehrhardt
> ---
> src/security/virt-aa-helper.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 7944dc1..d1518ea 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -55,6 +55,7 @@
> #include "virrandom.h"
> #include "virstring.h"
> #include "virgettext.h"
> +#include "virhostdev.h"
>
> #include "storage/storage_source.h"
>
> @@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
> if (usb == NULL)
> continue;
>
> +if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
> +continue;
> +
Shouldn't we rather fail in this case? Or, what happens if startupPolicy
of the device is set to 'optional'? I think we need to error out here
(although, we've probably errored out earlier in the process).
ACK to the rest of the patches (after some typo clean up, esp. in the
commit messages).
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] virt-aa-helper: fix paths for usb hostdevs
If users only specified vendor&product (the common case) then parsing
the xml via virDomainHostdevSubsysUSBDefParseXML would only set these.
Bus and Device would much later be added when the devices are prepared
to be added.
Due to that a hot-add of a usb hostdev works as the device is prepared
and virt-aa-helper processes the new internal xml. But on an initial
guest start at the time virt-aa-helper renders the apparmor rules the
bus/device id's are not set yet:
p ctl->def->hostdevs[0]->source.subsys.u.usb
$12 = {autoAddress = false, bus = 0, device = 0, vendor = 1921, product
= 21888}
That causes rules to be wrong:
"/dev/bus/usb/000/000" rw,
The fix calls virHostdevFindUSBDevice after reading the XML from
irt-aa-helper to only add apparmor rules for devices that could be found
and now are fully known to be able to write the rule correctly.
It uncondtionally sets virHostdevFindUSBDevice mandatory attribute as
adding an apparmor rule for a device not found makes no sense no matter
what startup policy it has set.
Signed-off-by: Christian Ehrhardt
---
src/security/virt-aa-helper.c | 4
1 file changed, 4 insertions(+)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 7944dc1..d1518ea 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -55,6 +55,7 @@
#include "virrandom.h"
#include "virstring.h"
#include "virgettext.h"
+#include "virhostdev.h"
#include "storage/storage_source.h"
@@ -1069,6 +1070,9 @@ get_files(vahControl * ctl)
if (usb == NULL)
continue;
+if (virHostdevFindUSBDevice(dev, true, &usb) < 0)
+continue;
+
rc = virUSBDeviceFileIterate(usb, file_iterate_hostdev_cb,
&buf);
virUSBDeviceFree(usb);
if (rc != 0)
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
