Re: [PATCH] util: fix virNetDevSendEthtoolIoctl() and its callers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]