4:17 a.m.
There are certain cases e.g. containers where the sysfs path might
exists, but might fail. Unfortunately the exact restrictions are only
known to libvirt when trying to write to it so we need to try it.
But in case it fails there is no need to fully abort, in those cases try
to fall back to the older ioctl interface which can still work.
That makes setting up a bridge in unprivileged LXD containers work.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index 071ebb7b35..cbba3c9652 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
* or by ioctl on older kernels. Perhaps we could just use
* ioctl for every kernel, but its not clear what the long
* term lifespan of the ioctl interface is...
+ * Fall back to ioctl if sysfs interface is not available or
+ * failing (e.g. due to container isolation).
*/
static int virNetDevBridgeSet(const char *brname,
const char *paramname, /* sysfs param name */
@@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
if (virFileExists(path)) {
char valuestr[INT_BUFSIZE_BOUND(value)];
snprintf(valuestr, sizeof(valuestr), "%lu", value);
- if (virFileWriteStr(path, valuestr, 0) < 0) {
- virReportSystemError(errno,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
+ if (virFileWriteStr(path, valuestr, 0) >= 0)
+ return 0;
+ virReportSystemError(errno,
+ _("Unable to set bridge %s %s via sysfs"),
+ brname, paramname);
+ }
+
+ unsigned long paramid;
+ if (STREQ(paramname, "stp_state")) {
+ paramid = BRCTL_SET_BRIDGE_STP_STATE;
+ } else if (STREQ(paramname, "forward_delay")) {
+ paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
} else {
- unsigned long paramid;
- if (STREQ(paramname, "stp_state")) {
- paramid = BRCTL_SET_BRIDGE_STP_STATE;
- } else if (STREQ(paramname, "forward_delay")) {
- paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
- } else {
- virReportSystemError(EINVAL,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
- unsigned long args[] = { paramid, value, 0, 0 };
- ifr->ifr_data = (char*)&args;
- if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
- virReportSystemError(errno,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
+ virReportSystemError(EINVAL,
+ _("Unable to set bridge %s %s via ioctl"),
+ brname, paramname);
+ return -1;
+ }
+ unsigned long args[] = { paramid, value, 0, 0 };
+ ifr->ifr_data = (char*)&args;
+ if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
+ virReportSystemError(errno,
+ _("Failed to set bridge %s %s via ioctl"),
+ brname, paramname);
+ return -1;
}
return 0;
--
2.17.1
5:54 a.m.
On Tue, Nov 20, 2018 at 11:17 AM Christian Ehrhardt
<christian.ehrhardt(a)canonical.com> wrote:
There are certain cases e.g. containers where the sysfs path might
exists, but might fail. Unfortunately the exact restrictions are only
known to libvirt when trying to write to it so we need to try it.
But in case it fails there is no need to fully abort, in those cases try
to fall back to the older ioctl interface which can still work.
That makes setting up a bridge in unprivileged LXD containers work.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
FYI: in the meantime I got the email of the bug-reporter.
Eventually I'll add:
Reported-by: Brian Candler <b.candler(a)pobox.com>
But that alone was not worth a v2 submission.
5:58 a.m.
On Tue, Nov 20, 2018 at 11:17:07AM +0100, Christian Ehrhardt wrote:
There are certain cases e.g. containers where the sysfs path might
exists, but might fail. Unfortunately the exact restrictions are only
known to libvirt when trying to write to it so we need to try it.
But in case it fails there is no need to fully abort, in those cases try
to fall back to the older ioctl interface which can still work.
That makes setting up a bridge in unprivileged LXD containers work.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
---
src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index 071ebb7b35..cbba3c9652 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
* or by ioctl on older kernels. Perhaps we could just use
* ioctl for every kernel, but its not clear what the long
* term lifespan of the ioctl interface is...
+ * Fall back to ioctl if sysfs interface is not available or
+ * failing (e.g. due to container isolation).
*/
static int virNetDevBridgeSet(const char *brname,
const char *paramname, /* sysfs param name */
@@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
if (virFileExists(path)) {
char valuestr[INT_BUFSIZE_BOUND(value)];
snprintf(valuestr, sizeof(valuestr), "%lu", value);
- if (virFileWriteStr(path, valuestr, 0) < 0) {
- virReportSystemError(errno,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
+ if (virFileWriteStr(path, valuestr, 0) >= 0)
+ return 0;
+ virReportSystemError(errno,
+ _("Unable to set bridge %s %s via sysfs"),
+ brname, paramname);
The virReportSystemError line should be dropped, since we're not
returning an error - we're moving onto the second code path instead.
+ }
+
+ unsigned long paramid;
+ if (STREQ(paramname, "stp_state")) {
+ paramid = BRCTL_SET_BRIDGE_STP_STATE;
+ } else if (STREQ(paramname, "forward_delay")) {
+ paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
} else {
- unsigned long paramid;
- if (STREQ(paramname, "stp_state")) {
- paramid = BRCTL_SET_BRIDGE_STP_STATE;
- } else if (STREQ(paramname, "forward_delay")) {
- paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
- } else {
- virReportSystemError(EINVAL,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
- unsigned long args[] = { paramid, value, 0, 0 };
- ifr->ifr_data = (char*)&args;
- if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
- virReportSystemError(errno,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
+ virReportSystemError(EINVAL,
+ _("Unable to set bridge %s %s via ioctl"),
+ brname, paramname);
+ return -1;
+ }
+ unsigned long args[] = { paramid, value, 0, 0 };
+ ifr->ifr_data = (char*)&args;
+ if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
+ virReportSystemError(errno,
+ _("Failed to set bridge %s %s via ioctl"),
+ brname, paramname);
+ return -1;
}
return 0;
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 :|
6:03 a.m.
On Tue, Nov 20, 2018 at 12:58 PM Daniel P. Berrangé <berrange(a)redhat.com> wrote:
On Tue, Nov 20, 2018 at 11:17:07AM +0100, Christian Ehrhardt wrote:
> There are certain cases e.g. containers where the sysfs path might
> exists, but might fail. Unfortunately the exact restrictions are only
> known to libvirt when trying to write to it so we need to try it.
>
> But in case it fails there is no need to fully abort, in those cases try
> to fall back to the older ioctl interface which can still work.
>
> That makes setting up a bridge in unprivileged LXD containers work.
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> ---
> src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> index 071ebb7b35..cbba3c9652 100644
> --- a/src/util/virnetdevbridge.c
> +++ b/src/util/virnetdevbridge.c
> @@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
> * or by ioctl on older kernels. Perhaps we could just use
> * ioctl for every kernel, but its not clear what the long
> * term lifespan of the ioctl interface is...
> + * Fall back to ioctl if sysfs interface is not available or
> + * failing (e.g. due to container isolation).
> */
> static int virNetDevBridgeSet(const char *brname,
> const char *paramname, /* sysfs param name */
> @@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
> if (virFileExists(path)) {
> char valuestr[INT_BUFSIZE_BOUND(value)];
> snprintf(valuestr, sizeof(valuestr), "%lu", value);
> - if (virFileWriteStr(path, valuestr, 0) < 0) {
> - virReportSystemError(errno,
> - _("Unable to set bridge %s %s"), brname,
paramname);
> - return -1;
> - }
> + if (virFileWriteStr(path, valuestr, 0) >= 0)
> + return 0;
> + virReportSystemError(errno,
> + _("Unable to set bridge %s %s via sysfs"),
> + brname, paramname);
The virReportSystemError line should be dropped, since we're not
returning an error - we're moving onto the second code path instead.
Yeah that sounds fine - would we want a VIR_WARN or VIR_INFO instead
at this place.
Any preferences?
> + }
> +
> + unsigned long paramid;
> + if (STREQ(paramname, "stp_state")) {
> + paramid = BRCTL_SET_BRIDGE_STP_STATE;
> + } else if (STREQ(paramname, "forward_delay")) {
> + paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
> } else {
> - unsigned long paramid;
> - if (STREQ(paramname, "stp_state")) {
> - paramid = BRCTL_SET_BRIDGE_STP_STATE;
> - } else if (STREQ(paramname, "forward_delay")) {
> - paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
> - } else {
> - virReportSystemError(EINVAL,
> - _("Unable to set bridge %s %s"), brname,
paramname);
> - return -1;
> - }
> - unsigned long args[] = { paramid, value, 0, 0 };
> - ifr->ifr_data = (char*)&args;
> - if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> - virReportSystemError(errno,
> - _("Unable to set bridge %s %s"), brname,
paramname);
> - return -1;
> - }
> + virReportSystemError(EINVAL,
> + _("Unable to set bridge %s %s via ioctl"),
> + brname, paramname);
> + return -1;
> + }
> + unsigned long args[] = { paramid, value, 0, 0 };
> + ifr->ifr_data = (char*)&args;
> + if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
> + virReportSystemError(errno,
> + _("Failed to set bridge %s %s via ioctl"),
> + brname, paramname);
> + return -1;
> }
>
> return 0;
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 :|
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
6:11 a.m.
On Tue, Nov 20, 2018 at 01:03:09PM +0100, Christian Ehrhardt wrote:
On Tue, Nov 20, 2018 at 12:58 PM Daniel P. Berrangé
<berrange(a)redhat.com> wrote:
>
> On Tue, Nov 20, 2018 at 11:17:07AM +0100, Christian Ehrhardt wrote:
> > There are certain cases e.g. containers where the sysfs path might
> > exists, but might fail. Unfortunately the exact restrictions are only
> > known to libvirt when trying to write to it so we need to try it.
> >
> > But in case it fails there is no need to fully abort, in those cases try
> > to fall back to the older ioctl interface which can still work.
> >
> > That makes setting up a bridge in unprivileged LXD containers work.
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> > ---
> > src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
> > 1 file changed, 26 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
> > index 071ebb7b35..cbba3c9652 100644
> > --- a/src/util/virnetdevbridge.c
> > +++ b/src/util/virnetdevbridge.c
> > @@ -113,6 +113,8 @@ static int virNetDevBridgeCmd(const char *brname,
> > * or by ioctl on older kernels. Perhaps we could just use
> > * ioctl for every kernel, but its not clear what the long
> > * term lifespan of the ioctl interface is...
> > + * Fall back to ioctl if sysfs interface is not available or
> > + * failing (e.g. due to container isolation).
> > */
> > static int virNetDevBridgeSet(const char *brname,
> > const char *paramname, /* sysfs param name */
> > @@ -128,29 +130,31 @@ static int virNetDevBridgeSet(const char *brname,
> > if (virFileExists(path)) {
> > char valuestr[INT_BUFSIZE_BOUND(value)];
> > snprintf(valuestr, sizeof(valuestr), "%lu", value);
> > - if (virFileWriteStr(path, valuestr, 0) < 0) {
> > - virReportSystemError(errno,
> > - _("Unable to set bridge %s %s"),
brname, paramname);
> > - return -1;
> > - }
> > + if (virFileWriteStr(path, valuestr, 0) >= 0)
> > + return 0;
> > + virReportSystemError(errno,
> > + _("Unable to set bridge %s %s via
sysfs"),
> > + brname, paramname);
>
> The virReportSystemError line should be dropped, since we're not
> returning an error - we're moving onto the second code path instead.
Yeah that sounds fine - would we want a VIR_WARN or VIR_INFO instead
at this place.
Any preferences?
I dont think it needs anything more than a VIR_DEBUG, since it is a normal
expected thing in the scenario you described.
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 :|
6:25 a.m.
New subject: [libvirt] [PATCH v2] util: netdevbridge: fall back to ioctl from sysfs
There are certain cases e.g. containers where the sysfs path might
exists, but might fail. Unfortunately the exact restrictions are only
known to libvirt when trying to write to it so we need to try it.
But in case it fails there is no need to fully abort, in those cases try
to fall back to the older ioctl interface which can still work.
That makes setting up a bridge in unprivileged LXD containers work.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
Reported-by: Brian Candler <b.candler(a)pobox.com>
---
src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
index 071ebb7b35..fc6389d0c7 100644
--- a/src/util/virnetdevbridge.c
+++ b/src/util/virnetdevbridge.c
@@ -28,6 +28,7 @@
#include "virutil.h"
#include "virfile.h"
#include "viralloc.h"
+#include "virlog.h"
#include "intprops.h"
#include "virstring.h"
@@ -74,6 +75,7 @@
#define VIR_FROM_THIS VIR_FROM_NONE
+VIR_LOG_INIT("util.netdevbridge");
#if defined(HAVE_BSD_BRIDGE_MGMT)
static int virNetDevBridgeCmd(const char *brname,
@@ -113,6 +115,8 @@ static int virNetDevBridgeCmd(const char *brname,
* or by ioctl on older kernels. Perhaps we could just use
* ioctl for every kernel, but its not clear what the long
* term lifespan of the ioctl interface is...
+ * Fall back to ioctl if sysfs interface is not available or
+ * failing (e.g. due to container isolation).
*/
static int virNetDevBridgeSet(const char *brname,
const char *paramname, /* sysfs param name */
@@ -128,29 +132,29 @@ static int virNetDevBridgeSet(const char *brname,
if (virFileExists(path)) {
char valuestr[INT_BUFSIZE_BOUND(value)];
snprintf(valuestr, sizeof(valuestr), "%lu", value);
- if (virFileWriteStr(path, valuestr, 0) < 0) {
- virReportSystemError(errno,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
+ if (virFileWriteStr(path, valuestr, 0) >= 0)
+ return 0;
+ VIR_DEBUG("Unable to set bridge %s %s via sysfs", brname, paramname);
+ }
+
+ unsigned long paramid;
+ if (STREQ(paramname, "stp_state")) {
+ paramid = BRCTL_SET_BRIDGE_STP_STATE;
+ } else if (STREQ(paramname, "forward_delay")) {
+ paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
} else {
- unsigned long paramid;
- if (STREQ(paramname, "stp_state")) {
- paramid = BRCTL_SET_BRIDGE_STP_STATE;
- } else if (STREQ(paramname, "forward_delay")) {
- paramid = BRCTL_SET_BRIDGE_FORWARD_DELAY;
- } else {
- virReportSystemError(EINVAL,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
- unsigned long args[] = { paramid, value, 0, 0 };
- ifr->ifr_data = (char*)&args;
- if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
- virReportSystemError(errno,
- _("Unable to set bridge %s %s"), brname,
paramname);
- return -1;
- }
+ virReportSystemError(EINVAL,
+ _("Unable to set bridge %s %s via ioctl"),
+ brname, paramname);
+ return -1;
+ }
+ unsigned long args[] = { paramid, value, 0, 0 };
+ ifr->ifr_data = (char*)&args;
+ if (ioctl(fd, SIOCDEVPRIVATE, ifr) < 0) {
+ virReportSystemError(errno,
+ _("Failed to set bridge %s %s via ioctl"),
+ brname, paramname);
+ return -1;
}
return 0;
--
2.17.1
6:26 a.m.
New subject: [libvirt] [PATCH v2] util: netdevbridge: fall back to ioctl from sysfs
On Tue, Nov 20, 2018 at 01:25:46PM +0100, Christian Ehrhardt wrote:
There are certain cases e.g. containers where the sysfs path might
exists, but might fail. Unfortunately the exact restrictions are only
known to libvirt when trying to write to it so we need to try it.
But in case it fails there is no need to fully abort, in those cases try
to fall back to the older ioctl interface which can still work.
That makes setting up a bridge in unprivileged LXD containers work.
Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
Reported-by: Brian Candler <b.candler(a)pobox.com>
---
src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 22 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
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 :|
12:42 a.m.
New subject: [libvirt] [PATCH v2] util: netdevbridge: fall back to ioctl from sysfs
On Tue, Nov 20, 2018 at 1:26 PM Daniel P. Berrangé <berrange(a)redhat.com>
wrote:
On Tue, Nov 20, 2018 at 01:25:46PM +0100, Christian Ehrhardt wrote:
> There are certain cases e.g. containers where the sysfs path might
> exists, but might fail. Unfortunately the exact restrictions are only
> known to libvirt when trying to write to it so we need to try it.
>
> But in case it fails there is no need to fully abort, in those cases try
> to fall back to the older ioctl interface which can still work.
>
> That makes setting up a bridge in unprivileged LXD containers work.
>
> Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> Reported-by: Brian Candler <b.candler(a)pobox.com>
> ---
> src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
Thanks for the review Daniel!
Brian (on CC) also tested a Ubuntu build with the fix applied and it worked
for him in unprivileged containers.
There was no other feedback in the last three days.
But this is no area I feel entitled to push the change on my own, therefore
I wanted to ping on this - ping
3:01 p.m.
New subject: [libvirt] [PATCH v2] util: netdevbridge: fall back to ioctl from sysfs
On 11/23/18 1:42 AM, Christian Ehrhardt wrote:
On Tue, Nov 20, 2018 at 1:26 PM Daniel P. Berrangé
<berrange(a)redhat.com <mailto:berrange@redhat.com>> wrote:
On Tue, Nov 20, 2018 at 01:25:46PM +0100, Christian Ehrhardt wrote:
> There are certain cases e.g. containers where the sysfs path might
> exists, but might fail. Unfortunately the exact restrictions are
only
> known to libvirt when trying to write to it so we need to try it.
>
> But in case it fails there is no need to fully abort, in those
cases try
> to fall back to the older ioctl interface which can still work.
>
> That makes setting up a bridge in unprivileged LXD containers work.
>
> Fixes:
https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
>
> Signed-off-by: Christian Ehrhardt
<christian.ehrhardt(a)canonical.com
<mailto:christian.ehrhardt@canonical.com>>
> Reported-by: Brian Candler <b.candler(a)pobox.com
<mailto:b.candler@pobox.com>>
> ---
> src/util/virnetdevbridge.c | 48
+++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 22 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com
<mailto:berrange@redhat.com>>
Thanks for the review Daniel!
Brian (on CC) also tested a Ubuntu build with the fix applied and it
worked for him in unprivileged containers.
There was no other feedback in the last three days.
But this is no area I feel entitled to push the change on my own,
therefore I wanted to ping on this - ping
As long as you have commit privileges, feel free to push once there is a
Reviewed-by: (unless we are in freeze).
If it makes you feel any more confident about pushing - I had personally
expressed misgivings about this patch in IRC to Dan because on first
read it sounded like we might be exploiting a security flaw in LXC to
modify networking when it shouldn't actually be allowed, but he
convinced me that the situation isn't that "bridge and tap device
management via sysfs is blocked because it should be, and ioctls are
accidentally left enabled when they should have been disabled", but
rather that "bridge/tap device management is acceptable in this
situation, but sysfs is a huge can of worms that can only be made
read-only on a global basis (and *must* be made read-only due to all the
other things that shouldn't be allowed in this case)". Based on that,
I'm okay with the patch as well.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
12:52 a.m.
New subject: [libvirt] [PATCH v2] util: netdevbridge: fall back to ioctl from sysfs
On Sun, Nov 25, 2018 at 10:01 PM Laine Stump <laine(a)laine.org> wrote:
On 11/23/18 1:42 AM, Christian Ehrhardt wrote:
On Tue, Nov 20, 2018 at 1:26 PM Daniel P. Berrangé <berrange(a)redhat.com>
wrote:
> On Tue, Nov 20, 2018 at 01:25:46PM +0100, Christian Ehrhardt wrote:
> > There are certain cases e.g. containers where the sysfs path might
> > exists, but might fail. Unfortunately the exact restrictions are only
> > known to libvirt when trying to write to it so we need to try it.
> >
> > But in case it fails there is no need to fully abort, in those cases try
> > to fall back to the older ioctl interface which can still work.
> >
> > That makes setting up a bridge in unprivileged LXD containers work.
> >
> > Fixes: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1802906
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt(a)canonical.com>
> > Reported-by: Brian Candler <b.candler(a)pobox.com>
> > ---
> > src/util/virnetdevbridge.c | 48 +++++++++++++++++++++-----------------
> > 1 file changed, 26 insertions(+), 22 deletions(-)
>
> Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com>
>
Thanks for the review Daniel!
Brian (on CC) also tested a Ubuntu build with the fix applied and it
worked for him in unprivileged containers.
There was no other feedback in the last three days.
But this is no area I feel entitled to push the change on my own,
therefore I wanted to ping on this - ping
As long as you have commit privileges, feel free to push once there is a
Reviewed-by: (unless we are in freeze).
I wanted to be better safe than sorry, thanks for the confirmation.
If it makes you feel any more confident about pushing - I had
personally
expressed misgivings about this patch in IRC to Dan because on first read
it sounded like we might be exploiting a security flaw in LXC to modify
networking when it shouldn't actually be allowed, but he convinced me that
the situation isn't that "bridge and tap device management via sysfs is
blocked because it should be, and ioctls are accidentally left enabled when
they should have been disabled", but rather that "bridge/tap device
management is acceptable in this situation, but sysfs is a huge can of
worms that can only be made read-only on a global basis (and *must* be made
read-only due to all the other things that shouldn't be allowed in this
case)". Based on that, I'm okay with the patch as well.
Ack to the can-of-worms being the reason :-)
Thanks !
... pushed to master now
2223
days inactive
2229
days old
9 comments
3 participants
participants (3)
-
Christian Ehrhardt -
Daniel P. Berrangé -
Laine Stump