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: <CAKgT0Uewo+Rr19EVf9br9zBPsyOUANGMSQ0kqNVAzOJ8cjWMdw@mail.gmail.com>
Date:   Fri, 20 Nov 2020 17:49:35 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     "Nguyen, Anthony L" <anthony.l.nguyen@...el.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 Fri, Nov 20, 2020 at 4:42 PM Nguyen, Anthony L
<anthony.l.nguyen@...el.com> wrote:
>
> On Fri, 2020-11-13 at 15:56 -0800, Alexander Duyck wrote:
> > On Fri, Nov 13, 2020 at 1:46 PM Tony Nguyen <
> > anthony.l.nguyen@...el.com> wrote:
> > >
> > > From: Real Valiquette <real.valiquette@...el.com>
> > >
> > > Implement the initial steps for creating an ACL filter to support
> > > ntuple
> > > masks. Create a flow profile based on a given mask rule and program
> > > it to
> > > the hardware. Though the profile is written to hardware, no actions
> > > are
> > > associated with the profile yet.
> > >
> > > Co-developed-by: Chinh Cao <chinh.t.cao@...el.com>
> > > Signed-off-by: Chinh Cao <chinh.t.cao@...el.com>
> > > Signed-off-by: Real Valiquette <real.valiquette@...el.com>
> > > Co-developed-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.com>
> > > Tested-by: Brijesh Behera <brijeshx.behera@...el.com>
> >
> > So I see two big issues with the patch.
> >
> > First it looks like there is an anti-pattern of defensive NULL
> > pointer
> > checks throughout. Those can probably all go since all of the callers
> > either use the pointer, or verify it is non-NULL before calling the
> > function in question.
>
> I'm removing those checks that you pointed out and some others as well.
>
> >
> > In addition the mask handling doens't look right to me. It is calling
> > out a partial mask as being the only time you need an ACL and I would
> > think it is any time you don't have a full mask for all
> > ports/addresses since a flow director rule normally pulls in the full
> > 4 tuple based on ice_ntuple_set_input_set() .
>
> Commented below as well.
>
> <snip>
>
> > > +/**
> > > + * ice_is_acl_filter - Checks if it's a FD or ACL filter
> > > + * @fsp: pointer to ethtool Rx flow specification
> > > + *
> > > + * If any field of the provided filter is using a partial mask
> > > then this is
> > > + * an ACL filter.
> > > + *
> >
> > 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.

Thanks.

- Alex