From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 12610C55ABD for ; Fri, 13 Nov 2020 23:56:43 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C503E20A8B for ; Fri, 13 Nov 2020 23:56:40 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="AEDibhsk" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726249AbgKMX4T (ORCPT ); Fri, 13 Nov 2020 18:56:19 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35584 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725866AbgKMX4T (ORCPT ); Fri, 13 Nov 2020 18:56:19 -0500 Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 510BCC0613D1 for ; Fri, 13 Nov 2020 15:56:19 -0800 (PST) Received: by mail-io1-xd42.google.com with SMTP id r9so11512898ioo.7 for ; Fri, 13 Nov 2020 15:56:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uPdPaeyxEfqihggpE/tpdbGJs8hNDYsaZY5ptdNJlno=; b=AEDibhsk129Oy3vorGwaHJ/zMf1bJVGQPpINAYyx+D5Y7LAxNrHfdopeJIKucms5xY w6qBVc6PUgtbwJjyrpFOiZIbg4ZSrOtjVXt+vEjTTR3RnCgiQ6Lvx+DBFUzP38bOzgvI xJUAe0U9ZP8TZLpTPnyh8ASaUpGQdgveTHeAsLjnezQR3GOdcnCzjox0JFkPLIAw+7WY zZSORT7k+/9Bu+K3KG3p2wzdLjXDVGEk8TK8Q/Z91F54tOci/5R7gVhse6L3bM+Yfo/a DpIzRZ8lv361Zr/j0l4/6DTzWs2DQBLVz5WkLDzipBCEjMvmC6mr1Z2LHxhT6e6rk6JI ahrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uPdPaeyxEfqihggpE/tpdbGJs8hNDYsaZY5ptdNJlno=; b=r/OoCF05G3anc5R8c00F8nX/kvTRiXS1gbm9h9/02YGFCjwW1HUv0vaetbzwvSTZ1W 0bbpKMDgFzd+0hNDcKsSjauRGF0c84RnktFSYB2nGvV5GMVPSwMHmRSBHm68GgrPhrof KDNlddOUiRwf5Q7308cPEKxVdnl3yuXX4SUkavihXkBPDZv0GMMi6OHmQseHEM9TVNCH T3tmx7ymf+wTN2ls6N4Vrvgmkxq5sIdg04meTUjjRdEU21AABSDSPg5o0eTjS932aZ+7 /qCrLgGcfjYrSdpnrck5nsweovQSa04WBvZwcfda1SX41VehCELN3ZDN0HGR9bUGTv0Z Ws9Q== X-Gm-Message-State: AOAM530W4U0/BCOh2CIBGnYpT0dmsE+feLBvTIRMdJXrgdUM6GsxfF+D 7tAxU2MeaiAuDrEQSHSUWPDOIt1W8wOfDf7/BLM= X-Google-Smtp-Source: ABdhPJzKbOZ/ILSX+m8AZJ0Rn2dCjW2c7t0khRVxMeXhoH86iDaXXdTeA+2Hl2myPbbJ1/CLUewJO89yxgL60Vaglvk= X-Received: by 2002:a6b:38c6:: with SMTP id f189mr1751545ioa.38.1605311778530; Fri, 13 Nov 2020 15:56:18 -0800 (PST) MIME-Version: 1.0 References: <20201113214429.2131951-1-anthony.l.nguyen@intel.com> <20201113214429.2131951-6-anthony.l.nguyen@intel.com> In-Reply-To: <20201113214429.2131951-6-anthony.l.nguyen@intel.com> From: Alexander Duyck Date: Fri, 13 Nov 2020 15:56:07 -0800 Message-ID: Subject: Re: [net-next v3 05/15] ice: create flow profile To: Tony Nguyen Cc: David Miller , Jakub Kicinski , Real Valiquette , Netdev , Stefan Assmann , Chinh Cao , Brijesh Behera Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Nov 13, 2020 at 1:46 PM Tony Nguyen wrote: > > From: Real Valiquette > > 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 > Signed-off-by: Chinh Cao > Signed-off-by: Real Valiquette > Co-developed-by: Tony Nguyen > Signed-off-by: Tony Nguyen > Tested-by: Brijesh Behera 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. > + 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; > +} > +