[libvirt] [PATCH] Revert "utils: Remove the logging of errors from virNetDevSendEthtoolIoctl"

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> --- 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 9789e93..657df3a 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3145,15 +3145,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; } @@ -3170,12 +3194,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; } @@ -3192,12 +3216,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.5.0

On 11/03/2015 09:17 AM, Daniel P. Berrange wrote:
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.
A bit of history... Going back through the timeline... Moshe Levi proposes patches to add support for new feature/bits: http://www.redhat.com/archives/libvir-list/2015-June/msg00921.html reviewed and posted v2 in July: http://www.redhat.com/archives/libvir-list/2015-July/msg00548.html Changes get pushed; however, they lead to an issue: http://www.redhat.com/archives/libvir-list/2015-August/msg00162.html Moshe provided a patch to resolve that: http://www.redhat.com/archives/libvir-list/2015-August/msg00263.html Laine has a different proposal: http://www.redhat.com/archives/libvir-list/2015-August/msg00382.html which after review gets shortened a bit: http://www.redhat.com/archives/libvir-list/2015-August/msg00471.html and pushed Moshe as an update to his v1 msg00263 http://www.redhat.com/archives/libvir-list/2015-August/msg00567.html Comments from there lead to posting changes just for the ioctl errors: http://www.redhat.com/archives/libvir-list/2015-August/msg00625.html Which leads to the v2 of that series (which you're proposing to revert): http://www.redhat.com/archives/libvir-list/2015-August/msg00720.html I suppose when I reviewed I just saw using virNetDevSetupControl as way to use existing functions as opposed to calling socket() directly. It was something different between v2 and v1 of that change, but other than the additional virSetInherit call - it was essentially the same. Even with the revert, in the unpriv'd libvirt, wouldn't the error: virReportSystemError(errno, "%s", _("Cannot open control socket")); still still be spewed? Since virNetDevSetupControlFull:148 is the socket call failure and not the ioctl failure. Sure the caller wouldn't spew a second one, but why would the first one not be spewed? Seems another option would be to add a "!virGetLastError()" prior to the virReportSystemError in the callers (at least to avoid the two error messages. Although there was a exchange between Martin and Moshe about that. The whole reason for the removal of the switch on ioctl() failure was detailed in one of the responses to the link above. It wasn't clear why we would filter on certain errno values, although I suppose the unpriv'd libvirt path makes it more understandable. Still the socket() call failure path would be hit first. John
Signed-off-by: Daniel P. Berrange <berrange@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 9789e93..657df3a 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3145,15 +3145,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; }
@@ -3170,12 +3194,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; }
@@ -3192,12 +3216,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

