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]

Date:   Wed, 3 Nov 2021 14:38:17 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Baowen Zheng <baowen.zheng@...igine.com>,
        Vlad Buslov <vladbu@...dia.com>,
        "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 Wed, Nov 03, 2021 at 09:33:52AM -0400, Jamal Hadi Salim wrote:
> On 2021-11-03 08:33, Jamal Hadi Salim wrote:
> > 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).
> > 
> > 
> 
> Apologies, i will like to take this one back. Couldnt stop thinking
> about it while sipping coffee;->
> To be safe that should be illegal. The flags have to match _exactly_
> for both  action and filter to make any sense. i.e in the above case
> they are not.

I could be wrong, but I would have thought that in this case the flow
is legal but is only added to hw (because the action doesn't exist in sw).

But if you prefer to make it illegal I guess that is ok too.