[Intel-wired-lan] [PATCH net] ice: add profile conflict check for AVF FDIR

[Michal Swiatkowski]

On Thu, Mar 09, 2023 at 01:10:11PM +0800, Junfeng Guo wrote:
> Add profile conflict check while adding some FDIR rules to aviod
> unexpected flow behavior, rules may have conflict including:
>         IPv4 <---> {IPv4_UDP, IPv4_TCP, IPv4_SCTP}
>         IPv6 <---> {IPv6_UDP, IPv6_TCP, IPv6_SCTP}
> 
> For example, when we create an FDIR rule for IPv4, this rule will work
> on packets including IPv4, IPv4_UDP, IPv4_TCP and IPv4_SCTP. But if we
> then create an FDIR rule for IPv4_UDP and then destroy it, the first
> FDIR rule for IPv4 cannot work on pkt IPv4_UDP then.
> 
> To prevent this unexpected behavior, we add restriction in software
> when creating FDIR rules by adding necessary profile conflict check.

What about flow conflict when rule is added from host perspective (by
ethtool)? Do we also need to check for conflict? Maybe it is worth
create common code for this case.
> 
> Fixes: 1f7ea1cd6a37 ("ice: Enable FDIR Configure for AVF")
> Signed-off-by: Junfeng Guo <junfeng.guo at intel.com>
> ---
>  .../ethernet/intel/ice/ice_virtchnl_fdir.c    | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> index e6ef6b303222..1431789c194e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
> @@ -541,6 +541,71 @@ static void ice_vc_fdir_rem_prof_all(struct ice_vf *vf)
>  	}
>  }
>  
> +/**
> + * ice_vc_fdir_has_prof_conflict
> + * @vf: pointer to the VF structure
> + * @conf: FDIR configuration for each filter
> + *
> + * Check if @conf has conflicting profile with existing profiles
> + *
> + * Return: true on success, and false on error.
> + */
> +static bool
> +ice_vc_fdir_has_prof_conflict(struct ice_vf *vf,
> +			      struct virtchnl_fdir_fltr_conf *conf)
It isn't aligned.

> +{
> +	struct ice_fdir_fltr *desc;
> +
> +	list_for_each_entry(desc, &vf->fdir.fdir_rule_list, fltr_node) {
> +		struct virtchnl_fdir_fltr_conf *existing_conf =
> +						to_fltr_conf_from_desc(desc);
> +		struct ice_fdir_fltr *a = &existing_conf->input;
> +		struct ice_fdir_fltr *b = &conf->input;
> +
> +		enum ice_fltr_ptype flow_type_a = a->flow_type;
> +		enum ice_fltr_ptype flow_type_b = b->flow_type;
I think You should folow RCT variable declaration here, and remove empty
line.

> +
> +		/* No need to compare two rules with different tunnel type */
> +		if (existing_conf->ttype != conf->ttype)
> +			continue;
> +
> +		/* No need to compare two rules with same protocol */
> +		if (flow_type_a == flow_type_b)
> +			continue;
This two ifs can be combined into one.

> +
> +		switch (flow_type_a) {
> +		case ICE_FLTR_PTYPE_NONF_IPV4_UDP:
> +		case ICE_FLTR_PTYPE_NONF_IPV4_TCP:
> +		case ICE_FLTR_PTYPE_NONF_IPV4_SCTP:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_OTHER)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV4_OTHER:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_UDP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_TCP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV4_SCTP)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV6_UDP:
> +		case ICE_FLTR_PTYPE_NONF_IPV6_TCP:
> +		case ICE_FLTR_PTYPE_NONF_IPV6_SCTP:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_OTHER)
> +				return true;
> +			break;
> +		case ICE_FLTR_PTYPE_NONF_IPV6_OTHER:
> +			if (flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_UDP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_TCP ||
> +			    flow_type_b == ICE_FLTR_PTYPE_NONF_IPV6_SCTP)
> +				return true;
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  /**
>   * ice_vc_fdir_write_flow_prof
>   * @vf: pointer to the VF structure
> @@ -677,6 +742,12 @@ ice_vc_fdir_config_input_set(struct ice_vf *vf, struct virtchnl_fdir_add *fltr,
>  	enum ice_fltr_ptype flow;
>  	int ret;
>  
> +	ret = ice_vc_fdir_has_prof_conflict(vf, conf);
> +	if (ret) {
> +		dev_dbg(dev, "Found flow prof conflict for VF %d\n", vf->vf_id);
> +		return ret;
> +	}
> +
>  	flow = input->flow_type;
>  	ret = ice_vc_fdir_alloc_prof(vf, flow);
>  	if (ret) {
> -- 
> 2.25.1
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan