netdev - Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT

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:   Thu, 26 Jul 2018 08:52:24 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Cc:     Paolo Abeni <pabeni@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Pirko <jiri@...nulli.us>,
        Daniel Borkmann <daniel@...earbox.net>,
        Eyal Birger <eyal.birger@...il.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

On 25/07/18 01:09 PM, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 25, 2018 at 09:48:16AM -0700, Cong Wang wrote:
>> On Wed, Jul 25, 2018 at 5:27 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>>>
>>> Those changes were there from the beginning (above patch did
>>> not introduce them).
>>> IIRC, the reason was to distinguish between policy intended
>>> drops and drops because of errors.
>>
>> There must be a limit for "overlimit" to make sense. There is
>> no limit in mirred action's context, probably there is only
>> such a limit in act_police. So, all rest should not touch overlimit.
> 
> +1
> 

I agree we should at least record drop count(unrelated patch though).
we should keep overlimit (for no other reason other than this
has been around for at least 15 years).

On why "overlimit"? It is just a name for a counter that is useless
for most actions (but was still being transfered to user space).
It is the closest counter to counting "this failed because of
runtime errors" as opposed to "user asked us to drop this".

Probably a good alternative is to make a very small stats v3 structure
(we have migrated stats structures before) and extend for
each action/classifier/qdisc to add its extra counters using XSTATS.

Note:
If you are _mirroring_ packets - incrementing the drop counter is
misleading because the packet is not dropped by the system.
i.e the qdisc will not record it as dropped; it should for
redirect policy.  It is useful to be able to tell
them apart when you are collecting  analytics just for actions.
(if youve worked on a massive amount of machines you'll appreciate
being able to debug by looking at counters that reduce ambiguity).

cheers,
jamal