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: <20200420134944.GI6581@nanopsycho.orion>
Date:   Mon, 20 Apr 2020 15:49:44 +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 02:13:51PM CEST, pablo@...filter.org wrote:
>On Mon, Apr 20, 2020 at 01:52:10PM +0200, Jiri Pirko wrote:
>> 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.
>
>I see, I think requesting HW_STATS_DISABLED in tc fails with the
>existing code though.
>
>> However so far (before HW_STATS patchset), the offload just did the
>> stats and you ignored them in netfilter code, correct?
>
>Yes, netfilter is not collecting stats yet.
>
>> Perhaps we need another value of this, like "HW_STATS_MAY_DISABLED" for
>> such case.
>
>Or just redefine FLOW_ACTION_HW_STATS_DISABLED to define a bit in
>enum flow_action_hw_stats_bit.
>
>enum flow_action_hw_stats_bit {
>        FLOW_ACTION_HW_STATES_DISABLED_BIT,
>        FLOW_ACTION_HW_STATS_IMMEDIATE_BIT,
>        FLOW_ACTION_HW_STATS_DELAYED_BIT,
>};
>
>Then update:
>
>        FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_DISABLED |
>                                   FLOW_ACTION_HW_STATS_IMMEDIATE |
>                                   FLOW_ACTION_HW_STATS_DELAYED,

No! That would break the default. "ANY" can never mean "disabled".


>
>?
>
>> 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.
>
>My interpretation is that _DISABLED means that front-end does not
>request counters to the driver.
>
>> >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).
>
>If this is solved as I'm proposing above, then
>__flow_action_hw_stats_check() need to take a bitmask instead of enum
>flow_action_hw_stats_bit as parameter, because a driver need to
>specify what they support, eg.
>
>        if (!__flow_action_hw_stats_check(action, &extack,
>                                         FLOW_ACTION_HW_STATS_DISABLED |
>                                         FLOW_ACTION_HW_STATS_DELAYED))
>                return -EOPNOSUPP;
>
>or alternatively, if the driver supports any case:
>
>        if (!__flow_action_hw_stats_check(action, &extack,
>                                         FLOW_ACTION_HW_STATS_ANY))
>                return -EOPNOSUPP;