> -----Original Message----- > From: Laine Stump [mailto:laine@xxxxxxxxx] > Sent: Tuesday, August 11, 2015 7:10 AM > To: libvir-list@xxxxxxxxxx > Cc: Moshe Levi; Brian Rak; james.p.chapman@xxxxxxxxx > Subject: [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers > > This started out as a fix for a crash reported in IRC and on libvir-list: > > https://www.redhat.com/archives/libvir-list/2015-August/msg00162.html > > but as I examined the existing code I found so many nits to pick that I just did > them all. > > The most important fix here is that virNetDevGetFeatures was creating a > struct ethtool_gfeatures object as a local on the stack and, although the > struct is defined with 0 elements in its features array, we were telling the > ethtool ioctl that we had made space for 2 elements. This led to a crash, as > outlined in the email above. The fix for this is to allocate the memory for the > ethtool_gfeatures object using VIR_ALLOC_N to a char*, giving it the length: I test it on rhel7 several time with enabling and disabling the TX_UDP_TNL > > sizeof(ethtool_gfeatures) + (2 * sizeof(ethtool_get_features_block) > > because VIR_ALLOC_N is a macro and fails when you try to typecast a pointer > to char* within the invocation, I made a union that has both a > char* and an ethtool_gfeatures*, and used the char* of the union for > VIR_ALLOC_N, and the other for everything else. > > Beyond that crash fixer, the following fixups were made to the hierarchy of > functions between virNetDevGetFeatures() and > virNetDevSendEthtoolIoctl(): > > * macros used to examine the gfeatures bits were renamed from > FEATURE_* to GFEATURE_* > > virNetDevSentEthtoolIoctl(): > > * no need to initialize sock to -1, since it is always set at the top > of the function. > > * remove VIR_DEBUG log (and subsequent *skipping* of error log!) when > errno is EPERM, EINVAL or EOPNOTSUPP. If one of those errors were > ever encountered, this would have been *very* problematic, as it > would have led to one of those situations where virsh reports "an > error was encountered but the cause is unknown" (or whatever the > message is when we have an error but no log message). > > * always call VIR_FORCE_CLOSE(sock) since we know that sock is either > a valid fd, or else -1 (which VIR_FORCE_CLOSE() will skip). > > virNetDevFeatureAvailable() > > * simplify it - no need for ret. > > * follow libvirt convention of checking for "bobLobLaw(lawblog) < 0" > instead of "!bobLobLaw(lawblog)". > > virNetDevGFeatureAvailable() > > * eliminate this function, as it was ill-conceived (it really was only > checking for one gfeature (TX_UDP_TNL), not *any* gfeature. > > virNetDevGetFeatures() > > * move all data tables (struct elem) out of the function so that they > will be statics instead of taking up space on the stack. > > * remove pointless/incorrect initialization of i = -1. > > * remove unnecessary static initialization of struct ethtool_value cmd > > * put struct ethtool_gfeatures into a union with a char* as described above > > * use libvirt convention of checking return from > virNetDevFeatureAvailable() < 0, instead of > "!virNetDevFeatureAvailable()", and immediately return to caller > with an error when virNetDevFeatureAvailable() returns an error > (previously it was ignored). > > * remove superfluous size_t j, and just re-use i instead. > > * runtime allocation/free of proper size object for ethtoolfeatures as > described above. > > * directly call virNetDevSentEthtoolIoctl() instead of now defunct > virNetDevGFeatureAvailable(). > > === > > NB: This patch does *not* attempt to determine the proper number of > elements for the gfeature array at runtime, as proposed in this patch: > > https://www.redhat.com/archives/libvir-list/2015-August/msg00263.html > > since that wasn't the cause of the crash. I'll leave it up to Moshe to repost > that patch rebased around this one (or whatever ends up being > pushed) if he thinks there is value to it. > > Also - as I mentioned in earlier mail in response to the crash, I noticed when > looking into the gfeatures ethtool code that it looks to me like TX_UDP_TNL > should actually be 26 rather than 25, but I may be missing something. Moshe > - can you either confirm or deny that? Where did you get the value 25 from? I answer to your question on my patch https://www.redhat.com/archives/libvir-list/2015-August/msg00389.html > --- > src/util/virnetdev.c | 182 ++++++++++++++++++++++++------------------------ > --- > 1 file changed, 84 insertions(+), 98 deletions(-) > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 1e20789..7f0837d > 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -91,10 +91,10 @@ VIR_LOG_INIT("util.netdev"); #if > HAVE_DECL_ETHTOOL_GFEATURES # define TX_UDP_TNL 25 # define > GFEATURES_SIZE 2 -# define FEATURE_WORD(blocks, index, field) > ((blocks)[(index) / 32U].field) > -# define FEATURE_FIELD_FLAG(index) (1U << (index) % 32U) > -# define FEATURE_BIT_IS_SET(blocks, index, field) \ > - (FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index)) > +# define GFEATURE_WORD(blocks, index, field) ((blocks)[(index) / > 32U].field) > +# define GFEATURE_FIELD_FLAG(index) (1U << (index) % 32U) > +# define GFEATURE_BIT_IS_SET(blocks, index, field) \ > + (GFEATURE_WORD(blocks, index, field) & > GFEATURE_FIELD_FLAG(index)) > #endif > > typedef enum { > @@ -3032,11 +3032,10 @@ static int > virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) { > int ret = -1; > - int sock = -1; > + int sock; > virIfreq ifr; > > - sock = socket(AF_LOCAL, SOCK_DGRAM, 0); > - if (sock < 0) { > + if ((sock = socket(AF_LOCAL, SOCK_DGRAM, 0)) < 0) { > virReportSystemError(errno, "%s", _("Cannot open control socket")); > goto cleanup; > } > @@ -3044,27 +3043,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, > void *cmd) > memset(&ifr, 0, sizeof(ifr)); > strcpy(ifr.ifr_name, ifname); > ifr.ifr_data = cmd; > - 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; > - } > - } > + if ((ret = ioctl(sock, SIOCETHTOOL, &ifr)) < 0) > + virReportSystemError(errno, "%s", _("ethtool ioctl error")); > > cleanup: > - if (sock) > - VIR_FORCE_CLOSE(sock); > + VIR_FORCE_CLOSE(sock); > return ret; > } > > @@ -3081,35 +3064,50 @@ 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)) > - ret = cmd->data > 0 ? 1 : 0; > - return ret; > + if (virNetDevSendEthtoolIoctl(ifname, cmd) < 0) > + return -1; > + return cmd->data > 0 ? 1 : 0; > } > > > -# if HAVE_DECL_ETHTOOL_GFEATURES > -/** > - * virNetDevGFeatureAvailable > - * This function checks for the availability of a network device gfeature > - * > - * @ifname: name of the interface > - * @cmd: reference to a gfeatures ethtool command structure > - * > - * Returns 0 if not found, 1 on success, and -1 on failure. > - */ > -static int > -virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures > *cmd) -{ > - int ret = -1; > +/* static tables used by virNetDevGetFeatures() */ struct elem { > + const int cmd; > + const virNetDevFeature feat; > +}; > > - cmd = (void*)cmd; > - if (!virNetDevSendEthtoolIoctl(ifname, cmd)) > - ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); > - return ret; > -} > +/* legacy ethtool getters */ > +struct elem cmds[] = { > + {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM}, > + {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM}, > + {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG}, > + {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO}, # if > HAVE_DECL_ETHTOOL_GGSO > + {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO}, # endif # if > +HAVE_DECL_ETHTOOL_GGRO > + {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO}, # endif }; > + > +# if HAVE_DECL_ETHTOOL_GFLAGS > +/* ethtool masks */ > +struct elem flags[] = { > +# if HAVE_DECL_ETH_FLAG_LRO > + {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, # endif # if > +HAVE_DECL_ETH_FLAG_TXVLAN > + {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN}, > + {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN}, # endif # if > +HAVE_DECL_ETH_FLAG_NTUBLE > + {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE}, # endif # if > +HAVE_DECL_ETH_FLAG_RXHASH > + {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH}, # endif }; > # endif > > > @@ -3127,71 +3125,59 @@ int > virNetDevGetFeatures(const char *ifname, > virBitmapPtr *out) > { > - size_t i = -1; > - struct ethtool_value cmd = { 0 }; > + size_t i; > + int ret; > + struct ethtool_value cmd; > # if HAVE_DECL_ETHTOOL_GFEATURES > - struct ethtool_gfeatures g_cmd = { 0 }; > + union { > + struct ethtool_gfeatures *cmd; > + char *cmd_char; > + } g; > # endif > - struct elem{ > - const int cmd; > - const virNetDevFeature feat; > - }; > - /* legacy ethtool getters */ > - struct elem cmds[] = { > - {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM}, > - {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM}, > - {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG}, > - {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO}, > -# if HAVE_DECL_ETHTOOL_GGSO > - {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO}, > -# endif > -# if HAVE_DECL_ETHTOOL_GGRO > - {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO}, > -# endif > - }; > > if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST))) > return -1; > > + /* first set of capabilities are one capability per > + * command. cmd.data is set if the interface has that > + * capability > + */ > for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { > cmd.cmd = cmds[i].cmd; > - if (virNetDevFeatureAvailable(ifname, &cmd)) > + if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0) > + return -1; > + if (ret) > ignore_value(virBitmapSetBit(*out, cmds[i].feat)); > } > > # if HAVE_DECL_ETHTOOL_GFLAGS > - size_t j = -1; > - /* ethtool masks */ > - struct elem flags[] = { > -# if HAVE_DECL_ETH_FLAG_LRO > - {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, > -# endif > -# if HAVE_DECL_ETH_FLAG_TXVLAN > - {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN}, > - {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN}, > -# endif > -# if HAVE_DECL_ETH_FLAG_NTUBLE > - {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE}, > -# endif > -# if HAVE_DECL_ETH_FLAG_RXHASH > - {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH}, > -# endif > - }; > - > + /* second set of capabilities are all stored as 1 bit each in > + * cmd.data of the result of the single ETHTOOL_GFLAGS command > + */ > cmd.cmd = ETHTOOL_GFLAGS; > - if (virNetDevFeatureAvailable(ifname, &cmd)) { > - for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { > - if (cmd.data & flags[j].cmd) > - ignore_value(virBitmapSetBit(*out, flags[j].feat)); > - } > - } > + if ((ret = virNetDevFeatureAvailable(ifname, &cmd)) < 0) > + return -1; > + for (i = 0; ret && i < ARRAY_CARDINALITY(flags); i++) > + if (cmd.data & flags[i].cmd) > + ignore_value(virBitmapSetBit(*out, flags[i].feat)); > # endif > > # if HAVE_DECL_ETHTOOL_GFEATURES > - g_cmd.cmd = ETHTOOL_GFEATURES; > - g_cmd.size = GFEATURES_SIZE; > - if (virNetDevGFeatureAvailable(ifname, &g_cmd)) > + /* allocate an object with GFEATURES_SIZE elements in the features array > */ > + if (VIR_ALLOC_N(g.cmd_char, > + sizeof(struct ethtool_gfeatures) + > + (sizeof(struct ethtool_get_features_block) * GFEATURES_SIZE)) < > 0) Ok I see so basically I missed this part in ethtool code state = malloc(sizeof(*state) + FEATURE_BITS_TO_BLOCKS(defs->n_features) * sizeof(state->features.features[0])); It wired how it work for me, I did some test on redhat7 like enabling disabling The TX_UDP_TNL feature and calling nodedev-dumpxml to see it represented In the nodedev xml. Thanks for the help. > + return -1; > + > + g.cmd->cmd = ETHTOOL_GFEATURES; > + g.cmd->size = GFEATURES_SIZE; > + if (virNetDevSendEthtoolIoctl(ifname, g.cmd) < 0) { > + VIR_FREE(g.cmd); > + return -1; > + } > + if (GFEATURE_BIT_IS_SET(g.cmd->features, TX_UDP_TNL, active)) > ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL)); > + VIR_FREE(g.cmd); > # endif > > if (virNetDevRDMAFeature(ifname, out) < 0) > -- > 2.1.0 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list