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

Medvedkin, Vladimir
2026-02-11T19:25:15Z

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