Re: [PATCH] net/txgbe: fix leak of filters on flow create

[Zhang Tengfei <zhtfdev@gmail.com>]

Thanks for the review.
Item 6: the ixgbe counterpart is already on the list:
   [PATCH] net/ixgbe: fix leak of filters on flow create
https://patches.dpdk.org/project/dpdk/patch/20260914133836.14644-1-zhtfdev@gmail.com/

I will send a v2 for txgbe.

On 9/15/26 23:38, Stephen Hemminger wrote:
> On Tue, 15 Sep 2026 23:24:36 +0800
> Zhang Tengfei <zhtfdev@gmail.com> 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: stable@dpdk.org
>>
>> Signed-off-by: Zhang Tengfei <zhtfdev@gmail.com>
>> ---
> 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.