When this function was recently changed to add in parsing of
IFLA_VF_STATS, I noticed that the checks for existence of IFLA_VF_MAC
and IFLA_VF_VLAN were looking in the *wrong array*. The array that
contains the results of parsing each IFLA_VFINFO in
tb[IFLA_VFINFO_LIST] is tb_vf, but we were checking for these in tb
(which is the array containing the results of the toplevel parsing of
the netlink message, *not* the results of parsing one of the nested
IFLA_VFINFO's.

This incorrect code has been here since the function was originally
written in 2012. It has only worked all these years due to coincidence
- the items at those indexes in tb are IFLA_ADDRESS and IFLA_BROADCAST
(of the *PF*, not of any of its VFs), and those happen to always be
present in the toplevel netlink message; since we are only looking in
the incorrect place to check for mere existence of the attribute (but
are doing the actual retrieval of the attribute from the correct
place), this bug has no real consequences other than confusing anyone
trying to understand the code.

Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/util/virnetdev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index e284d62233..591a73cb45 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1691,7 +1691,7 @@ virNetDevParseVfInfo(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
             return rc;
         }
 
-        if (mac && tb[IFLA_VF_MAC]) {
+        if (mac && tb_vf[IFLA_VF_MAC]) {
             vf_mac = RTA_DATA(tb_vf[IFLA_VF_MAC]);
             if (vf_mac && vf_mac->vf == vf)  {
                 virMacAddrSetRaw(mac, vf_mac->mac);
@@ -1699,7 +1699,7 @@ virNetDevParseVfInfo(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
             }
         }
 
-        if (vlanid && tb[IFLA_VF_VLAN]) {
+        if (vlanid && tb_vf[IFLA_VF_VLAN]) {
             vf_vlan = RTA_DATA(tb_vf[IFLA_VF_VLAN]);
             if (vf_vlan && vf_vlan->vf == vf)  {
                 *vlanid = vf_vlan->vlan;
-- 
2.26.2

On 10/21/20 4:38 AM, Laine Stump wrote:
> When this function was recently changed to add in parsing of
> IFLA_VF_STATS, I noticed that the checks for existence of IFLA_VF_MAC
> and IFLA_VF_VLAN were looking in the *wrong array*. The array that
> contains the results of parsing each IFLA_VFINFO in
> tb[IFLA_VFINFO_LIST] is tb_vf, but we were checking for these in tb
> (which is the array containing the results of the toplevel parsing of
> the netlink message, *not* the results of parsing one of the nested
> IFLA_VFINFO's.
> 
> This incorrect code has been here since the function was originally
> written in 2012. It has only worked all these years due to coincidence
> - the items at those indexes in tb are IFLA_ADDRESS and IFLA_BROADCAST
> (of the *PF*, not of any of its VFs), and those happen to always be
> present in the toplevel netlink message; since we are only looking in
> the incorrect place to check for mere existence of the attribute (but
> are doing the actual retrieval of the attribute from the correct
> place), this bug has no real consequences other than confusing anyone
> trying to understand the code.
> 
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>   src/util/virnetdev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index e284d62233..591a73cb45 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1691,7 +1691,7 @@ virNetDevParseVfInfo(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
>               return rc;
>           }
>   
> -        if (mac && tb[IFLA_VF_MAC]) {
> +        if (mac && tb_vf[IFLA_VF_MAC]) {
>               vf_mac = RTA_DATA(tb_vf[IFLA_VF_MAC]);
>               if (vf_mac && vf_mac->vf == vf)  {
>                   virMacAddrSetRaw(mac, vf_mac->mac);
> @@ -1699,7 +1699,7 @@ virNetDevParseVfInfo(struct nlattr **tb, int32_t vf, virMacAddrPtr mac,
>               }
>           }
>   
> -        if (vlanid && tb[IFLA_VF_VLAN]) {
> +        if (vlanid && tb_vf[IFLA_VF_VLAN]) {
>               vf_vlan = RTA_DATA(tb_vf[IFLA_VF_VLAN]);
>               if (vf_vlan && vf_vlan->vf == vf)  {
>                   *vlanid = vf_vlan->vlan;
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Also, feel free to bring the @tb_vf declaration into the loop (in a 
separate trivial patch).

Michal