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

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

[xiaoshoukui]

From: xiaoshoukui <xiaoshoukui@gmail.com>

tipc_data_input() returns false when an extracted inner skb or
reassembled fragment is not consumed (e.g. unhandled protocol user
types).

The bundle extraction loop and fragment reassembly path in
tipc_link_input() both ignore this return value, leaving unconsumed
skbs unreachable and leaking SLUB memory.

Fix this by freeing unconsumed skbs with kfree_skb_reason() and
setting the drop reason to SKB_DROP_REASON_UNHANDLED_PROTO.

Fixes: c637c1035534 ("tipc: resolve race problem at unicast message reception")
Signed-off-by: xiaoshoukui <xiaoshoukui@gmail.com>
---
 net/tipc/link.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 49dfc098d89b..3278a559347b 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1306,15 +1306,18 @@ static int tipc_link_input(struct tipc_link *l, struct sk_buff *skb,
 		skb_queue_head_init(&tmpq);
 		l->stats.recv_bundles++;
 		l->stats.recv_bundled += msg_msgcnt(hdr);
-		while (tipc_msg_extract(skb, &iskb, &pos))
-			tipc_data_input(l, iskb, &tmpq);
+		while (tipc_msg_extract(skb, &iskb, &pos)) {
+			if (!tipc_data_input(l, iskb, &tmpq))
+				kfree_skb_reason(iskb, SKB_DROP_REASON_UNHANDLED_PROTO);
+		}
 		tipc_skb_queue_splice_tail(&tmpq, inputq);
 		return 0;
 	} else if (usr == MSG_FRAGMENTER) {
 		l->stats.recv_fragments++;
 		if (tipc_buf_append(reasm_skb, &skb)) {
 			l->stats.recv_fragmented++;
-			tipc_data_input(l, skb, inputq);
+			if (!tipc_data_input(l, skb, inputq))
+				kfree_skb_reason(skb, SKB_DROP_REASON_UNHANDLED_PROTO);
 		} else if (!*reasm_skb && !link_is_bc_rcvlink(l)) {
 			pr_warn_ratelimited("Unable to build fragment list\n");
 			return tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
-- 
2.34.1

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

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: fix memory leaks in bundle and fragment paths
>
>From: xiaoshoukui <xiaoshoukui@gmail.com>
>
>tipc_data_input() returns false when an extracted inner skb or reassembled
>fragment is not consumed (e.g. unhandled protocol user types).
>
>The bundle extraction loop and fragment reassembly path in
>tipc_link_input() both ignore this return value, leaving unconsumed skbs
>unreachable and leaking SLUB memory.
This cannot occur because the sending side already does sanity check to make sure that no invalid protocol type exists in bundled and fragmented messages.
This only occurs when you create fake TIPC messages or tampering with existing messages. This is an invalid use case because TIPC is being used in an insecure
environment. In such environment, IPSec or TIPC encryption must be used as mentioned here https://datatracker.ietf.org/doc/html/draft-maloy-tipc-01.txt#section-6

>
>Fix this by freeing unconsumed skbs with kfree_skb_reason() and setting the

This approach can break user applications as explained below.

>drop reason to SKB_DROP_REASON_UNHANDLED_PROTO.
>
>Fixes: c637c1035534 ("tipc: resolve race problem at unicast message
>reception")
>Signed-off-by: xiaoshoukui <xiaoshoukui@gmail.com>
>---
> net/tipc/link.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
>diff --git a/net/tipc/link.c b/net/tipc/link.c index 49dfc098d89b..3278a559347b
>100644
>--- a/net/tipc/link.c
>+++ b/net/tipc/link.c
>@@ -1306,15 +1306,18 @@ static int tipc_link_input(struct tipc_link *l, struct
>sk_buff *skb,
> 		skb_queue_head_init(&tmpq);
> 		l->stats.recv_bundles++;
> 		l->stats.recv_bundled += msg_msgcnt(hdr);
>-		while (tipc_msg_extract(skb, &iskb, &pos))
>-			tipc_data_input(l, iskb, &tmpq);
>+		while (tipc_msg_extract(skb, &iskb, &pos)) {
>+			if (!tipc_data_input(l, iskb, &tmpq))

This check is redundant because bundled messages do not contain message types that can make tipc_data_input() return false. The only exception is tampering with TIPC messages.
If that is the case, a fake valid message type can be inserted into the bundle message and tipc_data_input() returns true but user applications are broken by corrupt messages.

>+				kfree_skb_reason(iskb,
>SKB_DROP_REASON_UNHANDLED_PROTO);

Compiling  warnings: https://netdev-ctrl.bots.linux.dev/logview.html?f=/logs/build/1166117/14820384/checkpatch/stdout

>+		}
> 		tipc_skb_queue_splice_tail(&tmpq, inputq);
> 		return 0;
> 	} else if (usr == MSG_FRAGMENTER) {
> 		l->stats.recv_fragments++;
> 		if (tipc_buf_append(reasm_skb, &skb)) {
> 			l->stats.recv_fragmented++;
>-			tipc_data_input(l, skb, inputq);
>+			if (!tipc_data_input(l, skb, inputq))

This check is redundant because fragmented messages do not contain message types that can make tipc_data_input() return false. The only exception is tampering with TIPC messages.
If that is the case:
- dropping the invalid fragmented message will cause the reassembled message corrupt (For example: sending a 65KB message but dropping/truncating 1500 bytes). As a result, user
  applications receive corrupt messages.
- a fake fragmented message can ben sent and tipc_data_input() returns true but user applications are broken by corrupt messages.

>+				kfree_skb_reason(skb,
>SKB_DROP_REASON_UNHANDLED_PROTO);
> 		} else if (!*reasm_skb && !link_is_bc_rcvlink(l)) {
> 			pr_warn_ratelimited("Unable to build fragment
>list\n");
> 			return tipc_link_fsm_evt(l, LINK_FAILURE_EVT);
>--
>2.34.1
>

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

[xiaoshoukui]

Hi Tung,

Thanks for the review and feedback.

On Thu, Sep 17, 2026 at 03:51:16AM +0000, Tung Quang Nguyen wrote:
> This cannot occur because the sending side already does sanity check to
> make sure that no invalid protocol type exists in bundled and fragmented
> messages.
> This only occurs when you create fake TIPC messages or tampering with
> existing messages. This is an invalid use case because TIPC is being used
> in an insecure environment. In such environment, IPSec or TIPC encryption
> must be used as mentioned here
> https://datatracker.ietf.org/doc/html/draft-maloy-tipc-01.txt#section-6

I agree that the normal transmit path performs checks that prevent the
relevant internal protocol users from being bundled. In particular,
tipc_msg_try_bundle() rejects MSG_FRAGMENTER, TUNNEL_PROTOCOL, and
BCAST_PROTOCOL.

However, I think this is separate from the receive-side skb ownership
issue addressed by this patch. The patch does not assume that these
message types are normally generated by the transmit path. It handles
the case where an skb has reached tipc_data_input() and that function
explicitly reports that it did not consume the skb.

The relevant point here is that an skb that is not consumed by
tipc_data_input() must still have a defined owner and cleanup path.

> This check is redundant because bundled messages do not contain message
> types that can make tipc_data_input() return false. The only exception
> is tampering with TIPC messages.
> If that is the case, a fake valid message type can be inserted into the
> bundle message and tipc_data_input() returns true but user applications
> are broken by corrupt messages.

I agree that a tampered message can be changed to a valid user type.
In that case, tipc_data_input() returns true and the message is queued
for further receive processing.

However, that case does not exercise the check added by this patch.
The added kfree_skb_reason() is only executed when tipc_data_input()
returns false.

The two cases therefore have different ownership behavior:

valid user type
    -> tipc_data_input() returns true
    -> skb is queued to inputq
    -> receive processing continues

unhandled protocol user
    -> tipc_data_input() returns false
    -> skb is not queued or consumed by tipc_data_input()

For the latter case, the bundle path currently ignores the return value:

while (tipc_msg_extract(skb, &iskb, &pos))
    tipc_data_input(l, iskb, &tmpq);

tipc_msg_extract() has already allocated and validated iskb before
returning it. For the protocol users listed above, tipc_data_input()
then returns false without consuming the skb. Since the caller ignores
that return value, the extracted skb has no subsequent cleanup path.

The proposed check only closes this ownership gap; it does not attempt
to solve message-integrity or anti-tampering problems.

> >+				kfree_skb_reason(iskb,
> >SKB_DROP_REASON_UNHANDLED_PROTO);
> 
> Compiling warnings:
> https://netdev-ctrl.bots.linux.dev/logview.html?f=/logs/build/1166117/14820384/checkpatch/stdout

Thanks for pointing this out. I will fix the checkpatch formatting issue
in v2.

> This check is redundant because fragmented messages do not contain message
> types that can make tipc_data_input() return false. The only exception
> is tampering with TIPC messages.
> If that is the case:
> - dropping the invalid fragmented message will cause the reassembled message
>   corrupt (For example: sending a 65KB message but dropping/truncating 1500
>   bytes). As a result, user applications receive corrupt messages.
> - a fake fragmented message can ben sent and tipc_data_input() returns true
>   but user applications are broken by corrupt messages.

Regarding the fragment path, I believe the proposed change does not drop
an individual fragment during reassembly.

tipc_buf_append() is documented to return 1 only when reassembly is
complete. For the first and intermediate fragments it returns 0. When
the last fragment is received, it validates the reassembled message,
assigns the complete reassembled skb to *buf, clears the reassembly
state, and returns 1.

Therefore, the relevant sequence is:

  first fragment
      |
  intermediate fragments
      |
  last fragment
      |
  complete reassembly
      |
  tipc_msg_validate()
      |
  tipc_data_input()

The added kfree_skb_reason() is reached only after this complete
reassembly step:

  if (tipc_buf_append(reasm_skb, &skb)) {
      l->stats.recv_fragmented++;
      if (!tipc_data_input(l, skb, inputq))
          kfree_skb_reason(skb,
                           SKB_DROP_REASON_UNHANDLED_PROTO);
  }

At that point, skb refers to the complete reassembled message, not to an
individual 1500-byte fragment.

If tipc_data_input() returns true, the reassembled skb is queued to
inputq as before. If it returns false, the complete reassembled skb is
not consumed by tipc_data_input(). Without an explicit cleanup at this
point, the reassembled skb and its associated fragment data become
unreachable.

Thus, the proposed change does not truncate a 65KB message by dropping
one 1500-byte fragment. It only releases the complete reassembled skb
when the receive-side dispatcher reports that it did not consume it.

I agree that a forged valid user type is a separate message-integrity
issue. This patch is not intended to provide authentication or
protection against message tampering; its purpose is limited to ensuring
that skbs rejected by tipc_data_input() are properly released.

Thanks again for the review.

Best regards,
xiaoshoukui

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

[Tung Quang Nguyen]

>Subject: Re: [PATCH net] tipc: fix memory leaks in bundle and fragment paths
>
>Hi Tung,
>
>Thanks for the review and feedback.
>
>On Thu, Sep 17, 2026 at 03:51:16AM +0000, Tung Quang Nguyen wrote:
>> This cannot occur because the sending side already does sanity check
>> to make sure that no invalid protocol type exists in bundled and
>> fragmented messages.
>> This only occurs when you create fake TIPC messages or tampering with
>> existing messages. This is an invalid use case because TIPC is being
>> used in an insecure environment. In such environment, IPSec or TIPC
>> encryption must be used as mentioned here
>> https://datatracker.ietf.org/doc/html/draft-maloy-tipc-01.txt#section-
>> 6
>
>I agree that the normal transmit path performs checks that prevent the
>relevant internal protocol users from being bundled. In particular,
>tipc_msg_try_bundle() rejects MSG_FRAGMENTER, TUNNEL_PROTOCOL, and
>BCAST_PROTOCOL.
>
>However, I think this is separate from the receive-side skb ownership issue
>addressed by this patch. The patch does not assume that these message types
>are normally generated by the transmit path. It handles the case where an skb
>has reached tipc_data_input() and that function explicitly reports that it did
>not consume the skb.

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 relevant point here is that an skb that is not consumed by
>tipc_data_input() must still have a defined owner and cleanup path.
>
>> This check is redundant because bundled messages do not contain
>> message types that can make tipc_data_input() return false. The only
>> exception is tampering with TIPC messages.
>> If that is the case, a fake valid message type can be inserted into
>> the bundle message and tipc_data_input() returns true but user
>> applications are broken by corrupt messages.
>
>I agree that a tampered message can be changed to a valid user type.
>In that case, tipc_data_input() returns true and the message is queued for
>further receive processing.
>
>However, that case does not exercise the check added by this patch.
>The added kfree_skb_reason() is only executed when tipc_data_input()
>returns false.

The check is redundant because tipc_data_input() never returns false after calling tipc_msg_extract() or tipc_buf_append().

>
>The two cases therefore have different ownership behavior:
>
>valid user type
>    -> tipc_data_input() returns true
>    -> skb is queued to inputq
>    -> receive processing continues
>

This is correct but there is more valid use case:
-> tipc_data_input() returns false
-> skb is passed to tipc_link_input()
->  tipc_data_input() returns true
-> skb is queued to inputq

>unhandled protocol user
>    -> tipc_data_input() returns false
>    -> skb is not queued or consumed by tipc_data_input()

This is not correct. If tipc_data_input() returns false, the skb will be dropped.

>
>For the latter case, the bundle path currently ignores the return value:
>
>while (tipc_msg_extract(skb, &iskb, &pos))
>    tipc_data_input(l, iskb, &tmpq);
>
>tipc_msg_extract() has already allocated and validated iskb before returning it.
>For the protocol users listed above, tipc_data_input() then returns false
>without consuming the skb. Since the caller ignores that return value, the
>extracted skb has no subsequent cleanup path.

This is not correct as explained above.

>
>The proposed check only closes this ownership gap; it does not attempt to
>solve message-integrity or anti-tampering problems.
>
>> >+				kfree_skb_reason(iskb,
>> >SKB_DROP_REASON_UNHANDLED_PROTO);
>>
>> Compiling warnings:
>> https://netdev-ctrl.bots.linux.dev/logview.html?f=/logs/build/1166117/
>> 14820384/checkpatch/stdout
>
>Thanks for pointing this out. I will fix the checkpatch formatting issue in v2.
>
>> This check is redundant because fragmented messages do not contain
>> message types that can make tipc_data_input() return false. The only
>> exception is tampering with TIPC messages.
>> If that is the case:
>> - dropping the invalid fragmented message will cause the reassembled
>message
>>   corrupt (For example: sending a 65KB message but dropping/truncating
>1500
>>   bytes). As a result, user applications receive corrupt messages.
>> - a fake fragmented message can ben sent and tipc_data_input() returns true
>>   but user applications are broken by corrupt messages.
>
>Regarding the fragment path, I believe the proposed change does not drop an
>individual fragment during reassembly.
>
>tipc_buf_append() is documented to return 1 only when reassembly is
>complete. For the first and intermediate fragments it returns 0. When the last
>fragment is received, it validates the reassembled message, assigns the
>complete reassembled skb to *buf, clears the reassembly state, and returns 1.
>
>Therefore, the relevant sequence is:
>
>  first fragment
>      |
>  intermediate fragments
>      |
>  last fragment
>      |
>  complete reassembly
>      |
>  tipc_msg_validate()
>      |
>  tipc_data_input()
>
>The added kfree_skb_reason() is reached only after this complete reassembly
>step:
>
>  if (tipc_buf_append(reasm_skb, &skb)) {
>      l->stats.recv_fragmented++;
>      if (!tipc_data_input(l, skb, inputq))
>          kfree_skb_reason(skb,
>                           SKB_DROP_REASON_UNHANDLED_PROTO);
>  }
>
>At that point, skb refers to the complete reassembled message, not to an
>individual 1500-byte fragment.
>
>If tipc_data_input() returns true, the reassembled skb is queued to inputq as
>before. If it returns false, the complete reassembled skb is not consumed by
>tipc_data_input(). Without an explicit cleanup at this point, the reassembled
>skb and its associated fragment data become unreachable.
>
>Thus, the proposed change does not truncate a 65KB message by dropping one
>1500-byte fragment. It only releases the complete reassembled skb when the
>receive-side dispatcher reports that it did not consume it.

You are right. I misread tipc_buf_append().

>
>I agree that a forged valid user type is a separate message-integrity issue. This
>patch is not intended to provide authentication or protection against message
>tampering; its purpose is limited to ensuring that skbs rejected by
>tipc_data_input() are properly released.

tipc_data_input() releases invalid skbs as expected.
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:

Setup: disable NAGLE to generate bundling messages. Two netperf threads with different message sizes are executed at the same time.

[BEFORE PATCH]:
node1 ~ # taskset -c 0 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 64
TIPC STREAM TEST to <1.0.561:547203127>
Throughput 
521.49

node1 ~ # taskset -c 1 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 65536
TIPC STREAM TEST to <1.0.561:309928041>
Throughput 
9342.63    

[AFTER PATCH]:
node1 ~ # taskset -c 0 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 64
TIPC STREAM TEST to <1.0.561:1589461199>
Throughput 
440.38      

node1 ~ # taskset -c 1 netperf -n 2 -f m -c -C -H 100.100.100.2 -t TIPC_STREAM -l 60 -- -O THROUGHPUT -m 65536
TIPC STREAM TEST to <1.0.561:2903145288>
Throughput 
8417.67

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

[xiaoshoukui]

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