Re: [PATCH net] tipc: fix memory leaks in bundle and fragment paths

[xiaoshoukui@gmail.com]

Hi Tung,

Thanks for bringing this performance concern to our attention and
sharing your benchmark results.

> This patch tries to address an unreal use case. I am waiting for
> your real reproducer and explanation on how data part of bundling
> and fragmented messages can contain invalid types/users.
> Of course, tipc_data_input() drops invalid types/users as designed.
> The check is redundant because tipc_data_input() never returns false
> after calling tipc_msg_extract() or tipc_buf_append().
> This is not correct. If tipc_data_input() returns false, the skb
> will be dropped.
> tipc_data_input() releases invalid skbs as expected.

Your observation regarding truly illegal message users is correct. In
the default case, tipc_data_input() drops the skb with kfree_skb() and
returns true. However, this issue concerns the recognized internal
control users. For these users, tipc_data_input() returns false without
freeing the skb:

static bool tipc_data_input(struct tipc_link *l, struct sk_buff *skb,
			    struct sk_buff_head *inputq)
{
	...
	switch (msg_user(hdr)) {
	case TIPC_LOW_IMPORTANCE:
	...
	case NAME_DISTRIBUTOR:
		...
		return true;
	case MSG_BUNDLER:
	case TUNNEL_PROTOCOL:
	case MSG_FRAGMENTER:
	case BCAST_PROTOCOL:
		return false; /* <--- Returns false WITHOUT kfree_skb() */
	...
	default:
		pr_warn("Dropping received illegal msg type\n");
		kfree_skb(skb);
		return true;
	}
}

The two receive paths where this occurs are:

1. Nested Bundle Path
   crafted packet
     -> outer msg_user = MSG_BUNDLER
     -> payload contains a structurally valid inner MSG_BUNDLER
     -> tipc_msg_extract() allocates and extracts the inner skb
     -> tipc_data_input() sees MSG_BUNDLER and returns false
        without consuming the skb
     -> the bundle extraction loop ignores the return value
     -> the extracted inner skb is leaked

2. Fragment Reassembly Path
   crafted fragment sequence
     -> reassembly completes in tipc_buf_append()
     -> the complete reassembled skb has msg_user = MSG_FRAGMENTER
     -> tipc_data_input() sees MSG_FRAGMENTER and returns false
        without consuming the skb
     -> the fragment completion path ignores the return value
     -> the reassembled skb is leaked

The reproducer source code and instructions have been sent to you
separately.


> Note that we do not want to add check for unreal cases because it
> causes performance regression.
> I tested your patch and it showed regression as below:
...

Regarding the reported performance regression, We conducted two
investigations: an assembly-level analysis of the hot path,
and a clean-environment performance benchmark with strict
CPU affinity.

1. Assembly & Hot-Path Analysis
--------------------------------
We compared the generated disassembly of baseline, patched, and
unlikely()-annotated builds using identical compiler toolchains and
optimization flags.

For the bundled-message receive path:

Baseline:
	call	tipc_data_input
	...
	call	tipc_msg_extract
	test	%al, %al
	jne	<next_extracted_message>

Patched:
	call	tipc_data_input
	test	%al, %al
	je	<drop_reason_path>
	...
	call	tipc_msg_extract
	test	%al, %al
	jne	<next_extracted_message>

The disassembly confirms that adding a single `test` + conditional
branch on a register cannot account for a 10%--15% throughput drop.

Adding unlikely() shifts the basic-block layout and branch direction
as expected:

	call	tipc_data_input
	test	%al, %al
	jne	<next_extracted_message>

Nevertheless, using unlikely() makes the exceptional path explicit,
improves readability, and is consistent with kernel coding
conventions. The fragment-reassembly path is similar.

2. Benchmark Methodology & Results
----------------------------------
We re-evaluated the patch series in a high-throughput, CPU-isolated
network namespace environment (`netns` + `veth`) to eliminate
external hardware I/O bottlenecks and measure pure kernel hot-path
overhead.

To eliminate thread contention and CPU migration jitter---which
previously caused bimodal performance drops (fluctuating between
~32 Gbps and ~48 Gbps when threads floated across cores)---we strictly
isolated CPU cores via taskset:
  - Client side (netperf): Pinned to CPU 0 and CPU 1
  - Server side (netserver): Pinned to CPU 2 and CPU 3
    (taskset -c 2,3)

Under identical hardware and isolation conditions, we ran 10-iteration
benchmarks comparing unpatched vs. patched kernels with NAGLE disabled.

The average throughput was:
+----------------+----------------+---------------+------------------+
| Message Size   | Unpatched      | Patched       | Delta (%)        |
+----------------+----------------+---------------+------------------+
| 64B (Small)    | 69.80 Mbps     | 68.58 Mbps    | -1.22M (-1.75%)  |
| 65,536B (Large)| 46,992.62 Mbps | 45,834.77 Mbps| -1.16G (-2.46%)  |
+----------------+----------------+---------------+------------------+

The results show NO statistically significant performance regression
(< 2.5% variation, falling entirely within standard measurement noise
and thermal throttling).


3. Explanation for the Previously Reported Degradation
------------------------------------------------------
During troubleshooting, we observed that if `netserver` (or softirq
handling) is not pinned to dedicated cores separate from `netperf`,
the Linux CFS scheduler frequently migrates `netserver` worker threads
onto the core running `netperf`.

When client and server threads contend for the same core, cache
bouncing and time-slice preemption can reduce throughput by 25%--30%
in 40Gbps+ testing.

To help align our benchmark methodologies, could you share a few
details about your test setup?
- The exact netperf repository version/commit used;
- Whether `netserver` and softirq handling were CPU-pinned;
- Whether node1 and node2 were physical hosts or virtual machines;
- NIC / link speed and CPU model;
- The exact method used to disable TIPC Nagle.

Thanks,
Xiaoshoukui