netdev - Re: [net-next v3 05/15] ice: create flow profile

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: <4bd4d1e76cd74319ab54aa5ff63a1e3979c62887.camel@intel.com>
Date:   Tue, 8 Dec 2020 16:58:53 +0000
From:   "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>
To:     "Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
        "alexander.duyck@...il.com" <alexander.duyck@...il.com>
CC:     "Cao, Chinh T" <chinh.t.cao@...el.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "Behera, BrijeshX" <brijeshx.behera@...el.com>,
        "Valiquette, Real" <real.valiquette@...el.com>,
        "sassmann@...hat.com" <sassmann@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [net-next v3 05/15] ice: create flow profile

On Mon, 2020-11-23 at 17:11 -0800, Alexander Duyck wrote:
> On Mon, Nov 23, 2020 at 3:21 PM Jesse Brandeburg
> <jesse.brandeburg@...el.com> wrote:
> > 
> > Alexander Duyck wrote:
> > 
> > > > > I'm not sure this logic is correct. Can the flow director
> > > > > rules
> > > > > handle
> > > > > a field that is removed? Last I knew it couldn't. If that is
> > > > > the case
> > > > > you should be using ACL for any case in which a full mask is
> > > > > not
> > > > > provided. So in your tests below you could probably drop the
> > > > > check
> > > > > for
> > > > > zero as I don't think that is a valid case in which flow
> > > > > director
> > > > > would work.
> > > > > 
> > > > 
> > > > I'm not sure what you meant by a field that is removed, but
> > > > Flow
> > > > Director can handle reduced input sets. Flow Director is able
> > > > to handle
> > > > 0 mask, full mask, and less than 4 tuples. ACL is needed/used
> > > > only when
> > > > a partial mask rule is requested.
> > > 
> > > So historically speaking with flow director you are only allowed
> > > one
> > > mask because it determines the inputs used to generate the hash
> > > that
> > > identifies the flow. So you are only allowed one mask for all
> > > flows
> > > because changing those inputs would break the hash mapping.
> > > 
> > > Normally this ends up meaning that you have to do like what we
> > > did in
> > > ixgbe and disable ATR and only allow one mask for all inputs. I
> > > believe for i40e they required that you always use a full 4
> > > tuple. I
> > > didn't see something like that here. As such you may want to
> > > double
> > > check that you can have a mix of flow director rules that are
> > > using 1
> > > tuple, 2 tuples, 3 tuples, and 4 tuples as last I knew you
> > > couldn't.
> > > Basically if you had fields included they had to be included for
> > > all
> > > the rules on the port or device depending on how the tables are
> > > set
> > > up.
> > 
> > The ice driver hardware is quite a bit more capable than the ixgbe
> > or
> > i40e hardware, and uses a limited set of ACL rules to support
> > different
> > sets of masks. We have some limits on the number of masks and the
> > number of fields that we can simultaneously support, but I think
> > that is pretty normal for limited hardware resources.
> > 
> > Let's just say that if the code doesn't work on an E810 card then
> > we
> > messed up and we'll have to fix it. :-)
> > 
> > Thanks for the review! Hope this helps...
> 
> I gather all that. The issue was the code in ice_is_acl_filter().
> Basically if we start dropping fields it will not trigger the rule to
> be considered an ACL rule if the field is completely dropped.
> 
> So for example I could define 4 rules, one that ignores the IPv4
> source, one that ignores the IPv4 destination, one that ignores the
> TCP source port, and one that ignores the TCP destination port.

We have the limitation that you can use one input set at a time so any
of these rules could be created but they couldn't exist concurrently.

> With
> the current code all 4 of those rules would be considered to be
> non-ACL rules because the mask is 0 and not partial.

Correct. I did this to test Flow Director:

'ethtool -N ens801f0 flow-type tcp4 src-ip 192.168.0.10 dst-ip
192.168.0.20 src-port 8500 action 10' and sent traffic matching this.
Traffic correctly went to queue 10.

> If I do the same
> thing and ignore all but one bit then they are all ACL rules.

Also correct. I did as follows:

'ethtool -N ens801f0 flow-type tcp4 src-ip 192.168.0.10 dst-ip
192.168.0.20 src-port 9000 m 0x1 action 15'

Sending traffic to port 9000 and 90001, traffic went to queue 15
Sending traffic to port 8000 and 90002, traffic went to other queues

Thanks,
Tony

> In
> addition I don't see anything telling flow director it can ignore
> certain inputs over verifying the mask so I am assuming that the
> previously mentioned rules that drop entire fields would likely not
> work with Flow Director.
> 
> Anyway I just wanted to point that out as that would be an issue
> going
> forward and it seems like it would be easy to fix by simply just
> rejecting rules where the required flow director fields are not
> entirely masked in.
> 
> - Alex