netdev - Re: [PATCH net-next 1/2] 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: <20160314190435.GA23419@nanopsycho.orion>
Date:	Mon, 14 Mar 2016 20:04:35 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	roopa <roopa@...ulusnetworks.com>
Cc:	netdev@...r.kernel.org, jhs@...atatu.com, davem@...emloft.net
Subject: Re: [PATCH net-next 1/2] rtnetlink: add new RTM_GETSTATS message to
 dump link stats

Mon, Mar 14, 2016 at 07:45:23PM CET, roopa@...ulusnetworks.com wrote:
>On 3/14/16, 7:51 AM, Jiri Pirko wrote:
>> Sun, Mar 13, 2016 at 02:56:25AM CET, roopa@...ulusnetworks.com wrote:
>>> 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.
>>>
>>> RTM_GETSTATS is an attempt to provide a light weight netlink message
>>> to explicity query only link stats from the kernel on an interface.
>>> The idea is to also keep it extensible so that new kinds of stats can be
>>> added to it in the future.
>>>
>>> This patch adds the following attribute for NETDEV stats:
>>> struct nla_policy ifla_stats_policy[IFLA_STATS_MAX + 1] = {
>>>        [IFLA_STATS_LINK64]  = { .len = sizeof(struct rtnl_link_stats64) },
>>> };
>>>
>>> This patch also allows for af family stats (an example af stats for IPV6
>>> is available with the second patch in the series).
>>>
>>> Like any other rtnetlink message, RTM_GETSTATS can be used to get stats of
>>> a single interface or all interfaces with NLM_F_DUMP.
>>>
>>> Future possible new types of stat attributes:
>>> - IFLA_MPLS_STATS  (nested. for mpls/mdev stats)
>>> - IFLA_EXTENDED_STATS (nested. extended software netdev stats like bridge,
>>>  vlan, vxlan etc)
>>> - IFLA_EXTENDED_HW_STATS (nested. extended hardware stats which are
>>>  available via ethtool today)
>>>
>>> This patch also declares a filter mask for all stat attributes.
>>> User has to provide a mask of stats attributes to query. This will be
>>> specified in a new hdr 'struct if_stats_msg' for stats messages.
>>>
>>> Without any attributes in the filter_mask, no stats will be returned.
>>>
>>> This patch has been tested with modified iproute2 ifstat.
>>>
>>> Suggested-by: Jamal Hadi Salim <jhs@...atatu.com>
>>> Signed-off-by: Roopa Prabhu <roopa@...ulusnetworks.com>
>>> ---
>>> include/net/rtnetlink.h        |   5 ++
>>> include/uapi/linux/if_link.h   |  19 ++++
>>> include/uapi/linux/rtnetlink.h |   7 ++
>>> net/core/rtnetlink.c           | 200 +++++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 231 insertions(+)
>>>
>>> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
>>> index 2f87c1b..fa68158 100644
>>> --- a/include/net/rtnetlink.h
>>> +++ b/include/net/rtnetlink.h
>>> @@ -131,6 +131,11 @@ struct rtnl_af_ops {
>>> 						    const struct nlattr *attr);
>>> 	int			(*set_link_af)(struct net_device *dev,
>>> 					       const struct nlattr *attr);
>>> +	size_t			(*get_link_af_stats_size)(const struct net_device *dev,
>>> +							  u32 filter_mask);
>>> +	int			(*fill_link_af_stats)(struct sk_buff *skb,
>>> +						      const struct net_device *dev,
>>> +						      u32 filter_mask);
>>> };
>>>
>>> void __rtnl_af_unregister(struct rtnl_af_ops *ops);
>>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>> index 249eef9..0840f3e 100644
>>> --- a/include/uapi/linux/if_link.h
>>> +++ b/include/uapi/linux/if_link.h
>>> @@ -741,4 +741,23 @@ enum {
>>>
>>> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>>>
>>> +/* STATS section */
>>> +
>>> +struct if_stats_msg {
>>> +	__u8  family;
>>> +	__u32 ifindex;
>>> +	__u32 filter_mask;
>> This limit future extension to only 32 groups of stats. I can imagine
>> that more than that can be added, easily.
>I thought about that, but it is going to be a while before we run out of the u32.
>Most of the other stats will be nested like per logical interface stats or
>per hw stats. If we do run out of them, in the future we could add a netlink
>attribute for extended filter mask to carry more bits (similar to IFLA_EXT_MASK).
>I did also start with just having a IFLA_STATS_EXT_MASK like attribute
>to begin with, but since no stats are dumped by default, having a way to easily specify
>mask in the hdr will be easier on apps. And this will again be a u32 anyways.

I believe that using *any* structs to send over netlink is a mistake.
Netlink is capable to transfer everything using attrs. Easy to compose,
easy to parse. easy to extend. Couple of more bytes in the message? So what?
For newly introduced things, I suggest to do this properly.


>
>
>>  Why don't you use nested
>> attribute IFLA_STATS_FILTER with flag attributes for every type?
>>  That
>> would be easily extendable.
>a u8 for each stats selector seems like an overkill.
>> Using netlink header struct for this does not look correct to me.
>> In past, this was done lot of times and turned out to be a problem later.
>>
>>
>I started with not adding it, but rtnetlink rcv handler looks for family
>in the hdr. And hence all of the messages have a struct header
>with family as the first field (you can correct me if you find that it is not necessary.)
>
>