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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 2/11/2026 8:25 PM, Medvedkin, Vladimir wrote:
> 
> 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?

No, this should be correct.

However, I now suspect the original code was a bug too.

Here's what the original code said:

```
	return ixgbe_crypto_add_ingress_sa_from_flow(conf, ...);
```

The `conf` pointer points to `action->conf`, which in case of 
RTE_FLOW_ACTION_SECURITY translates to `struct rte_flow_action_security` 
structure.

However, the actual function we passed this to was doing this:

```
	struct ixgbe_crypto_session *ic_session = (void *)(uintptr_t)((const 
struct rte_security_session *)sess)->driver_priv_data;
```

where `sess` is the `conf`, but it is treated as pointer to 
`rte_security_session` not `rte_flow_action_security`. So, this is 
actually a bug too, and it kinda feels like this never worked because 
this code has been here since the very beginning...

There's also no reason why *any* of this has to work with void pointers 
when this is the only user of this function, so I'll rewrite it and 
include it in the bugfix patchset, so that this patch arrives clean.

>> +            item->spec, item->type == RTE_FLOW_ITEM_TYPE_IPV6);
> probably need to add check if item->spec is NULL

Yes, thanks for pointing that out. Will fix in bugfix patchset as well.

>> +}
>> +
>>   /* a specific function for ixgbe because the flags is specific */
>>   static int
>>   ixgbe_parse_ntuple_filter(struct rte_eth_dev *dev,
> <snip>
> 


-- 
Thanks,
Anatoly