Re: [net-next v2 03/15] ice: initialize ACL table

From: Alexander Duyck <alexander.duyck@gmail.com>
To: "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>
Cc: "Cao, Chinh T" <chinh.t.cao@intel.com>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"Valiquette, Real" <real.valiquette@intel.com>,
	"davem@davemloft.neti" <davem@davemloft.neti>,
	"Bokkena, HarikumarX" <harikumarx.bokkena@intel.com>,
	"sassmann@redhat.com" <sassmann@redhat.com>
Subject: Re: [net-next v2 03/15] ice: initialize ACL table
Date: Fri, 13 Nov 2020 17:50:53 -0800	[thread overview]
Message-ID: <CAKgT0UdiZGGjSaF=8gx4ZcApYzWDe_FpQwSZmtUjC2ZBsOeXDg@mail.gmail.com> (raw)
In-Reply-To: <2c35ef3950756f0ea04fb61246b2c9b22859d3d4.camel@intel.com>

On Fri, Nov 13, 2020 at 4:16 PM Nguyen, Anthony L
<anthony.l.nguyen@intel.com> wrote:
>
> On Fri, 2020-11-13 at 14:59 -0800, Alexander Duyck wrote:
> > On Fri, Nov 13, 2020 at 1:36 PM Tony Nguyen <
> > anthony.l.nguyen@intel.com> wrote:
> > >
> > > From: Real Valiquette <real.valiquette@intel.com>
> > >
> > > ACL filtering can be utilized to expand support of ntuple rules by
> > > allowing
> > > mask values to be specified for redirect to queue or drop.
> > >
> > > Implement support for specifying the 'm' value of ethtool ntuple
> > > command
> > > for currently supported fields (src-ip, dst-ip, src-port, and dst-
> > > port).
> > >
> > > For example:
> > >
> > > ethtool -N eth0 flow-type tcp4 dst-port 8880 m 0x00ff action 10
> > > or
> > > ethtool -N eth0 flow-type tcp4 src-ip 192.168.0.55 m 0.0.0.255
> > > action -1
> > >
> > > At this time the following flow-types support mask values: tcp4,
> > > udp4,
> > > sctp4, and ip4.
> >
> > So you spend all of the patch description describing how this might
> > be
> > used in the future. However there is nothing specific to the ethtool
> > interface as far as I can tell anywhere in this patch. With this
> > patch
> > the actual command called out above cannot be performed, correct?
> >
> > > Begin implementation of ACL filters by setting up structures,
> > > AdminQ
> > > commands, and allocation of the ACL table in the hardware.
> >
> > This seems to be what this patch is actually doing. You may want to
> > rewrite this patch description to focus on this and explain that you
> > are enabling future support for ethtool ntuple masks. However save
> > this feature description for the patch that actually enables the
> > functionality.
>
> Thanks for the feedback Alex. I believe you're still reviewing the
> patches, I'l look through and make changes accordingly or get responses
> as neeeded.
>
> Thanks,
> Tony

I've pretty much finished up now. My main concern with the set is the
mask handling for the ACL functionality. I think it may be doing the
wrong thing since last I knew Flow Director required the full 4 tuple
to function for TCP/UDP, so there are probably cases that are being
sent to Flow Director when it should be handled by ACL, specifically
when one of the ports is masked out of the filtering entirely.

- Alex