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

[Stephen Hemminger <stephen@networkplumber.org>]

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.