[PATCH iproute2] iplink: work around rtattr length limits for IFLA_VFINFO_LIST

* [PATCH iproute2] iplink: work around rtattr length limits for IFLA_VFINFO_LIST
@ 2021-01-15 22:59 Edwin Peer
  2021-01-15 23:53 ` Jakub Kicinski
  0 siblings, 1 reply; 13+ messages in thread
From: Edwin Peer @ 2021-01-15 22:59 UTC (permalink / raw)
  To: netdev
  Cc: Stephen Hemminger, Jakub Kicinski, Andrew Gospodarek,
	Michael Chan, Edwin Peer

The maximum possible length of an RTNL attribute is 64KB, but the
nested VFINFO list exceeds this for more than about 220 VFs (each VF
consumes approximately 300 bytes, depending on alignment and optional
fields). Exceeding the limit causes IFLA_VFINFO_LIST's length to wrap
modulo 16 bits in the kernel's nla_nest_end().

This patch is a horrible hack exploiting the fact that the full set
of attributes is actually present in the netlink packet, even though
the published length of the nested rtattr may be considerably shorter.
The total number of VFs is known, however, and can instead be used as
the basis for the iteration over the VFINFO list.

As ugly as this solution is, it does appear to be a reasonable and
practical compromise selected from a number of alternate approaches
that were considered and deemed worse or otherwise unworkable:

  - Extending the apparent maximum length of rtattr:

  To do this is a way that maintains ABI compatibility is easier said
  than done. Pushing the nested contents through deflate in response to
  a special request filter flag so that the data still fits within the
  64KB limit was considered (not entirely as crazy as this first sounds
  because there is a lot of redundancy in the data that would definitely
  compress well) as well as approaches based on providing new attribute
  types to pair with ATTR_TYPE_NESTED that extend its length in various
  ways (such as a "more" attribute or an extended attribute header with
  a wider length type). Ultimately these length extension ideas were
  rejected because the client parser APIs are expressed in terms of the
  base rtattr type, which cannot be extended cleanly without tacking on
  kludgy helpers or otherwise conducting major rework of client APIs.

  - Filtering based approaches:

  An obvious idea is to reduce the amount of data actually sent using
  filters. For example, by extending RTEXT_FILTER_SKIP_STATS to the VF
  stats, which make up a large proportion of the dump. But, the problem
  arises when it is the stats that are desired. One now either has to
  filter by VF when requesting full resolution data (ie. fetch each VF
  separately) or one has to pick another subset of fields to exclude
  and stitch the results together in the client. But, the requests are
  not atomic and the VF configuration could have changed in the interim.
  This may be less of a concern when requesting a VF's entire data as
  a whole (at least the data would necessarily apply to the same VF),
  but even so there would then need to be a mechanism to select only
  the VFINFO of interest, which is particularly messy given that we're
  not requesting a top level object here and would involve extensions
  to an otherwise frozen VF query API (and still not be atomic).

  - API redesign:

  The clean solution is to decompose the API into smaller granularity
  requests and otherwise rethink the structure of netlink attributes in
  a V2 RTM_GETLINK redesign. Such ideas are all moot, however, because
  VF config has been punted to switchdev and any new work should happen
  there instead.

Signed-off-by: Edwin Peer <edwin.peer@broadcom.com>
---
 ip/ipaddress.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 571346b15cc3..3be61f49204c 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1198,13 +1198,13 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
 	}
 
 	if ((do_link || show_details) && tb[IFLA_VFINFO_LIST] && tb[IFLA_NUM_VF]) {
-		struct rtattr *i, *vflist = tb[IFLA_VFINFO_LIST];
-		int rem = RTA_PAYLOAD(vflist);
+		struct rtattr *vf = RTA_DATA(tb[IFLA_VFINFO_LIST]);
+		int i, ignore = 0, num_vf = rta_getattr_u32(tb[IFLA_NUM_VF]);
 
 		open_json_array(PRINT_JSON, "vfinfo_list");
-		for (i = RTA_DATA(vflist); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
+		for (i = 0; i < num_vf; vf = RTA_NEXT(vf, ignore), i++) {
 			open_json_object(NULL);
-			print_vfinfo(fp, ifi, i);
+			print_vfinfo(fp, ifi, vf);
 			close_json_object();
 		}
 		close_json_array(PRINT_JSON, NULL);
@@ -2157,22 +2157,20 @@ out:
 static void
 ipaddr_loop_each_vf(struct rtattr *tb[], int vfnum, int *min, int *max)
 {
-	struct rtattr *vflist = tb[IFLA_VFINFO_LIST];
-	struct rtattr *i, *vf[IFLA_VF_MAX+1];
+	int i, ignore = 0, num_vf = rta_getattr_u32(tb[IFLA_NUM_VF]);
+	struct rtattr *vf = RTA_DATA(tb[IFLA_VFINFO_LIST]);
+	struct rtattr *vf_tb[IFLA_VF_MAX+1];
 	struct ifla_vf_rate *vf_rate;
-	int rem;
 
-	rem = RTA_PAYLOAD(vflist);
+	for (i = 0; i < num_vf; vf = RTA_NEXT(vf, ignore), i++) {
+		parse_rtattr_nested(vf_tb, IFLA_VF_MAX, vf);
 
-	for (i = RTA_DATA(vflist); RTA_OK(i, rem); i = RTA_NEXT(i, rem)) {
-		parse_rtattr_nested(vf, IFLA_VF_MAX, i);
-
-		if (!vf[IFLA_VF_RATE]) {
+		if (!vf_tb[IFLA_VF_RATE]) {
 			fprintf(stderr, "VF min/max rate API not supported\n");
 			exit(1);
 		}
 
-		vf_rate = RTA_DATA(vf[IFLA_VF_RATE]);
+		vf_rate = RTA_DATA(vf_tb[IFLA_VF_RATE]);
 		if (vf_rate->vf == vfnum) {
 			*min = vf_rate->min_tx_rate;
 			*max = vf_rate->max_tx_rate;
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread