[libvirt] [PATCH 0/3] Alternative proposal to revert the removal of logging of errors from virNetDevSendEthtoolIoctl
Rather than revert as proposed here: http://www.redhat.com/archives/libvir-list/2015-November/msg00082.html Let's make some adjustments to the existing algorithm. The only question I don't have answered in my mind is whether an unprivileged daemon that has an interface with the feature bits defined should be allowed to start. Historically we haven't stopped it, so doing so now may not be right/good. Conversely, someone may "believe" their bits are set on their device when in fact they are not. Of course it's not documented on formatnode.html that setting the feature bits requires root. I did find through various searches that SIOCETHTOOL doesn't work unless it's run under root. John Ferlan (3): virnetdev: Check correct return value for virNetDevFeatureAvailable virnetdev: Message in virNetDevSendEthtoolIoctl rather than caller. virnetdev: Add check for unprivileged daemon src/util/virnetdev.c | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) -- 2.1.0
Rather than "if (virNetDevFeatureAvailable(ifname, &cmd))" change the success criteria to "if (virNetDevFeatureAvailable(ifname, &cmd) == 1)". The called helper returns -1 on failure, 0 on not found, and 1 on found. Thus a failure was setting bits. 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 ab00605..12faf51 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3250,7 +3250,7 @@ virNetDevGetFeatures(const char *ifname, for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { cmd.cmd = cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd)) + if (virNetDevFeatureAvailable(ifname, &cmd) == 1) ignore_value(virBitmapSetBit(*out, cmds[i].feat)); } @@ -3274,7 +3274,7 @@ virNetDevGetFeatures(const char *ifname, }; cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd)) { + if (virNetDevFeatureAvailable(ifname, &cmd) == 1) { for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { if (cmd.data & flags[j].cmd) ignore_value(virBitmapSetBit(*out, flags[j].feat)); @@ -3288,7 +3288,7 @@ virNetDevGetFeatures(const char *ifname, return -1; g_cmd->cmd = ETHTOOL_GFEATURES; g_cmd->size = GFEATURES_SIZE; - if (virNetDevGFeatureAvailable(ifname, g_cmd)) + if (virNetDevGFeatureAvailable(ifname, g_cmd) == 1) ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL)); VIR_FREE(g_cmd); # endif -- 2.1.0
On Tue, Nov 03, 2015 at 07:18:09PM -0500, John Ferlan wrote:
Rather than "if (virNetDevFeatureAvailable(ifname, &cmd))" change the success criteria to "if (virNetDevFeatureAvailable(ifname, &cmd) == 1)".
The called helper returns -1 on failure, 0 on not found, and 1 on found. Thus a failure was setting bits.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK, introduced by commit ac3ed20 which changed the helper's return values without adjusting its callers. Jan
Since virNetDevSetupControl can generate a virReportSystemError, rather than message in the caller for any errors. Do all the messaging in the virNetDevSendEthtoolIoctl. This change partially reverts commit id '6f2a0198' by moving the error message in the virNetDev[G]FeatureAvailable to where the ioctl fails. However, the ioctl will report any error rather than filtering some. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 12faf51..d47859e 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3157,7 +3157,8 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return ret; ifr.ifr_data = cmd; - ret = ioctl(fd, SIOCETHTOOL, &ifr); + if ((ret = ioctl(fd, SIOCETHTOOL, &ifr)) < 0) + virReportSystemError(errno, "%s", _("ethtool ioctl error")); VIR_FORCE_CLOSE(fd); return ret; @@ -3176,12 +3177,13 @@ 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 +3200,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
On Tue, Nov 03, 2015 at 07:18:10PM -0500, John Ferlan wrote:
Since virNetDevSetupControl can generate a virReportSystemError, rather than message in the caller for any errors. Do all the messaging in the virNetDevSendEthtoolIoctl.
This change partially reverts commit id '6f2a0198' by moving the error message in the virNetDev[G]FeatureAvailable to where the ioctl fails. However, the ioctl will report any error rather than filtering some.
I think we should not pollute the logs for some error codes and just VIR_DEBUG the error, even in privileged mode. On the other hand, the proposed revert unnecessarily reintroduces the use of AF_LOCAL, but that's because the orignal commit does two things at once. Jan
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 12faf51..d47859e 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3157,7 +3157,8 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return ret; ifr.ifr_data = cmd; - ret = ioctl(fd, SIOCETHTOOL, &ifr); + if ((ret = ioctl(fd, SIOCETHTOOL, &ifr)) < 0) + virReportSystemError(errno, "%s", _("ethtool ioctl error"));
VIR_FORCE_CLOSE(fd); return ret; @@ -3176,12 +3177,13 @@ 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 +3200,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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 11/04/2015 04:39 AM, Ján Tomko wrote:
On Tue, Nov 03, 2015 at 07:18:10PM -0500, John Ferlan wrote:
Since virNetDevSetupControl can generate a virReportSystemError, rather than message in the caller for any errors. Do all the messaging in the virNetDevSendEthtoolIoctl.
This change partially reverts commit id '6f2a0198' by moving the error message in the virNetDev[G]FeatureAvailable to where the ioctl fails. However, the ioctl will report any error rather than filtering some.
The shorter version - ditch patch 2 & 3 of this series, revert as Dan suggests, then add new series to 1. Avoid calling when not privileged from udevProcessNetworkInterface (node_device_udev.c), but not from update_caps (e.g. called from nodeDeviceGetXMLDesc) 2. Use virNetDevSetupControl instead of socket as well as adding a couple of comments regarding the skipped errno values. However, this could cause issues for the XMLDesc path for unprivileged daemons.
I think we should not pollute the logs for some error codes and just VIR_DEBUG the error, even in privileged mode.
I think a question I never got answered when posed during review of other related changes is why we were filtering certain error codes: http://www.redhat.com/archives/libvir-list/2015-August/msg00584.html However, based on the unprivileged daemon I think I now know part of the reason. Digging further back into the patch series that added support for the ethtool ioctl, there's another hint: http://www.redhat.com/archives/libvir-list/2015-February/msg00506.html When v2 was posted - the ReportSystemError's were changed to VIR_DEBUG's: http://www.redhat.com/archives/libvir-list/2015-February/msg00881.html when pushed those VIR_DEBUG's were slightly adjusted. So, it seems EPERM is to avoid the unprivileged daemon run, EINVAL seems to be because a kernel doesn't support SIOCETHTOOL, and EOPNOTSUPP is when a requested feature check is not supported in the kernel. As an aside - this is one of the reasons why it's "nice" to have either in the comments or the commit message a reason why decisions to filter certain messages were made. The code isn't self documenting. Future changes to the code then won't have to assume, wonder, and/or adjust the code "incorrectly" from initial design. John
On the other hand, the proposed revert unnecessarily reintroduces the use of AF_LOCAL, but that's because the orignal commit does two things at once.
Jan
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 12faf51..d47859e 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3157,7 +3157,8 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return ret; ifr.ifr_data = cmd; - ret = ioctl(fd, SIOCETHTOOL, &ifr); + if ((ret = ioctl(fd, SIOCETHTOOL, &ifr)) < 0) + virReportSystemError(errno, "%s", _("ethtool ioctl error"));
VIR_FORCE_CLOSE(fd); return ret; @@ -3176,12 +3177,13 @@ 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 +3200,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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Nov 04, 2015 at 10:18:27AM -0500, John Ferlan wrote:
On 11/04/2015 04:39 AM, Ján Tomko wrote:
On Tue, Nov 03, 2015 at 07:18:10PM -0500, John Ferlan wrote:
Since virNetDevSetupControl can generate a virReportSystemError, rather than message in the caller for any errors. Do all the messaging in the virNetDevSendEthtoolIoctl.
This change partially reverts commit id '6f2a0198' by moving the error message in the virNetDev[G]FeatureAvailable to where the ioctl fails. However, the ioctl will report any error rather than filtering some.
The shorter version - ditch patch 2 & 3 of this series, revert as Dan suggests, then add new series to
1. Avoid calling when not privileged from udevProcessNetworkInterface (node_device_udev.c), but not from update_caps (e.g. called from nodeDeviceGetXMLDesc)
Both code paths should be avoided for unprivileged daemons.
2. Use virNetDevSetupControl instead of socket as well as adding a couple of comments regarding the skipped errno values. However, this could cause issues for the XMLDesc path for unprivileged daemons.
I think we should not pollute the logs for some error codes and just VIR_DEBUG the error, even in privileged mode.
I think a question I never got answered when posed during review of other related changes is why we were filtering certain error codes:
http://www.redhat.com/archives/libvir-list/2015-August/msg00584.html
However, based on the unprivileged daemon I think I now know part of the reason. Digging further back into the patch series that added support for the ethtool ioctl, there's another hint:
http://www.redhat.com/archives/libvir-list/2015-February/msg00506.html
When v2 was posted - the ReportSystemError's were changed to VIR_DEBUG's:
http://www.redhat.com/archives/libvir-list/2015-February/msg00881.html
when pushed those VIR_DEBUG's were slightly adjusted.
So, it seems EPERM is to avoid the unprivileged daemon run, EINVAL seems to be because a kernel doesn't support SIOCETHTOOL, and EOPNOTSUPP is when a requested feature check is not supported in the kernel. As an aside - this is one of the reasons why it's "nice" to have either in the comments or the commit message a reason why decisions to filter certain messages were made. The code isn't self documenting. Future changes to the code then won't have to assume, wonder, and/or adjust the code "incorrectly" from initial design.
Yes. Jan
Using virNetDevSetupControl (due to usage of AF_PACKET instead of AF_LOCAL for the socket call) or even ioctl(SIOCETHTOOL) will fail for an unprivileged daemon. Since in the long run the caller only cares if bits are found in order to set a bit in a bitmap, let's just add a VIR_WARN instead of an error message. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index d47859e..acf8ba6 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3154,6 +3154,14 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) int fd; struct ifreq ifr; + /* Won't work if not running as root. Rather than generate + * error, just WARN and return. + */ + if (geteuid() != 0) { + VIR_WARN("cannot get ETHTOOL feature bits"); + return ret; + } + if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) return ret; ifr.ifr_data = cmd; -- 2.1.0
On Tue, Nov 03, 2015 at 07:18:11PM -0500, John Ferlan wrote:
Using virNetDevSetupControl (due to usage of AF_PACKET instead of AF_LOCAL for the socket call) or even ioctl(SIOCETHTOOL) will fail for an unprivileged daemon. Since in the long run the caller only cares if bits are found in order to set a bit in a bitmap, let's just add a VIR_WARN instead of an error message.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virnetdev.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index d47859e..acf8ba6 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3154,6 +3154,14 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) int fd; struct ifreq ifr;
+ /* Won't work if not running as root. Rather than generate + * error, just WARN and return. + */ + if (geteuid() != 0) { + VIR_WARN("cannot get ETHTOOL feature bits"); + return ret; + } +
I think rather than this, we should patch node_Device_udev.c to simply not call this API when running unprivileged 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 (3)
-
Daniel P. Berrange -
John Ferlan -
Ján Tomko