netdev - Re: [PATCH net-next RFC] rtnetlink: add new RTM

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]

Message-Id: <20160224.164003.357595978348193472.davem@davemloft.net>
Date:	Wed, 24 Feb 2016 16:40:03 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	roopa@...ulusnetworks.com
Cc:	netdev@...r.kernel.org, jhs@...atatu.com
Subject: Re: [PATCH net-next RFC] rtnetlink: add new RTM_GETSTATS to dump
 link stats

From: Roopa Prabhu <roopa@...ulusnetworks.com>
Date: Mon, 22 Feb 2016 22:01:33 -0800

> From: Roopa Prabhu <roopa@...ulusnetworks.com>
> 
> This patch adds a new RTM_GETSTATS message to query link stats via netlink
> from the kernel. RTM_NEWLINK also dumps stats today, but RTM_NEWLINK returns a
> lot more than just stats and is expensive in some cases when frequent
> polling for stats from userspace is a common operation.

Even worse, we push two copies of the same exact stats.  Once to give
the (deprecated) 32-bit rtnl stats, and once to give the 64-bit ones.

This is rather rediculous, but was probably done for compatability.

> To begin with, this patch adds the following types of stats (which are
> also available via RTM_NEWLINK today):
> 
> struct nla_policy ifla_stats_policy[IFLA_LINK_STATS_MAX + 1] = {
>         [IFLA_LINK_STATS]       = { .len = sizeof(struct rtnl_link_stats) },
>         [IFLA_LINK_STATS64]     = { .len = sizeof(struct rtnl_link_stats64) },
>         [IFLA_LINK_INET6_STATS] = {. type = NLA_NESTED },
> };

I think we should skip the old 32-bit stats and only provide the full
native 64-bit rtnl_link_stats64.  Apps have to have changes to use
this new facility, therefore they can be adjusted for the (extremely
minimal) amount of work necessary to understand 64-bit stats if
necessary.

By using rtnl_fill_stats() we would unnecessarily continue to send
duplicate and extra information unnecessarily.

We have the chance to fix this here, so let's take advantage of the
opportunity to promote the data structure which can then be a first
class citizen not only in the kernel but in userspace too.

> Filtering stats (if the user wanted to query just ipv6 stats ?):
> a) I have a version of the patch that uses 'struct ifinfomsg->ifi_family'
> as a filter from userspace. But, given that in the future we may want to
> not just filter by family, I have dropped it in favor of
> attribute filtering (not included in this patch but point b) below).
> 
> b) RTM_GETSTATS message to contain attributes in the request message
> 
> 	nlmsg_parse(nlh, ....IFLA_LINK_STATS_MAX, ifla_stats_policy...)
> 
> 	if tb[IFLA_LINK_INET6_STATS]
> 		include ipv6 stats in the dump
> 
> 	if tb[IFLA_LINK_MPLS_STATS]
> 		include mpls stats in the dump

I wonder if an additive method is better.  Provide IFLA_LINK_STATS64
by default, and the user has to ask for more.

Or the user gets nothing, and a simply u32 bitmap in the request acts
as a selector, with enable bits for each stat type.

It really has to be easy to carve out exactly the information the user
wants, and be very minimal by default in order to be as efficient as
possible.