| Message ID | 20210123045321.2797360-5-edwin.peer@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | support for 256 VFs in RTM_GETLINK | expand |
On Fri, 22 Jan 2021 20:53:21 -0800 Edwin Peer wrote: > Separating the VF stats out of IFLA_VF_INFO appears to be the least > impact way of resolving the nlattr overflow bug in IFLA_VFINFO_LIST. > > Since changing the hierarchy does constitute an ABI change, it must > be explicitly requested via RTEXT_FILTER_VF_SEPARATE_STATS. Otherwise, > the old location is maintained for compatibility. We don't want any additions to the VF ABI via GETLINK. IMO the clear truncation is fine, hiding stats is pushing it, new attr is a no go. > A new container type, namely IFLA_VFSTATS_LIST, is introduced to group > the stats objects into an ordered list that corresponds with the order > of VFs in IFLA_VFINFO_LIST.
On Mon, Jan 25, 2021 at 6:01 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Since changing the hierarchy does constitute an ABI change, it must > > be explicitly requested via RTEXT_FILTER_VF_SEPARATE_STATS. Otherwise, > > the old location is maintained for compatibility. > > We don't want any additions to the VF ABI via GETLINK. IMO the clear > truncation is fine, hiding stats is pushing it, new attr is a no go. How does one fix an API bug if one is not allowed to change it in any way? Perhaps that's a rhetorical question, but I had hoped we could be more pragmatic. I'm not particularly proud of this proposed patch on its standalone technical merits. There are obviously far superior solutions if we are permitted to add to the GETLINK VF API in a meaningful way. That said, it does present a compromise that cannot be construed as adding capabilities to a deprecated API. Instead of adding new operations that don't suffer the same ills, it merely moves existing structures around without changing any of the semantics of the prevailing request. By drawing such a hard line in the sand, you are declaring that this bug has no possible resolution by decree. With my vendor hat on, it's probably no skin off my teeth - all my competitors suffer the same Linux usability issue. As a long time Linux user and somebody who cares about the customer experience, it just makes me sad. :( There's a difference between encouraging people to move towards a newer API, by insisting that the development of new functionality happens there, and actively making sure the old API sucks to use. Regards, Edwin Peer
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 2bd0d8bbcdb2..db12ffd2bffd 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -341,6 +341,7 @@ enum { IFLA_ALT_IFNAME, /* Alternative ifname */ IFLA_PERM_ADDRESS, IFLA_PROTO_DOWN_REASON, + IFLA_VFSTATS_LIST, __IFLA_MAX }; diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h index b841caa4657e..f2f4f9b4d595 100644 --- a/include/uapi/linux/rtnetlink.h +++ b/include/uapi/linux/rtnetlink.h @@ -789,6 +789,7 @@ enum { #define RTEXT_FILTER_MRP (1 << 4) #define RTEXT_FILTER_CFM_CONFIG (1 << 5) #define RTEXT_FILTER_CFM_STATUS (1 << 6) +#define RTEXT_FILTER_VF_SEPARATE_STATS (1 << 7) /* End of information exported to user level */ diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 95564fd12f24..cddd3945bc11 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -933,6 +933,8 @@ static inline int rtnl_vfinfo_size(const struct net_device *dev, nla_total_size(sizeof(struct ifla_vf_rss_query_en)) + nla_total_size(sizeof(struct ifla_vf_trust))); if (~ext_filter_mask & RTEXT_FILTER_SKIP_STATS) { + if (ext_filter_mask & RTEXT_FILTER_VF_SEPARATE_STATS) + size += nla_total_size(0); /* IFLA_VFSTATS_LIST */ size += num_vfs * (nla_total_size(0) + /* nest IFLA_VF_STATS */ /* IFLA_VF_STATS_RX_PACKETS */ @@ -1368,7 +1370,8 @@ static noinline_for_stack int rtnl_fill_vfinfo(struct sk_buff *skb, goto nla_put_vf_failure; } nla_nest_end(skb, vfvlanlist); - if (~ext_filter_mask & RTEXT_FILTER_SKIP_STATS) { + if (~ext_filter_mask & RTEXT_FILTER_SKIP_STATS && + ~ext_filter_mask & RTEXT_FILTER_VF_SEPARATE_STATS) { if (rtnl_fill_vfstats(skb, dev, vfs_num)) goto nla_put_vf_failure; } @@ -1386,7 +1389,7 @@ static noinline_for_stack int rtnl_fill_vf(struct sk_buff *skb, struct net_device *dev, u32 ext_filter_mask) { - struct nlattr *vfinfo; + struct nlattr *vfinfo, *vfstats; int i, num_vfs; if (!dev->dev.parent || ((ext_filter_mask & RTEXT_FILTER_VF) == 0)) @@ -1407,8 +1410,23 @@ static noinline_for_stack int rtnl_fill_vf(struct sk_buff *skb, if (rtnl_fill_vfinfo(skb, dev, i, vfinfo, ext_filter_mask)) return -EMSGSIZE; } - nla_nest_end(skb, vfinfo); + + if (~ext_filter_mask & RTEXT_FILTER_SKIP_STATS && + ext_filter_mask & RTEXT_FILTER_VF_SEPARATE_STATS) { + vfstats = nla_nest_start_noflag(skb, IFLA_VFSTATS_LIST); + if (!vfstats) + return -EMSGSIZE; + + for (i = 0; i < num_vfs; i++) { + if (rtnl_fill_vfstats(skb, dev, i)) { + nla_nest_cancel(skb, vfstats); + return -EMSGSIZE; + } + } + nla_nest_end(skb, vfstats); + } + return 0; }
Separating the VF stats out of IFLA_VF_INFO appears to be the least impact way of resolving the nlattr overflow bug in IFLA_VFINFO_LIST. Since changing the hierarchy does constitute an ABI change, it must be explicitly requested via RTEXT_FILTER_VF_SEPARATE_STATS. Otherwise, the old location is maintained for compatibility. A new container type, namely IFLA_VFSTATS_LIST, is introduced to group the stats objects into an ordered list that corresponds with the order of VFs in IFLA_VFINFO_LIST. Fixes: 3b766cd83232 ("net/core: Add reading VF statistics through the PF netdevice") Fixes: c5a9f6f0ab40 ("net/core: Add drop counters to VF statistics") Signed-off-by: Edwin Peer <edwin.peer@broadcom.com> --- include/uapi/linux/if_link.h | 1 + include/uapi/linux/rtnetlink.h | 1 + net/core/rtnetlink.c | 24 +++++++++++++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-)