netdev - Re: [PATCH net-next v2 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: <570A59B4.30007@mojatatu.com>
Date:	Sun, 10 Apr 2016 09:48:36 -0400
From:	Jamal Hadi Salim <jhs@...atatu.com>
To:	roopa <roopa@...ulusnetworks.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net-next v2 1/2] rtnetlink: add new RTM_GETSTATS message
 to dump link stats

On 16-04-09 02:00 PM, roopa wrote:

> This EXTENDED_HW_STATS is for ethtool like extended hw stats. This is keeping in
> mind that we want to also move ethtool to netlink in the future and with switchdev
> it becomes more necessary that we provide all stats closer to the other netdev stats.
> So far hw extended stats have always been available through this separate ethtool
> channel. The intent here is to unify the api for extended hw and software only stats.

Ok, so these are _not_ the stats which are broken down by packet size
ranges which are quiet common; but rather "proprietary" per port
type h/w stats? I browsed a couple of users of ethtool_stats and i see
they return proprietary looking 64 bit counters (batman for example
has a very strange meaning to those stats).
What i meant is a lot of ASICS have counters for byte ranges
[64bytes-128bytes], [128bytes-256bytes], etc - sorry i cant pin a name
to those but i am sure you have seen them and i thought those at minimal
need their own TLV since they are always fixed.

> XSTATS is per netdev can be included as a nested attribute inside IFLA_EXTENDED_STATS
> which are per netdev. bridge vlan stats will also fall here.
>

And you are going to distinguish which come from hardware and which are
software derived?

> And this api  provides netdev specific stats. We have always mapped all
> asic stats to the switch port netdev stats. and this api does not cover the non-netdev specific stats.
> If you are for example asking for stats for a hardware offloaded bridge, then yes, they will fall here
> and be available on the bridge netdev. For asic stats that don't map to any netdev, devlink will be an
> appropriate infrastructure IMO.
>
> I am not sure if I answered your question :).
>

It is useful to have this discussion; unfortunately these user APIS once 
are in will never be allowed to change. The answers could come
in time.

>> Should such a command then not be rejected with an error code?
> Dumps with no data are not rejected with an error code AFAIK. ie they don't return
> -ENODATA. This is consistent with all other dumps (unless i missed it).
>   But, if there is a need for an error code, i can certainly check again.
>

It is mostly because you chose a whitelist filter i.e you list what
is allowed to be sent back. And if such a list is missing there
needs to be an opposite default (which is a deny all).

>>> +/* STATS section */
>>> +
>>> +struct if_stats_msg {
>>> +    __u8  family;
>>> +    __u32 ifindex;
>>> +    __u32 filter_mask;
>>> +};
>>
>> Needs to be 32 bit aligned.
>> Do you need 32 bits for the filter mask?
> yes, i think we should keep it minimum 32 bits.

Ok, that is fine then. I thought it wont exceed
3-4 per port type but i could be wrong. 32 bits
should be safer.

>> Perhaps a 16bit mask and an 8bit pad for future use.
>>
>> struct if_stats_msg {
>>             __u32 ifindex;
>>         __u16 filter_mask;
>>         __u8  family;
>>             __u8 pad; /* future use */
>> };
>>
>> Or you could reverse those (from smallest to largest).
>
> The __u8 family needs to be the first field in the structure and at the first byte in the header data.
> hence family is first and i added the others after that. It follows the format for existing such structs (for other message types).
>

True, but it must be 32 bit aligned. So something like:
struct if_stats_msg {
      __u8  family;
      __u8 pad1;
      __u16 pad2;
     __u32 ifindex;
     __u32 filter_mask;
}


> Yeah i remember :). But deferring it for a later incremental feature. That needs some more thought.

NP ;->

> Right now there is an urgent need to get the basic get stats api for a bunch of other stats: mpls, bridge vlan etc.
> Because it is not clean to extend the current stats infra part of the link message for this. So trying to get this in first.
>

Agreed.
The only thing i strongly feel about is the if_stats_msg - please fix
that and lets get at least the basics in. We can resolve other things
with further discussions.

> And this patchset only adds a handler for RTM_NEWSTATS dump and get stats. Your stats events request should be part of the  RTM_NEWSTATS handler and can include other attributes (like timeout) in the future.
>

Ok.

cheers,
jamal
> Thanks,
> Roopa
>