Re: [PATCH v3 03/27] net/ixgbe: split security and ntuple filters

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

On 2/11/2026 1:52 PM, Anatoly Burakov wrote:
> These filters are mashed together even though they almost do not share any
> code at all between each other. Separate security filter from ntuple filter
> and parse it separately.
>
> While we're at it, we're making checks more stringent (such as checking for
> NULL conf), and more type safe.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>   drivers/net/intel/ixgbe/ixgbe_flow.c | 136 ++++++++++++++++++---------
>   1 file changed, 91 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/net/intel/ixgbe/ixgbe_flow.c b/drivers/net/intel/ixgbe/ixgbe_flow.c
> index c17c6c4bf6..cd8d46019f 100644
> --- a/drivers/net/intel/ixgbe/ixgbe_flow.c
> +++ b/drivers/net/intel/ixgbe/ixgbe_flow.c
> @@ -214,41 +214,6 @@ cons_parse_ntuple_filter(const struct rte_flow_attr *attr,
>   	memset(&eth_null, 0, sizeof(struct rte_flow_item_eth));
>   	memset(&vlan_null, 0, sizeof(struct rte_flow_item_vlan));
>   
> -	/**
> -	 *  Special case for flow action type RTE_FLOW_ACTION_TYPE_SECURITY
> -	 */
> -	act = next_no_void_action(actions, NULL);
> -	if (act->type == RTE_FLOW_ACTION_TYPE_SECURITY) {
> -		const void *conf = act->conf;
> -		/* check if the next not void item is END */
> -		act = next_no_void_action(actions, act);
> -		if (act->type != RTE_FLOW_ACTION_TYPE_END) {
> -			memset(filter, 0, sizeof(struct rte_eth_ntuple_filter));
> -			rte_flow_error_set(error, EINVAL,
> -				RTE_FLOW_ERROR_TYPE_ACTION,
> -				act, "Not supported action.");
> -			return -rte_errno;
> -		}
> -
> -		/* get the IP pattern*/
> -		item = next_no_void_pattern(pattern, NULL);
> -		while (item->type != RTE_FLOW_ITEM_TYPE_IPV4 &&
> -				item->type != RTE_FLOW_ITEM_TYPE_IPV6) {
> -			if (item->last ||
> -					item->type == RTE_FLOW_ITEM_TYPE_END) {
> -				rte_flow_error_set(error, EINVAL,
> -					RTE_FLOW_ERROR_TYPE_ITEM,
> -					item, "IP pattern missing.");
> -				return -rte_errno;
> -			}
> -			item = next_no_void_pattern(pattern, item);
> -		}
> -
> -		filter->proto = IPPROTO_ESP;
> -		return ixgbe_crypto_add_ingress_sa_from_flow(conf, item->spec,
> -					item->type == RTE_FLOW_ITEM_TYPE_IPV6);
> -	}
> -
>   	/* the first not void item can be MAC or IPv4 */
>   	item = next_no_void_pattern(pattern, NULL);
>   
> @@ -607,6 +572,81 @@ cons_parse_ntuple_filter(const struct rte_flow_attr *attr,
>   	return 0;
>   }
>   
> +static int
> +ixgbe_parse_security_filter(struct rte_eth_dev *dev, const struct rte_flow_attr *attr,
> +		const struct rte_flow_item pattern[], const struct rte_flow_action actions[],
> +		struct rte_flow_error *error)
> +{
> +	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	const struct rte_flow_action_security *security;
> +	const struct rte_flow_item *item;
> +	const struct rte_flow_action *act;
> +
> +	if (hw->mac.type != ixgbe_mac_82599EB &&
> +			hw->mac.type != ixgbe_mac_X540 &&
> +			hw->mac.type != ixgbe_mac_X550 &&
> +			hw->mac.type != ixgbe_mac_X550EM_x &&
> +			hw->mac.type != ixgbe_mac_X550EM_a &&
> +			hw->mac.type != ixgbe_mac_E610)
> +		return -ENOTSUP;
> +
> +	if (pattern == NULL) {
> +		rte_flow_error_set(error,
> +			EINVAL, RTE_FLOW_ERROR_TYPE_ITEM_NUM,
> +			NULL, "NULL pattern.");
> +		return -rte_errno;
> +	}
> +	if (actions == NULL) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ACTION_NUM,
> +				   NULL, "NULL action.");
> +		return -rte_errno;
> +	}
> +	if (attr == NULL) {
> +		rte_flow_error_set(error, EINVAL,
> +				   RTE_FLOW_ERROR_TYPE_ATTR,
> +				   NULL, "NULL attribute.");
> +		return -rte_errno;
> +	}
> +
> +	/* check if next non-void action is security */
> +	act = next_no_void_action(actions, NULL);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_SECURITY) {
> +		return rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION,
> +				act, "Not supported action.");
> +	}
> +	security = act->conf;
> +	if (security == NULL) {
this looks like a bug fix. In a previous implementation it didn't check 
if act->conf is NULL and  consequent calling for 
ixgbe_crypto_add_ingress_sa_from_flow() immediately dereference it. 
Probably it would be great to backport this as a fix.
> +		return rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION, act,
> +				"NULL security action config.");
> +	}
> +	/* check if the next not void item is END */
> +	act = next_no_void_action(actions, act);
> +	if (act->type != RTE_FLOW_ACTION_TYPE_END) {
> +		return rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ACTION,
> +				act, "Not supported action.");
> +	}
> +
> +	/* get the IP pattern*/
> +	item = next_no_void_pattern(pattern, NULL);
> +	while (item->type != RTE_FLOW_ITEM_TYPE_IPV4 &&
> +			item->type != RTE_FLOW_ITEM_TYPE_IPV6) {
> +		if (item->last || item->type == RTE_FLOW_ITEM_TYPE_END) {
> +			rte_flow_error_set(error, EINVAL,
> +				RTE_FLOW_ERROR_TYPE_ITEM,
> +				item, "IP pattern missing.");
> +			return -rte_errno;
> +		}
> +		item = next_no_void_pattern(pattern, item);
> +	}
> +
> +	return ixgbe_crypto_add_ingress_sa_from_flow(security->security_session,
did you mean &security->security_session here?
> +			item->spec, item->type == RTE_FLOW_ITEM_TYPE_IPV6);
probably need to add check if item->spec is NULL
> +}
> +
>   /* a specific function for ixgbe because the flags is specific */
>   static int
>   ixgbe_parse_ntuple_filter(struct rte_eth_dev *dev,
<snip>

-- 
Regards,
Vladimir