10:39 a.m.
It's partially a reposting, partially some new stuff, and perhaps a v2
of something else.
Patch 1 is Dan's revert patch - just because it's easier to have it
done first.
Patches 2 & 3 just adjust comments - no functional changes
Patch 4 adds a check for root in virNetDevGetFeatures (rather than
both of the callers). If not root, an empty bitmap would be returned
(somewhat similar to what a platform without the features does). I'm
not against moving the check to before the virBitmapNew; however, I
was considering the case of xml parsing which does a virBitmapFree,
then just calls virNetDevGetFeatures. Not filling in the *out with
either NULL or an empty bitmap would result in that caller referencing
a net.features which no longer exists. If the check should be done
before the virBitmapNew, then an "*out = NULL" would need to be
added before returning 0 just to be 'safe'...
As an aside, it just felt better to add one check rather than two.
Patch 5 restore the virNetDevSetupControl from the reverted patch
back into virNetDevSendEthtoolIoctl. Since we cannot be called unless
we're privileged and the ioctl shouldn't fail any with EPERM, that
was also removed from the list of filtered failure errno values.
Daniel P. Berrange (1):
Revert "utils: Remove the logging of errors from
virNetDevSendEthtoolIoctl"
John Ferlan (4):
virnetdev: Document reasons for ignoring some SIOCETHTOOL errno values
virnetdev: Fix function comments for virNetDevGetFeatures
virnetdev: Check for root in virNetDevGetFeatures
virnetdev: Use virNetDevSetupControl in virNetDevSendEthtoolIoctl
src/util/virnetdev.c | 52 ++++++++++++++++++++++++++++++++++++++--------------
1 file changed, 38 insertions(+), 14 deletions(-)
--
2.1.0
10:39 a.m.
New subject: [libvirt] [PATCH 1/5] Revert "utils: Remove the logging of errors from virNetDevSendEthtoolIoctl"
From: "Daniel P. Berrange" <berrange(a)redhat.com>
This reverts commit 6f2a0198e913c91a2ef8b99db79b7d3cc5396957.
This commit removed error reporting from virNetDevSendEthtoolIoctl
pushing responsibility onto the callers. This is wrong, however,
since virNetDevSendEthtoolIoctl calls virNetDevSetupControl
which can still report errors. So as a result virNetDevSendEthtoolIoctl
may or may not report errors depending on which bit of it fails, and as
a result callers now overwrite some errors.
It also introduced a regression causing unprivileged libvirtd to
spew error messages to the console due to inability to query the
NIC features, an error which was previously ignored.
virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation
not permitted
virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted
virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation
not permitted
virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted
virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation
not permitted
virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted
virNetDevSetupControlFull:148 : Cannot open network interface control socket: Operation
not permitted
virNetDevFeatureAvailable:3062 : Cannot get device wlp3s0 flags: Operation not permitted
Looking back at the original posting I see no explanation of why
thsi refactoring was needed, so reverting the clearly broken
error reporting logic looks like the best option.
Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com>
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virnetdev.c | 56 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 12faf51..971e24d 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3151,15 +3151,39 @@ static int
virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
{
int ret = -1;
- int fd;
- struct ifreq ifr;
+ int sock = -1;
+ virIfreq ifr;
- if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
- return ret;
+ sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
+ if (sock < 0) {
+ virReportSystemError(errno, "%s", _("Cannot open control
socket"));
+ goto cleanup;
+ }
+
+ memset(&ifr, 0, sizeof(ifr));
+ strcpy(ifr.ifr_name, ifname);
ifr.ifr_data = cmd;
- ret = ioctl(fd, SIOCETHTOOL, &ifr);
+ ret = ioctl(sock, SIOCETHTOOL, &ifr);
+ if (ret != 0) {
+ switch (errno) {
+ case EPERM:
+ VIR_DEBUG("ethtool ioctl: permission denied");
+ break;
+ case EINVAL:
+ VIR_DEBUG("ethtool ioctl: invalid request");
+ break;
+ case EOPNOTSUPP:
+ VIR_DEBUG("ethtool ioctl: request not supported");
+ break;
+ default:
+ virReportSystemError(errno, "%s", _("ethtool ioctl
error"));
+ goto cleanup;
+ }
+ }
- VIR_FORCE_CLOSE(fd);
+ cleanup:
+ if (sock)
+ VIR_FORCE_CLOSE(sock);
return ret;
}
@@ -3176,12 +3200,12 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
static int
virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd)
{
+ int ret = -1;
+
cmd = (void*)cmd;
- if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) {
- virReportSystemError(errno, _("Cannot get device %s flags"), ifname);
- return -1;
- }
- return cmd->data > 0 ? 1 : 0;
+ if (!virNetDevSendEthtoolIoctl(ifname, cmd))
+ ret = cmd->data > 0 ? 1 : 0;
+ return ret;
}
@@ -3198,12 +3222,12 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value
*cmd)
static int
virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
{
+ int ret = -1;
+
cmd = (void*)cmd;
- if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) {
- virReportSystemError(errno, _("Cannot get device %s generic features"),
ifname);
- return -1;
- }
- return FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
+ if (!virNetDevSendEthtoolIoctl(ifname, cmd))
+ ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active);
+ return ret;
}
# endif
--
2.1.0
10:39 a.m.
New subject: [libvirt] [PATCH 2/5] virnetdev: Document reasons for ignoring some SIOCETHTOOL errno values
Recently reverted commit id '6f2a0198' showed a need to add extra
comments when dealing with filtering of potential "non-issues".
Scanning through upstream patch postings indicates early on the
reasons for the filtering of specific ioctl failures were provided;
however, when converted from causing an error to VIR_DEBUG's the
reasons were missing. A future read/change of the code incorrectly
assumed they could or should be removed.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virnetdev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 971e24d..3bd7dbb 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3166,13 +3166,13 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
ret = ioctl(sock, SIOCETHTOOL, &ifr);
if (ret != 0) {
switch (errno) {
- case EPERM:
+ case EPERM: /* attempt to call SIOCETHTOOL from unprivileged code */
VIR_DEBUG("ethtool ioctl: permission denied");
break;
- case EINVAL:
+ case EINVAL: /* kernel doesn't support SIOCETHTOOL */
VIR_DEBUG("ethtool ioctl: invalid request");
break;
- case EOPNOTSUPP:
+ case EOPNOTSUPP: /* kernel doesn't support specific feature */
VIR_DEBUG("ethtool ioctl: request not supported");
break;
default:
--
2.1.0
10:39 a.m.
New subject: [libvirt] [PATCH 3/5] virnetdev: Fix function comments for virNetDevGetFeatures
In commit id 'c9027d8f4' when updating the posted patch to generate
a bitmap instead of an array of named feature bits, adjustment of
the args was missed
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virnetdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 3bd7dbb..fb367e7 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3237,8 +3237,7 @@ virNetDevGFeatureAvailable(const char *ifname, struct
ethtool_gfeatures *cmd)
* This function gets the nic offloads features available for ifname
*
* @ifname: name of the interface
- * @features: network device feature structures
- * @nfeatures: number of features available
+ * @out: bitmap of the available virNetDevFeature feature bits
*
* Returns 0 on success, -1 on failure.
*/
--
2.1.0
10:39 a.m.
New subject: [libvirt] [PATCH 4/5] virnetdev: Check for root in virNetDevGetFeatures
Since the SIOCETHTOOL ioctl only works for privileged daemons, if called
when not root, then virNetDevGetFeatures will VIR_DEBUG a message and
return 0 as if the functions were not available for the architecture.
This effectively returns an empty bitmap indicating no features available.
Introduced by commit id 'c9027d8f4'
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virnetdev.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index fb367e7..0bc1a6f 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3239,7 +3239,8 @@ virNetDevGFeatureAvailable(const char *ifname, struct
ethtool_gfeatures *cmd)
* @ifname: name of the interface
* @out: bitmap of the available virNetDevFeature feature bits
*
- * Returns 0 on success, -1 on failure.
+ * Returns 0 on success or if called from session mode, -1 on failure.
+ * If called from session mode, an empty bitmap is returned.
*/
int
virNetDevGetFeatures(const char *ifname,
@@ -3271,6 +3272,12 @@ virNetDevGetFeatures(const char *ifname,
if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST)))
return -1;
+ /* Only fetch features if we're privileged, but no need to fail */
+ if (geteuid() != 0) {
+ VIR_DEBUG("ETHTOOL feature bits not available in session mode");
+ return 0;
+ }
+
for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) {
cmd.cmd = cmds[i].cmd;
if (virNetDevFeatureAvailable(ifname, &cmd) == 1)
--
2.1.0
10:39 a.m.
New subject: [libvirt] [PATCH 5/5] virnetdev: Use virNetDevSetupControl in virNetDevSendEthtoolIoctl
Use virNetDevSetupControl instead of open coding using socket(AF_LOCAL...)
and clearing virIfreq.
By using virNetDevSetupControl, the socket is then opened using
AF_PACKET which requires being privileged (effectively root) in
order to complete successfully. Since that's now a requirement,
then the ioctl(SIOCETHTOOL) should not fail with EPERM, thus it
is removed from the filtered listed of failure codes.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virnetdev.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 0bc1a6f..ade9afa 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -3151,24 +3151,19 @@ static int
virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
{
int ret = -1;
- int sock = -1;
- virIfreq ifr;
+ int fd = -1;
+ struct ifreq ifr;
- sock = socket(AF_LOCAL, SOCK_DGRAM, 0);
- if (sock < 0) {
- virReportSystemError(errno, "%s", _("Cannot open control
socket"));
- goto cleanup;
- }
+ /* Ultimately uses AF_PACKET for socket which requires privileged
+ * daemon support.
+ */
+ if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0)
+ return ret;
- memset(&ifr, 0, sizeof(ifr));
- strcpy(ifr.ifr_name, ifname);
ifr.ifr_data = cmd;
- ret = ioctl(sock, SIOCETHTOOL, &ifr);
+ ret = ioctl(fd, SIOCETHTOOL, &ifr);
if (ret != 0) {
switch (errno) {
- case EPERM: /* attempt to call SIOCETHTOOL from unprivileged code */
- VIR_DEBUG("ethtool ioctl: permission denied");
- break;
case EINVAL: /* kernel doesn't support SIOCETHTOOL */
VIR_DEBUG("ethtool ioctl: invalid request");
break;
@@ -3182,8 +3177,7 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd)
}
cleanup:
- if (sock)
- VIR_FORCE_CLOSE(sock);
+ VIR_FORCE_CLOSE(fd);
return ret;
}
--
2.1.0
10:49 a.m.
On Fri, Nov 06, 2015 at 11:39:26AM -0500, John Ferlan wrote:
It's partially a reposting, partially some new stuff, and perhaps
a v2
of something else.
Patch 1 is Dan's revert patch - just because it's easier to have it
done first.
Patches 2 & 3 just adjust comments - no functional changes
Patch 4 adds a check for root in virNetDevGetFeatures (rather than
both of the callers). If not root, an empty bitmap would be returned
(somewhat similar to what a platform without the features does). I'm
not against moving the check to before the virBitmapNew; however, I
was considering the case of xml parsing which does a virBitmapFree,
then just calls virNetDevGetFeatures. Not filling in the *out with
either NULL or an empty bitmap would result in that caller referencing
a net.features which no longer exists. If the check should be done
before the virBitmapNew, then an "*out = NULL" would need to be
added before returning 0 just to be 'safe'...
As an aside, it just felt better to add one check rather than two.
Patch 5 restore the virNetDevSetupControl from the reverted patch
back into virNetDevSendEthtoolIoctl. Since we cannot be called unless
we're privileged and the ioctl shouldn't fail any with EPERM, that
was also removed from the list of filtered failure errno values.
ACK to all 5. THanks for picking this work up.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3305
days inactive
3305
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrange -
John Ferlan