| Message ID | c361fce0960093e31aabbc0b45bb0c870896339e.1650615982.git.petrm@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0517a2fd66ae30bd037ddc9ac2358146e6a7bfb7 |
| Delegated to: | David Ahern |
| Headers | show |
| Series | ip stats: A new front-end for RTM_GETSTATS / RTM_SETSTATS | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Apr 22, 2022 at 10:30:55AM +0200, Petr Machata wrote: > +#define IPSTATS_RTA_PAYLOAD(TYPE, AT) \ > + ({ \ > + const struct rtattr *__at = (AT); \ > + TYPE *__ret = NULL; \ > + \ > + if (__at != NULL && \ > + __at->rta_len - RTA_LENGTH(0) >= sizeof(TYPE)) \ > + __ret = RTA_DATA(__at); \ > + __ret; \ > + }) > + > +static int ipstats_show_64(struct ipstats_stat_show_attrs *attrs, > + unsigned int group, unsigned int subgroup) > +{ > + struct rtnl_link_stats64 *stats; > + const struct rtattr *at; > + int err; > + > + at = ipstats_stat_show_get_attr(attrs, group, subgroup, &err); > + if (at == NULL) > + return err; > + > + stats = IPSTATS_RTA_PAYLOAD(struct rtnl_link_stats64, at); > + if (stats == NULL) { > + fprintf(stderr, "Error: attribute payload too short"); When I tested this on 5.15 / 5.16 everything was fine, but now I get: $ ip stats show dev lo group link 1: lo: group link Error: attribute payload too short Payload on 5.16: recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base=[{nlmsg_len=224, nlmsg_type=RTM_NEWSTATS, nlmsg_flags=0, nlmsg_seq=1651407379, nlmsg_pid=330213}, {family=AF_UNSPEC, ifindex=if_nametoindex("lo"), filter_mask=1<<IFLA_STATS_UNSPEC}, [{nla_len=196, nla_type=IFLA_STATS_LINK_64}, {rx_packets=321113, tx_packets=321113, rx_bytes=322735996, tx_bytes=322735996, rx_errors=0, tx_errors=0, rx_dropped=0, tx_dropped=0, multicast=0, collisions=0, rx_length_errors=0, rx_over_errors=0, rx_crc_errors=0, rx_frame_errors=0, rx_fifo_errors=0, rx_missed_errors=0, tx_aborted_errors=0, tx_carrier_errors=0, tx_fifo_errors=0, tx_heartbeat_errors=0, tx_window_errors=0, rx_compressed=0, tx_compressed=0, rx_nohandler=0}]], iov_len=32768}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 224 1: lo: group link Error: attribute payload too short+++ exited with 22 +++ Payload on net-next: recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base=[{nlmsg_len=232, nlmsg_type=RTM_NEWSTATS, nlmsg_flags=0, nlmsg_seq=1651407411, nlmsg_pid=198}, {family=AF_UNSPEC, ifindex=if_nametoindex("lo"), filter_mask=1<<IFLA_STATS_UNSPEC}, [{nla_len=204, nla_type=IFLA_STATS_LINK_64}, {rx_packets=0, tx_packets=0, rx_bytes=0, tx_bytes=0, rx_errors=0, tx_errors=0, rx_dropped=0, tx_dropped=0, multicast=0, collisions=0, rx_length_errors=0, rx_over_errors=0, rx_crc_errors=0, rx_frame_errors=0, rx_fifo_errors=0, rx_missed_errors=0, tx_aborted_errors=0, tx_carrier_errors=0, tx_fifo_errors=0, tx_heartbeat_errors=0, tx_window_errors=0, rx_compressed=0, tx_compressed=0, rx_nohandler=0}]], iov_len=32768}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 232 1: lo: group link RX: bytes packets errors dropped missed mcast 0 0 0 0 0 0 TX: bytes packets errors dropped carrier collsns 0 0 0 0 0 0 +++ exited with 0 +++ Note the difference in size of IFLA_STATS_LINK_64 which carries struct rtnl_link_stats64: 196 bytes vs. 204 bytes The 8 byte difference is most likely from the addition of rx_otherhost_dropped at the end: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=794c24e9921f32ded4422833a990ccf11dc3c00e I guess it worked for me because I didn't have this member in my copy of the uAPI file, but it's now in iproute2-next: https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=bba95837524d09ee2f0efdf6350b83a985f4b2f8 I was under the impression that such a size increase in a uAPI struct is forbidden, which is why we usually avoid passing structs over netlink. > + return -EINVAL; > + } > + > + open_json_object("stats64"); > + print_stats64(stdout, stats, NULL, NULL); > + close_json_object(); > + return 0; > +}
On 5/1/22 8:52 AM, Ido Schimmel wrote: > When I tested this on 5.15 / 5.16 everything was fine, but now I get: > > $ ip stats show dev lo group link > 1: lo: group link > Error: attribute payload too short > ... > > Note the difference in size of IFLA_STATS_LINK_64 which carries struct > rtnl_link_stats64: 196 bytes vs. 204 bytes > > The 8 byte difference is most likely from the addition of > rx_otherhost_dropped at the end: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=794c24e9921f32ded4422833a990ccf11dc3c00e > > I guess it worked for me because I didn't have this member in my copy of > the uAPI file, but it's now in iproute2-next: > > https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=bba95837524d09ee2f0efdf6350b83a985f4b2f8 > > I was under the impression that such a size increase in a uAPI struct is > forbidden, which is why we usually avoid passing structs over netlink. extending structs at the end is typically allowed. The ABI breakage is when an entry is added in the middle. > >> + return -EINVAL; >> + } >> + >> + open_json_object("stats64"); >> + print_stats64(stdout, stats, NULL, NULL); >> + close_json_object(); >> + return 0; >> +}
On Sun, May 01, 2022 at 09:56:16PM -0700, David Ahern wrote: > On 5/1/22 8:52 AM, Ido Schimmel wrote: > > When I tested this on 5.15 / 5.16 everything was fine, but now I get: > > > > $ ip stats show dev lo group link > > 1: lo: group link > > Error: attribute payload too short > > > > ... > > > > > Note the difference in size of IFLA_STATS_LINK_64 which carries struct > > rtnl_link_stats64: 196 bytes vs. 204 bytes > > > > The 8 byte difference is most likely from the addition of > > rx_otherhost_dropped at the end: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=794c24e9921f32ded4422833a990ccf11dc3c00e > > > > I guess it worked for me because I didn't have this member in my copy of > > the uAPI file, but it's now in iproute2-next: > > > > https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=bba95837524d09ee2f0efdf6350b83a985f4b2f8 > > > > I was under the impression that such a size increase in a uAPI struct is > > forbidden, which is why we usually avoid passing structs over netlink. > > extending structs at the end is typically allowed. In this case, the iproute2 check needs to be modified as new iproute2 should work with old kernels. > The ABI breakage is when an entry is added in the middle. Sure, that's well understood. Thanks! > > > > > >> + return -EINVAL; > >> + } > >> + > >> + open_json_object("stats64"); > >> + print_stats64(stdout, stats, NULL, NULL); > >> + close_json_object(); > >> + return 0; > >> +} >
Ido Schimmel <idosch@idosch.org> writes: > When I tested this on 5.15 / 5.16 everything was fine, but now I get: > > $ ip stats show dev lo group link > 1: lo: group link > Error: attribute payload too short > > Payload on 5.16: > > recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base=[{nlmsg_len=224, nlmsg_type=RTM_NEWSTATS, nlmsg_flags=0, nlmsg_seq=1651407379, nlmsg_pid=330213}, {family=AF_UNSPEC, ifindex=if_nametoindex("lo"), filter_mask=1<<IFLA_STATS_UNSPEC}, [{nla_len=196, nla_type=IFLA_STATS_LINK_64}, {rx_packets=321113, tx_packets=321113, rx_bytes=322735996, tx_bytes=322735996, rx_errors=0, tx_errors=0, rx_dropped=0, tx_dropped=0, multicast=0, collisions=0, rx_length_errors=0, rx_over_errors=0, rx_crc_errors=0, rx_frame_errors=0, rx_fifo_errors=0, rx_missed_errors=0, tx_aborted_errors=0, tx_carrier_errors=0, tx_fifo_errors=0, tx_heartbeat_errors=0, tx_window_errors=0, rx_compressed=0, tx_compressed=0, rx_nohandler=0}]], iov_len=32768}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 224 > 1: lo: group link > Error: attribute payload too short+++ exited with 22 +++ > > Payload on net-next: > > recvmsg(3, {msg_name={sa_family=AF_NETLINK, nl_pid=0, nl_groups=00000000}, msg_namelen=12, msg_iov=[{iov_base=[{nlmsg_len=232, nlmsg_type=RTM_NEWSTATS, nlmsg_flags=0, nlmsg_seq=1651407411, nlmsg_pid=198}, {family=AF_UNSPEC, ifindex=if_nametoindex("lo"), filter_mask=1<<IFLA_STATS_UNSPEC}, [{nla_len=204, nla_type=IFLA_STATS_LINK_64}, {rx_packets=0, tx_packets=0, rx_bytes=0, tx_bytes=0, rx_errors=0, tx_errors=0, rx_dropped=0, tx_dropped=0, multicast=0, collisions=0, rx_length_errors=0, rx_over_errors=0, rx_crc_errors=0, rx_frame_errors=0, rx_fifo_errors=0, rx_missed_errors=0, tx_aborted_errors=0, tx_carrier_errors=0, tx_fifo_errors=0, tx_heartbeat_errors=0, tx_window_errors=0, rx_compressed=0, tx_compressed=0, rx_nohandler=0}]], iov_len=32768}], msg_iovlen=1, msg_controllen=0, msg_flags=0}, 0) = 232 > 1: lo: group link > RX: bytes packets errors dropped missed mcast > 0 0 0 0 0 0 > TX: bytes packets errors dropped carrier collsns > 0 0 0 0 0 0 > +++ exited with 0 +++ > > Note the difference in size of IFLA_STATS_LINK_64 which carries struct > rtnl_link_stats64: 196 bytes vs. 204 bytes > > The 8 byte difference is most likely from the addition of > rx_otherhost_dropped at the end: > > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=794c24e9921f32ded4422833a990ccf11dc3c00e > > I guess it worked for me because I didn't have this member in my copy of > the uAPI file, but it's now in iproute2-next: > > https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=bba95837524d09ee2f0efdf6350b83a985f4b2f8 Thanks, I'll send a fix.
diff --git a/ip/ipstats.c b/ip/ipstats.c index 79f7b1ff..e4f97ddd 100644 --- a/ip/ipstats.c +++ b/ip/ipstats.c @@ -12,6 +12,15 @@ struct ipstats_stat_dump_filters { __u32 mask[IFLA_STATS_MAX + 1]; }; +static void +ipstats_stat_desc_enable_bit(struct ipstats_stat_dump_filters *filters, + unsigned int group, unsigned int subgroup) +{ + filters->mask[0] |= IFLA_STATS_FILTER_BIT(group); + if (subgroup) + filters->mask[group] |= IFLA_STATS_FILTER_BIT(subgroup); +} + struct ipstats_stat_show_attrs { struct if_stats_msg *ifsm; int len; @@ -89,6 +98,29 @@ ipstats_stat_show_attrs_alloc_tb(struct ipstats_stat_show_attrs *attrs, return err; } +static const struct rtattr * +ipstats_stat_show_get_attr(struct ipstats_stat_show_attrs *attrs, + int group, int subgroup, int *err) +{ + int tmp_err; + + if (err == NULL) + err = &tmp_err; + + *err = 0; + if (subgroup == 0) + return attrs->tbs[0][group]; + + if (attrs->tbs[0][group] == NULL) + return NULL; + + *err = ipstats_stat_show_attrs_alloc_tb(attrs, group); + if (*err != 0) + return NULL; + + return attrs->tbs[group][subgroup]; +} + static void ipstats_stat_show_attrs_free(struct ipstats_stat_show_attrs *attrs) { @@ -98,7 +130,65 @@ ipstats_stat_show_attrs_free(struct ipstats_stat_show_attrs *attrs) free(attrs->tbs[i]); } +#define IPSTATS_RTA_PAYLOAD(TYPE, AT) \ + ({ \ + const struct rtattr *__at = (AT); \ + TYPE *__ret = NULL; \ + \ + if (__at != NULL && \ + __at->rta_len - RTA_LENGTH(0) >= sizeof(TYPE)) \ + __ret = RTA_DATA(__at); \ + __ret; \ + }) + +static int ipstats_show_64(struct ipstats_stat_show_attrs *attrs, + unsigned int group, unsigned int subgroup) +{ + struct rtnl_link_stats64 *stats; + const struct rtattr *at; + int err; + + at = ipstats_stat_show_get_attr(attrs, group, subgroup, &err); + if (at == NULL) + return err; + + stats = IPSTATS_RTA_PAYLOAD(struct rtnl_link_stats64, at); + if (stats == NULL) { + fprintf(stderr, "Error: attribute payload too short"); + return -EINVAL; + } + + open_json_object("stats64"); + print_stats64(stdout, stats, NULL, NULL); + close_json_object(); + return 0; +} + +static void +ipstats_stat_desc_pack_link(struct ipstats_stat_dump_filters *filters, + const struct ipstats_stat_desc *desc) +{ + ipstats_stat_desc_enable_bit(filters, + IFLA_STATS_LINK_64, 0); +} + +static int +ipstats_stat_desc_show_link(struct ipstats_stat_show_attrs *attrs, + const struct ipstats_stat_desc *desc) +{ + print_nl(); + return ipstats_show_64(attrs, IFLA_STATS_LINK_64, 0); +} + +static const struct ipstats_stat_desc ipstats_stat_desc_toplev_link = { + .name = "link", + .kind = IPSTATS_STAT_DESC_KIND_LEAF, + .pack = &ipstats_stat_desc_pack_link, + .show = &ipstats_stat_desc_show_link, +}; + static const struct ipstats_stat_desc *ipstats_stat_desc_toplev_subs[] = { + &ipstats_stat_desc_toplev_link, }; static const struct ipstats_stat_desc ipstats_stat_desc_toplev_group = {
Add the "link" top-level group for showing IFLA_STATS_LINK_64 statistics. For example: # ip stats show dev swp1 group link 4178: swp1: group link RX: bytes packets errors dropped missed mcast 47048 354 0 0 0 64 TX: bytes packets errors dropped carrier collsns 47474 355 0 0 0 0 # ip -j stats show dev swp1 group link | jq [ { "ifindex": 4178, "ifname": "swp1", "group": "link", "stats64": { "rx": { "bytes": 47048, "packets": 354, "errors": 0, "dropped": 0, "over_errors": 0, "multicast": 64 }, "tx": { "bytes": 47474, "packets": 355, "errors": 0, "dropped": 0, "carrier_errors": 0, "collisions": 0 } } } ] Signed-off-by: Petr Machata <petrm@nvidia.com> Reviewed-by: Ido Schimmel <idosch@nvidia.com> --- ip/ipstats.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)