[libvirt] [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers
Moshe Levi
moshele at mellanox.com
Tue Aug 11 07:55:36 UTC 2015
> -----Original Message-----
> From: Laine Stump [mailto:laine at laine.org]
> Sent: Tuesday, August 11, 2015 7:10 AM
> To: libvir-list at redhat.com
> Cc: Moshe Levi; Brian Rak; james.p.chapman at intel.com
> 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
More information about the libvir-list
mailing list