netdev - Re: [PATCH iproute2-next 06/11] ipstats: Add a group "link"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 1 May 2022 17:52:03 +0300
From:   Ido Schimmel <idosch@...sch.org>
To:     Petr Machata <petrm@...dia.com>
Cc:     netdev@...r.kernel.org, David Ahern <dsahern@...il.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        Ido Schimmel <idosch@...dia.com>
Subject: Re: [PATCH iproute2-next 06/11] ipstats: Add a group "link"

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;
> +}