Date:   Fri, 13 Nov 2020 15:56:07 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Tony Nguyen <anthony.l.nguyen@...el.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Real Valiquette <real.valiquette@...el.com>,
        Netdev <netdev@...r.kernel.org>,
        Stefan Assmann <sassmann@...hat.com>,
        Chinh Cao <chinh.t.cao@...el.com>,
        Brijesh Behera <brijeshx.behera@...el.com>
Subject: Re: [net-next v3 05/15] ice: create flow profile

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.

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() .

I commented on what I saw below.

Thanks.

- Alex

> ---
>  drivers/net/ethernet/intel/ice/Makefile       |   1 +
>  drivers/net/ethernet/intel/ice/ice.h          |   9 +
>  drivers/net/ethernet/intel/ice/ice_acl_main.c | 260 ++++++++++++++++
>  .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  39 +++
>  .../net/ethernet/intel/ice/ice_ethtool_fdir.c | 290 ++++++++++++++----
>  .../net/ethernet/intel/ice/ice_flex_pipe.c    |  12 +-
>  drivers/net/ethernet/intel/ice/ice_flow.c     | 178 ++++++++++-
>  drivers/net/ethernet/intel/ice/ice_flow.h     |  17 +
>  8 files changed, 727 insertions(+), 79 deletions(-)
>  create mode 100644 drivers/net/ethernet/intel/ice/ice_acl_main.c
>
> diff --git a/drivers/net/ethernet/intel/ice/Makefile b/drivers/net/ethernet/intel/ice/Makefile
> index 0747976622cf..36a787b5ad8d 100644
> --- a/drivers/net/ethernet/intel/ice/Makefile
> +++ b/drivers/net/ethernet/intel/ice/Makefile
> @@ -20,6 +20,7 @@ ice-y := ice_main.o   \
>          ice_fltr.o     \
>          ice_fdir.o     \
>          ice_ethtool_fdir.o \
> +        ice_acl_main.o \
>          ice_acl.o      \
>          ice_acl_ctrl.o \
>          ice_flex_pipe.o \
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 1008a6785e55..d813a5c765d0 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -601,16 +601,25 @@ int ice_del_ntuple_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd);
>  int ice_get_ethtool_fdir_entry(struct ice_hw *hw, struct ethtool_rxnfc *cmd);
>  u32 ice_ntuple_get_max_fltr_cnt(struct ice_hw *hw);
>  int
> +ice_ntuple_l4_proto_to_port(enum ice_flow_seg_hdr l4_proto,
> +                           enum ice_flow_field *src_port,
> +                           enum ice_flow_field *dst_port);
> +int ice_ntuple_check_ip4_seg(struct ethtool_tcpip4_spec *tcp_ip4_spec);
> +int ice_ntuple_check_ip4_usr_seg(struct ethtool_usrip4_spec *usr_ip4_spec);
> +int
>  ice_get_fdir_fltr_ids(struct ice_hw *hw, struct ethtool_rxnfc *cmd,
>                       u32 *rule_locs);
>  void ice_fdir_release_flows(struct ice_hw *hw);
>  void ice_fdir_replay_flows(struct ice_hw *hw);
>  void ice_fdir_replay_fltrs(struct ice_pf *pf);
>  int ice_fdir_create_dflt_rules(struct ice_pf *pf);
> +enum ice_fltr_ptype ice_ethtool_flow_to_fltr(int eth);
>  int ice_aq_wait_for_event(struct ice_pf *pf, u16 opcode, unsigned long timeout,
>                           struct ice_rq_event_info *event);
>  int ice_open(struct net_device *netdev);
>  int ice_stop(struct net_device *netdev);
>  void ice_service_task_schedule(struct ice_pf *pf);
> +int
> +ice_acl_add_rule_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd);
>
>  #endif /* _ICE_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_acl_main.c b/drivers/net/ethernet/intel/ice/ice_acl_main.c
> new file mode 100644
> index 000000000000..be97dfb94652
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ice/ice_acl_main.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2018-2020, Intel Corporation. */
> +
> +/* ACL support for ice */
> +
> +#include "ice.h"
> +#include "ice_lib.h"
> +
> +/* Number of action */
> +#define ICE_ACL_NUM_ACT                1
> +
> +/**
> + * ice_acl_set_ip4_addr_seg
> + * @seg: flow segment for programming
> + *
> + * Set the IPv4 source and destination address mask for the given flow segment
> + */
> +static void ice_acl_set_ip4_addr_seg(struct ice_flow_seg_info *seg)
> +{
> +       u16 val_loc, mask_loc;
> +
> +       /* IP source address */
> +       val_loc = offsetof(struct ice_fdir_fltr, ip.v4.src_ip);
> +       mask_loc = offsetof(struct ice_fdir_fltr, mask.v4.src_ip);
> +
> +       ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_IPV4_SA, val_loc,
> +                        mask_loc, ICE_FLOW_FLD_OFF_INVAL, false);
> +
> +       /* IP destination address */
> +       val_loc = offsetof(struct ice_fdir_fltr, ip.v4.dst_ip);
> +       mask_loc = offsetof(struct ice_fdir_fltr, mask.v4.dst_ip);
> +
> +       ice_flow_set_fld(seg, ICE_FLOW_FIELD_IDX_IPV4_DA, val_loc,
> +                        mask_loc, ICE_FLOW_FLD_OFF_INVAL, false);
> +}
> +
> +/**
> + * ice_acl_set_ip4_port_seg
> + * @seg: flow segment for programming
> + * @l4_proto: Layer 4 protocol to program
> + *
> + * Set the source and destination port for the given flow segment based on the
> + * provided layer 4 protocol
> + */
> +static int
> +ice_acl_set_ip4_port_seg(struct ice_flow_seg_info *seg,
> +                        enum ice_flow_seg_hdr l4_proto)
> +{
> +       enum ice_flow_field src_port, dst_port;
> +       u16 val_loc, mask_loc;
> +       int err;
> +
> +       err = ice_ntuple_l4_proto_to_port(l4_proto, &src_port, &dst_port);
> +       if (err)
> +               return err;
> +
> +       /* Layer 4 source port */
> +       val_loc = offsetof(struct ice_fdir_fltr, ip.v4.src_port);
> +       mask_loc = offsetof(struct ice_fdir_fltr, mask.v4.src_port);
> +
> +       ice_flow_set_fld(seg, src_port, val_loc, mask_loc,
> +                        ICE_FLOW_FLD_OFF_INVAL, false);
> +
> +       /* Layer 4 destination port */
> +       val_loc = offsetof(struct ice_fdir_fltr, ip.v4.dst_port);
> +       mask_loc = offsetof(struct ice_fdir_fltr, mask.v4.dst_port);
> +
> +       ice_flow_set_fld(seg, dst_port, val_loc, mask_loc,
> +                        ICE_FLOW_FLD_OFF_INVAL, false);
> +
> +       return 0;
> +}
> +
> +/**
> + * ice_acl_set_ip4_seg
> + * @seg: flow segment for programming
> + * @tcp_ip4_spec: mask data from ethtool
> + * @l4_proto: Layer 4 protocol to program
> + *
> + * Set the mask data into the flow segment to be used to program HW
> + * table based on provided L4 protocol for IPv4
> + */
> +static int
> +ice_acl_set_ip4_seg(struct ice_flow_seg_info *seg,
> +                   struct ethtool_tcpip4_spec *tcp_ip4_spec,
> +                   enum ice_flow_seg_hdr l4_proto)
> +{
> +       int err;
> +
> +       if (!seg)
> +               return -EINVAL;
> +

Unnecessary NULL pointer check. This is a static function and all
callers check the value before calling this function. You would likely
be better off just letting a NULL pointer dereference catch this.

> +       err = ice_ntuple_check_ip4_seg(tcp_ip4_spec);
> +       if (err)
> +               return err;
> +
> +       ICE_FLOW_SET_HDRS(seg, ICE_FLOW_SEG_HDR_IPV4 | l4_proto);
> +       ice_acl_set_ip4_addr_seg(seg);
> +
> +       return ice_acl_set_ip4_port_seg(seg, l4_proto);
> +}
> +
> +/**
> + * ice_acl_set_ip4_usr_seg
> + * @seg: flow segment for programming
> + * @usr_ip4_spec: ethtool userdef packet offset
> + *
> + * Set the offset data into the flow segment to be used to program HW
> + * table for IPv4
> + */
> +static int
> +ice_acl_set_ip4_usr_seg(struct ice_flow_seg_info *seg,
> +                       struct ethtool_usrip4_spec *usr_ip4_spec)
> +{
> +       int err;
> +
> +       if (!seg)
> +               return -EINVAL;
> +

and here..

> +       err = ice_ntuple_check_ip4_usr_seg(usr_ip4_spec);
> +       if (err)
> +               return err;
> +
> +       ICE_FLOW_SET_HDRS(seg, ICE_FLOW_SEG_HDR_IPV4);
> +       ice_acl_set_ip4_addr_seg(seg);
> +
> +       return 0;
> +}
> +
> +/**
> + * ice_acl_check_input_set - Checks that a given ACL input set is valid
> + * @pf: ice PF structure
> + * @fsp: pointer to ethtool Rx flow specification
> + *
> + * Returns 0 on success and negative values for failure
> + */
> +static int
> +ice_acl_check_input_set(struct ice_pf *pf, struct ethtool_rx_flow_spec *fsp)
> +{
> +       struct ice_fd_hw_prof *hw_prof = NULL;
> +       struct ice_flow_prof *prof = NULL;
> +       struct ice_flow_seg_info *old_seg;
> +       struct ice_flow_seg_info *seg;
> +       enum ice_fltr_ptype fltr_type;
> +       struct ice_hw *hw = &pf->hw;
> +       enum ice_status status;
> +       struct device *dev;
> +       int err;
> +
> +       if (!fsp)
> +               return -EINVAL;
> +

and here...

> +       dev = ice_pf_to_dev(pf);
> +       seg = devm_kzalloc(dev, sizeof(*seg), GFP_KERNEL);
> +       if (!seg)
> +               return -ENOMEM;
> +

This check of seg covers the next 4 functions. So all of the functions
you call out below don't need to check for seg being NULL as you
already did it here.

> +       switch (fsp->flow_type & ~FLOW_EXT) {
> +       case TCP_V4_FLOW:
> +               err = ice_acl_set_ip4_seg(seg, &fsp->m_u.tcp_ip4_spec,
> +                                         ICE_FLOW_SEG_HDR_TCP);
> +               break;
> +       case UDP_V4_FLOW:
> +               err = ice_acl_set_ip4_seg(seg, &fsp->m_u.tcp_ip4_spec,
> +                                         ICE_FLOW_SEG_HDR_UDP);
> +               break;
> +       case SCTP_V4_FLOW:
> +               err = ice_acl_set_ip4_seg(seg, &fsp->m_u.tcp_ip4_spec,
> +                                         ICE_FLOW_SEG_HDR_SCTP);
> +               break;
> +       case IPV4_USER_FLOW:
> +               err = ice_acl_set_ip4_usr_seg(seg, &fsp->m_u.usr_ip4_spec);
> +               break;
> +       default:
> +               err = -EOPNOTSUPP;
> +       }
> +       if (err)
> +               goto err_exit;
> +
> +       fltr_type = ice_ethtool_flow_to_fltr(fsp->flow_type & ~FLOW_EXT);
> +
> +       if (!hw->acl_prof) {
> +               hw->acl_prof = devm_kcalloc(dev, ICE_FLTR_PTYPE_MAX,
> +                                           sizeof(*hw->acl_prof), GFP_KERNEL);
> +               if (!hw->acl_prof) {
> +                       err = -ENOMEM;
> +                       goto err_exit;
> +               }
> +       }
> +       if (!hw->acl_prof[fltr_type]) {
> +               hw->acl_prof[fltr_type] = devm_kzalloc(dev,
> +                                                      sizeof(**hw->acl_prof),
> +                                                      GFP_KERNEL);
> +               if (!hw->acl_prof[fltr_type]) {
> +                       err = -ENOMEM;
> +                       goto err_acl_prof_exit;
> +               }
> +               hw->acl_prof[fltr_type]->cnt = 0;
> +       }
> +
> +       hw_prof = hw->acl_prof[fltr_type];
> +       old_seg = hw_prof->fdir_seg[0];
> +       if (old_seg) {
> +               /* This flow_type already has an input set.
> +                * If it matches the requested input set then we are
> +                * done. If it's different then it's an error.
> +                */
> +               if (!memcmp(old_seg, seg, sizeof(*seg))) {
> +                       devm_kfree(dev, seg);
> +                       return 0;
> +               }
> +
> +               err = -EINVAL;
> +               goto err_acl_prof_flow_exit;
> +       }
> +
> +       /* Adding a profile for the given flow specification with no
> +        * actions (NULL) and zero actions 0.
> +        */
> +       status = ice_flow_add_prof(hw, ICE_BLK_ACL, ICE_FLOW_RX, fltr_type,
> +                                  seg, 1, &prof);
> +       if (status) {
> +               err = ice_status_to_errno(status);
> +               goto err_exit;
> +       }
> +
> +       hw_prof->fdir_seg[0] = seg;
> +       return 0;
> +
> +err_acl_prof_flow_exit:
> +       devm_kfree(dev, hw->acl_prof[fltr_type]);
> +err_acl_prof_exit:
> +       devm_kfree(dev, hw->acl_prof);
> +err_exit:
> +       devm_kfree(dev, seg);
> +
> +       return err;
> +}
> +
> +/**
> + * ice_acl_add_rule_ethtool - Adds an ACL rule
> + * @vsi: pointer to target VSI
> + * @cmd: command to add or delete ACL rule
> + *
> + * Returns 0 on success and negative values for failure
> + */
> +int ice_acl_add_rule_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd)
> +{
> +       struct ethtool_rx_flow_spec *fsp;
> +       struct ice_pf *pf;
> +
> +       if (!vsi || !cmd)
> +               return -EINVAL;
> +

This is unneeded. The one caller of this has already referenced cmd
and vsi in multiple spots so this is a redundant check.

Actually there are so many redundant NULL pointer checks throughout I
would suggest going through and everywhere where the function starts
with one of these checks you should evaluate if you really need it or
not, rather than me going through and calling out all of them.

<snip>

> +       ret = ice_ntuple_check_ip6_usr_seg(usr_ip6_spec);
> +       if (ret)
> +               return ret;
> +
>         *perfect_fltr = true;
>         ICE_FLOW_SET_HDRS(seg, ICE_FLOW_SEG_HDR_IPV6);
>
> @@ -1489,6 +1583,64 @@ int ice_del_ntuple_ethtool(struct ice_vsi *vsi, struct ethtool_rxnfc *cmd)
>         return val;
>  }
>
> +/**
> + * 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.

> + * Returns true if ACL filter otherwise false.
> + */
> +static bool ice_is_acl_filter(struct ethtool_rx_flow_spec *fsp)
> +{
> +       struct ethtool_tcpip4_spec *tcp_ip4_spec;
> +       struct ethtool_usrip4_spec *usr_ip4_spec;
> +
> +       switch (fsp->flow_type & ~FLOW_EXT) {
> +       case TCP_V4_FLOW:
> +       case UDP_V4_FLOW:
> +       case SCTP_V4_FLOW:
> +               tcp_ip4_spec = &fsp->m_u.tcp_ip4_spec;
> +
> +               /* IP source address */
> +               if (tcp_ip4_spec->ip4src &&
> +                   tcp_ip4_spec->ip4src != htonl(0xFFFFFFFF))
> +                       return true;
> +
> +               /* IP destination address */
> +               if (tcp_ip4_spec->ip4dst &&
> +                   tcp_ip4_spec->ip4dst != htonl(0xFFFFFFFF))
> +                       return true;
> +

Instead of testing this up here you could just skip the break and fall
through since the source and destination IP addresses occupy the same
spots on usr_ip4_spec and tcp_ip4_spec. You could probably also just
use tcp_ip4_spec for the entire test.

> +               /* Layer 4 source port */
> +               if (tcp_ip4_spec->psrc && tcp_ip4_spec->psrc != htons(0xFFFF))
> +                       return true;
> +
> +               /* Layer 4 destination port */
> +               if (tcp_ip4_spec->pdst && tcp_ip4_spec->pdst != htons(0xFFFF))
> +                       return true;
> +
> +               break;
> +       case IPV4_USER_FLOW:
> +               usr_ip4_spec = &fsp->m_u.usr_ip4_spec;
> +
> +               /* IP source address */
> +               if (usr_ip4_spec->ip4src &&
> +                   usr_ip4_spec->ip4src != htonl(0xFFFFFFFF))
> +                       return true;
> +
> +               /* IP destination address */
> +               if (usr_ip4_spec->ip4dst &&
> +                   usr_ip4_spec->ip4dst != htonl(0xFFFFFFFF))
> +                       return true;
> +
> +               break;
> +       }
> +
> +       return false;
> +}
> +

Powered by blists - more mailing lists