On Tue, 15 Sep 2026 23:24:36 +0800
Zhang Tengfei <[email protected]> wrote:
> txgbe_flow_create() programs ntuple, ethertype, SYN, FDIR, L2 tunnel and
> RSS filters into hardware before allocating the software flow object.
> If that allocation fails, create returns an error but leaves the
> hardware filter installed. The application has no handle to destroy it.
>
> Allocate the software copy first, then program the hardware. On a
> programming failure, free the copy. Set ENOMEM when allocation fails
> so the error path does not report success.
>
> L2 tunnel add failures now return immediately instead of falling
> through to RSS parsing, which cannot succeed for a VF/PF E-tag rule
> and overwrote the original error.
>
> Fixes: 5c2352b9ece6 ("net/txgbe: support creating consistent filter")
> Cc: [email protected]
>
> Signed-off-by: Zhang Tengfei <[email protected]>
> ---
Patch looks good, I was going to merge but AI had a couple of small items
that should be addressed first.
Review: [PATCH] net/txgbe: fix leak of filters on flow create
Patchwork: 169627
Applied to main (f43632a) and built with -Dwerror=true, no warnings.
The alloc-before-program reordering is the right fix. The add helpers
(ntuple, ethertype, syn, l2 tunnel) do not modify their input, so
copying the filter into the software object before programming is
equivalent to the old copy-after.
Warning
1. PF FDIR: mask_added not unwound on the new ENOMEM path.
The allocation now sits after the "A mask cannot be deleted" block.
When this rule is the first to set the mask, first_mask is TRUE and
fdir_info->mask_added has been set before rte_zmalloc() runs. On
allocation failure the code jumps to out without clearing
mask_added, unlike the program-failure path right below it.
A later rule with a different mask is then rejected with "only
support one global mask" even though no rule is using the mask.
Either move the fdir_rule_ptr allocation above the mask block
(free it on the mask error paths), or clear mask_added when
first_mask is set on allocation failure:
if (fdir_rule_ptr == NULL) {
PMD_DRV_LOG(ERR, "failed to allocate memory");
if (first_mask)
fdir_info->mask_added = FALSE;
ret = -ENOMEM;
goto out;
}
Allocating first is cleaner.
2. Missing Fixes tag for the VF FDIR path.
The txgbevf_fdir_filter_program() branch was added later by:
Fixes: 7eef71080e ("net/txgbe: switch to FDIR on VF")
Add it (12-char hash) alongside the existing tag so stable
maintainers know the VF hunk only applies to 25.11 and later.
Info
3. The commit message says ENOMEM is set "so the error path does not
report success". Other goto out paths in the same function still
reach rte_flow_error_set() with ret == 0: the flex_bytes_offset /
flex_relative mismatch, and the trailing goto out for an FDIR rule
without b_spec. The memcmp() mismatch path passes a positive ret,
so -ret is negative. Not introduced here, but either fix them in a
follow-up or narrow the wording.
4. The L2 tunnel early return is a separate behavior change (errno
reported to the caller changes from the RSS parse error to the
real add error). It is correct, but belongs in its own patch so
it can be backported or reverted independently.
5. Lines being moved anyway can drop rte_memcpy() for plain struct
assignment:
ntuple_filter_ptr->filter_info = ntuple_filter;
Same for ethertype, syn, fdir and l2 tunnel.
6. drivers/net/intel/ixgbe/ixgbe_flow.c has the same program-then-
allocate pattern in ixgbe_flow_create(). txgbe was derived from it,
the same fix applies there.
