netdev - Re: [PATCH net] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS

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: <20200420115210.GE6581@nanopsycho.orion>
Date:   Mon, 20 Apr 2020 13:52:10 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, kuba@...nel.org
Subject: Re: [PATCH net] net: flow_offload: skip hw stats check for
 FLOW_ACTION_HW_STATS_DISABLED

Mon, Apr 20, 2020 at 12:03:41PM CEST, pablo@...filter.org wrote:
>On Mon, Apr 20, 2020 at 11:13:02AM +0200, Jiri Pirko wrote:
>> Mon, Apr 20, 2020 at 11:05:05AM CEST, pablo@...filter.org wrote:
>> >On Mon, Apr 20, 2020 at 10:02:00AM +0200, Jiri Pirko wrote:
>> >> Sun, Apr 19, 2020 at 01:53:38PM CEST, pablo@...filter.org wrote:
>> >> >If the frontend requests no stats through FLOW_ACTION_HW_STATS_DISABLED,
>> >> >drivers that are checking for the hw stats configuration bail out with
>> >> >EOPNOTSUPP.
>> >>
>> >> Wait, that was a point. Driver has to support stats disabling.
>> >
>> >Hm, some drivers used to accept FLOW_ACTION_HW_STATS_DISABLED, now
>> >rulesets that used to work don't work anymore.
>>
>> How? This check is here since the introduction of hw stats types.
>
>Netfilter is setting the counter support to
>FLOW_ACTION_HW_STATS_DISABLED in this example below:
>
>  table netdev filter {
>        chain ingress {
>                type filter hook ingress device eth0 priority 0; flags offload;
>
>                tcp dport 22 drop
>        }
>  }

Hmm. In TC the HW_STATS_DISABLED has to be explicitly asked by the user,
as the sw stats are always on. Your case is different.
However so far (before HW_STATS patchset), the offload just did the
stats and you ignored them in netfilter code, correct?

Perhaps we need another value of this, like "HW_STATS_MAY_DISABLED" for
such case. Because you don't care if the HW actually does the stats or
not. It is an optimization for you.

However for TC, when user specifies "HW_STATS_DISABLED", the driver
should not do stats.


>
>The user did not specify a counter in this case.
>
>I think __flow_action_hw_stats_check() cannot work with
>FLOW_ACTION_HW_STATS_DISABLED.
>
>If check_allow_bit is false and FLOW_ACTION_HW_STATS_DISABLED is
>specified, then this always evaluates true:
>
>        if (!check_allow_bit &&
>            action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) {
>
>Similarly:
>
>        } else if (check_allow_bit &&
>                   !(action_entry->hw_stats & BIT(allow_bit))) {
>
>evaluates true for FLOW_ACTION_HW_STATS_DISABLED, assuming allow_bit is
>set, which I think it is the intention.

That is correct. __flow_action_hw_stats_check() helper is here for
simple drivers that support just one type of hw stats
(immediate/delayed).


>
>Another suggestion: This is control plane code and this
>__flow_action_hw_stats_check() function is relatively large, I'd suggest
>to move it to net/core/flow_offload.c at some point.

Sure, why not.


>
>Thank you!