On Tue, Nov 03, 2015 at 10:58:58AM -0500, John Ferlan wrote:
On 11/03/2015 09:17 AM, Daniel P. Berrange wrote:
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.
A bit of history...
Going back through the timeline... Moshe Levi proposes patches to add support for new feature/bits:
http://www.redhat.com/archives/libvir-list/2015-June/msg00921.html
reviewed and posted v2 in July:
http://www.redhat.com/archives/libvir-list/2015-July/msg00548.html
Changes get pushed; however, they lead to an issue:
http://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
Moshe provided a patch to resolve that:
http://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
Laine has a different proposal:
http://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
which after review gets shortened a bit:
http://www.redhat.com/archives/libvir-list/2015-August/msg00471.html
and pushed
Moshe as an update to his v1 msg00263
http://www.redhat.com/archives/libvir-list/2015-August/msg00567.html
Comments from there lead to posting changes just for the ioctl errors:
http://www.redhat.com/archives/libvir-list/2015-August/msg00625.html
Which leads to the v2 of that series (which you're proposing to revert):
http://www.redhat.com/archives/libvir-list/2015-August/msg00720.html
I suppose when I reviewed I just saw using virNetDevSetupControl as way to use existing functions as opposed to calling socket() directly. It was something different between v2 and v1 of that change, but other than the additional virSetInherit call - it was essentially the same.
Even with the revert, in the unpriv'd libvirt, wouldn't the error:
virReportSystemError(errno, "%s", _("Cannot open control socket"));
still still be spewed? Since virNetDevSetupControlFull:148 is the socket call failure and not the ioctl failure. Sure the caller wouldn't spew a second one, but why would the first one not be spewed?
Empirically I don't see any errors after reverting it. It appears the reason for this is that the original code uses AF_LOCAL while the virNetDevSetupControllFull uses AF_PACKET, and the latter is restricted to only privileged users. If I change the original code to use AF_PACKET then it shows errors too. The reason for using AF_PACKET was that it was substaintially faster than AF_LOCAL 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 :|

On 11/03/2015 11:15 AM, Daniel P. Berrange wrote:
On Tue, Nov 03, 2015 at 10:58:58AM -0500, John Ferlan wrote:
On 11/03/2015 09:17 AM, Daniel P. Berrange wrote:
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.
A bit of history...
Going back through the timeline... Moshe Levi proposes patches to add support for new feature/bits:
http://www.redhat.com/archives/libvir-list/2015-June/msg00921.html
reviewed and posted v2 in July:
http://www.redhat.com/archives/libvir-list/2015-July/msg00548.html
Changes get pushed; however, they lead to an issue:
http://www.redhat.com/archives/libvir-list/2015-August/msg00162.html
Moshe provided a patch to resolve that:
http://www.redhat.com/archives/libvir-list/2015-August/msg00263.html
Laine has a different proposal:
http://www.redhat.com/archives/libvir-list/2015-August/msg00382.html
which after review gets shortened a bit:
http://www.redhat.com/archives/libvir-list/2015-August/msg00471.html
and pushed
Moshe as an update to his v1 msg00263
http://www.redhat.com/archives/libvir-list/2015-August/msg00567.html
Comments from there lead to posting changes just for the ioctl errors:
http://www.redhat.com/archives/libvir-list/2015-August/msg00625.html
Which leads to the v2 of that series (which you're proposing to revert):
http://www.redhat.com/archives/libvir-list/2015-August/msg00720.html
I suppose when I reviewed I just saw using virNetDevSetupControl as way to use existing functions as opposed to calling socket() directly. It was something different between v2 and v1 of that change, but other than the additional virSetInherit call - it was essentially the same.
Even with the revert, in the unpriv'd libvirt, wouldn't the error:
virReportSystemError(errno, "%s", _("Cannot open control socket"));
still still be spewed? Since virNetDevSetupControlFull:148 is the socket call failure and not the ioctl failure. Sure the caller wouldn't spew a second one, but why would the first one not be spewed?
Empirically I don't see any errors after reverting it. It appears the reason for this is that the original code uses AF_LOCAL while the virNetDevSetupControllFull uses AF_PACKET, and the latter is restricted to only privileged users. If I change the original code to use AF_PACKET then it shows errors too. The reason for using AF_PACKET was that it was substaintially faster than AF_LOCAL
Ah - I see VIR_NETDEV_FAMILY used by virNetDevSetupControl is what hides this... #ifdef __linux__ # include <linux/sockios.h> # include <linux/if_vlan.h> # define VIR_NETDEV_FAMILY AF_PACKET #elif defined(HAVE_STRUCT_IFREQ) && defined(AF_LOCAL) # define VIR_NETDEV_FAMILY AF_LOCAL #else # undef HAVE_STRUCT_IFREQ #endif Well hopefully no one falls into this rabbit hole in the future as it's not well marked ;-) ACK to the revert John

On 11/03/2015 11:15 AM, Daniel P. Berrange wrote: [...]
still still be spewed? Since virNetDevSetupControlFull:148 is the socket call failure and not the ioctl failure. Sure the caller wouldn't spew a second one, but why would the first one not be spewed?
Empirically I don't see any errors after reverting it. It appears the reason for this is that the original code uses AF_LOCAL while the virNetDevSetupControllFull uses AF_PACKET, and the latter is restricted to only privileged users. If I change the original code to use AF_PACKET then it shows errors too. The reason for using AF_PACKET was that it was substaintially faster than AF_LOCAL
Alternatively in order to not fall into the rabbit hole again... Can we determine if we're not privileged at the start of virNetDevSendEthtoolIoctl, eg if (geteuid() != 0) { VIR_{WARN|INFO}("cannot set feature in unprivileged daemon"); cmd->data = 0; /* To indicate not found (e.g. lie) */ return 0; } or follow virNetDevBandwidthSet which causes an error. IOW: Should we allow these bits to be set in a non privileged daemon? And if not should we error on them? For the concern about the two error messages, just remove the virReportSystemError from the callers and adjust the ioctl() to: if ((ret = ioctl(fd, SIOCETHTOOL, &ifr)) < 0) { virReportSystemError(errno, "%s", _("ethtool ioctl error")); John
participants (2)
-
Daniel P. Berrange
-
John Ferlan