netdev - Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of filter and actions

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: <66f350c5-1fd7-6132-3791-390454c97256@mojatatu.com>
Date:   Wed, 3 Nov 2021 08:33:53 -0400
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Baowen Zheng <baowen.zheng@...igine.com>,
        Simon Horman <simon.horman@...igine.com>,
        Vlad Buslov <vladbu@...dia.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Roi Dayan <roid@...dia.com>, Ido Schimmel <idosch@...dia.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Baowen Zheng <notifications@...hub.com>,
        Louis Peens <louis.peens@...igine.com>,
        oss-drivers <oss-drivers@...igine.com>,
        Oz Shlomo <ozsh@...dia.com>
Subject: Re: [RFC/PATCH net-next v3 8/8] flow_offload: validate flags of
 filter and actions

On 2021-11-03 07:30, Baowen Zheng wrote:
> On November 3, 2021 6:14 PM, Jamal Hadi Salim wrote:
>> On 2021-11-03 03:57, Baowen Zheng wrote:
>>> On November 2, 2021 8:40 PM, Simon Horman wrote:
>>>> On Mon, Nov 01, 2021 at 09:38:34AM +0200, Vlad Buslov wrote:
>>>>> On Mon 01 Nov 2021 at 05:29, Baowen Zheng
>>
>> [..]
>>>>>
>>>>> My suggestion was to forgo the skip_sw flag for shared action
>>>>> offload and, consecutively, remove the validation code, not to add
>>>>> even more checks. I still don't see a practical case where skip_sw
>>>>> shared action is useful. But I don't have any strong feelings about
>>>>> this flag, so if Jamal thinks it is necessary, then fine by me.
>>>>
>>>> FWIIW, my feelings are the same as Vlad's.
>>>>
>>>> I think these flags add complexity that would be nice to avoid.
>>>> But if Jamal thinks its necessary, then including the flags
>>>> implementation is fine by me.
>>> Thanks Simon. Jamal, do you think it is necessary to keep the skip_sw
>>> flag for user to specify the action should not run in software?
>>>
>>
>> Just catching up with discussion...
>> IMO, we need the flag. Oz indicated with requirement to be able to identify
>> the action with an index. So if a specific action is added for skip_sw (as
>> standalone or alongside a filter) then it cant be used for skip_hw. To illustrate
>> using extended example:
>>
>> #filter 1, skip_sw
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_sw ip_proto tcp action police blah index 10
>>
>> #filter 2, skip_hw
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_hw ip_proto udp action police index 10
>>
>> Filter2 should be illegal.
>> And when i dump the actions as so:
>> tc actions ls action police
>>
>> For debugability, I should see index 10 clearly marked with the flag as skip_sw
>>
>> The other example i gave earlier which showed the sharing of actions:
>>
>> #add a policer action and offload it
>> tc actions add action police skip_sw rate ... index 20 #now add filter1 which is
>> offloaded using offloaded policer tc filter add dev $DEV1 proto ip parent ffff:
>> flower \
>>      skip_sw ip_proto tcp action police index 20 #add filter2 likewise offloaded
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_sw ip_proto udp action police index 20
>>
>> All good and filter 1 and 2 are sharing policer instance with index 20.
>>
>> #Now add a filter3 which is s/w only
>> tc filter add dev $DEV1 proto ip parent ffff: flower \
>>      skip_hw ip_proto icmp action police index 20
>>
>> filter3 should not be allowed.
> I think the use cases you mentioned above are clear for us. For the case:
> 
> #add a policer action and offload it
> tc actions add action police skip_sw rate ... index 20
> #Now add a filter4 which has no flag
> tc filter add dev $DEV1 proto ip parent ffff: flower \
>       ip_proto icmp action police index 20
> 
> Is filter4 legal? 

Yes it is _based on current semantics_.
The reason is when adding a filter and specifying neither
skip_sw nor skip_hw it defaults to allowing both.
i.e is the same as skip_sw|skip_hw. You will need to have
counters for both s/w and h/w (which i think is taken care of today).


>basically, it should be legal, but since filter4 may be offloaded failed so
> it will run in software, you know the action police should not run in software with skip_sw,
> so I think filter4 should be illegal and we should not allow this case.
> That is if the action is skip_sw, then the filter refers to this action should also skip_sw.
> WDYT?

See above..

cheers,
jamal