[libvirt] [PATCH 0/5] Revert virNetDevSendEthtoolIoctl logging errors and adjust code after
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
From: "Daniel P. Berrange" <berrange@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@redhat.com> Signed-off-by: John Ferlan <jferlan@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
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@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
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@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
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@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
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@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
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 :|
participants (2)
-
Daniel P. Berrange -
John Ferlan