[PATCH net-next 0/4] net: rmnet: optimize hot paths to alleviate CPU/memory bottlenecks

[PATCH net-next 0/4] net: rmnet: optimize hot paths to alleviate CPU/memory bottlenecks

[Koen Vandeputte]

While testing embedded ARM platforms over a high-speed
private 5G SA network, we observed severe CPU and memory
bottlenecks limiting overall throughput.

Examining the rmnet driver revealed several opportunities
to significantly increase efficiency,
particularly on the RX deaggregation hot path.

This patch series addresses these bottlenecks by optimizing
memory allocations, reducing unnecessary payload copies,
and improving timestamping efficiency.

Test Environment

Hardware: IPQ4019 (Quad ARM Cortex-A7 @ 717MHz), 512MB DDR3, USB 3.0
Modem: Telit FN990A40
OS: OpenWRT-based
Network: Private 5G SA (n78, 50MHz band, 4x4 MIMO, 1Gbps core uplink)

Methodology

Results are based on the average of 10 consecutive speedtests against
a local speedtest server located in the same datacenter.

Performance Results (Download)

Before: ~318 Mbps average (Peak: ~353 Mbps)
After:  ~389 Mbps average (Peak: ~458 Mbps)

Koen Vandeputte (4):
  net: rmnet: use fast monotonic time for tx aggregation
  net: rmnet: optimize rx deaggregation memory allocation
  net: rmnet: conditionally expand skb headroom in ingress handler
  net: rmnet: optimize rx handler by replacing skb_linearize with
    pskb_may_pull

 .../ethernet/qualcomm/rmnet/rmnet_config.h    |  4 ++--
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 10 ++++----
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 24 ++++++++++---------
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH net-next 1/4] net: rmnet: use fast monotonic time for tx aggregation

[Koen Vandeputte]

The rmnet egress aggregation logic currently relies on ktime_get_real_ts64()
to determine if the aggregation bypass time threshold has been reached.
Calling a wall-clock time function on the transmit hot path introduces
significant performance bottlenecks, causing cacheline bouncing and pipeline
stalls when processing high packet volumes. Furthermore, manipulating and
comparing struct timespec64 fields adds unnecessary branching overhead.

Since the driver only needs to measure elapsed time between consecutive
packets to evaluate the aggregation bypass condition, absolute wall-clock
time is not required.

Replace ktime_get_real_ts64() with ktime_get_mono_fast_ns(). This reads a
lockless, per-CPU timestamp directly in nanoseconds, returning a simple u64.
Update the aggregation state variables (agg_time and agg_last) in
struct rmnet_port to u64 accordingly.

This optimization significantly reduces CPU overhead, eliminates struct
timespec64 math, and provides a much faster, cache-friendly timekeeping
mechanism for high-throughput egress traffic.

Signed-off-by: Koen Vandeputte <koen.vandeputte@citymesh.com>
---
 .../ethernet/qualcomm/rmnet/rmnet_config.h    |  4 ++--
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 22 ++++++++++---------
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
index 5adda0323dda..78c0289b6583 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
@@ -47,8 +47,8 @@ struct rmnet_port {
 	struct sk_buff *skbagg_tail;
 	int agg_state;
 	u8 agg_count;
-	struct timespec64 agg_time;
-	struct timespec64 agg_last;
+	u64 agg_time;
+	u64 agg_last;
 	struct hrtimer hrtimer;
 	struct work_struct agg_wq;
 };
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 39d6d084e73f..2eafb1d969c1 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -536,7 +536,7 @@ static void reset_aggr_params(struct rmnet_port *port)
 	port->skbagg_head = NULL;
 	port->agg_count = 0;
 	port->agg_state = 0;
-	memset(&port->agg_time, 0, sizeof(struct timespec64));
+	port->agg_time = 0;
 }
 
 static void rmnet_send_skb(struct rmnet_port *port, struct sk_buff *skb)
@@ -591,21 +591,23 @@ static enum hrtimer_restart rmnet_map_flush_tx_packet_queue(struct hrtimer *t)
 unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
 				    struct net_device *orig_dev)
 {
-	struct timespec64 diff, last;
+	u64 diff, last, now;
 	unsigned int len = skb->len;
 	struct sk_buff *agg_skb;
 	int size;
 
 	spin_lock_bh(&port->agg_lock);
-	memcpy(&last, &port->agg_last, sizeof(struct timespec64));
-	ktime_get_real_ts64(&port->agg_last);
+	last = port->agg_last;
+
+	now = ktime_get_mono_fast_ns();
+	port->agg_last = now;
 
 	if (!port->skbagg_head) {
 		/* Check to see if we should agg first. If the traffic is very
 		 * sparse, don't aggregate.
 		 */
 new_packet:
-		diff = timespec64_sub(port->agg_last, last);
+		diff = now - last;
 		size = port->egress_agg_params.bytes - skb->len;
 
 		if (size < 0) {
@@ -614,8 +616,7 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
 			return 0;
 		}
 
-		if (diff.tv_sec > 0 || diff.tv_nsec > RMNET_AGG_BYPASS_TIME_NSEC ||
-		    size == 0)
+		if (diff > RMNET_AGG_BYPASS_TIME_NSEC || size == 0)
 			goto no_aggr;
 
 		port->skbagg_head = skb_copy_expand(skb, 0, size, GFP_ATOMIC);
@@ -625,11 +626,12 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
 		dev_kfree_skb_any(skb);
 		port->skbagg_head->protocol = htons(ETH_P_MAP);
 		port->agg_count = 1;
-		ktime_get_real_ts64(&port->agg_time);
+		port->agg_time = now;
 		skb_frag_list_init(port->skbagg_head);
 		goto schedule;
 	}
-	diff = timespec64_sub(port->agg_last, port->agg_time);
+
+	diff = now - port->agg_time;
 	size = port->egress_agg_params.bytes - port->skbagg_head->len;
 
 	if (skb->len > size) {
@@ -653,7 +655,7 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
 	port->skbagg_tail = skb;
 	port->agg_count++;
 
-	if (diff.tv_sec > 0 || diff.tv_nsec > port->egress_agg_params.time_nsec ||
+	if (diff > port->egress_agg_params.time_nsec ||
 	    port->agg_count >= port->egress_agg_params.count ||
 	    port->skbagg_head->len == port->egress_agg_params.bytes) {
 		agg_skb = port->skbagg_head;
-- 
2.43.0

[PATCH net-next 2/4] net: rmnet: optimize rx deaggregation memory allocation

[Koen Vandeputte]

Currently, rmnet_map_deaggregate() uses alloc_skb() with GFP_ATOMIC to
allocate new socket buffers for individual packets extracted from an
aggregated frame. Under high-speed receive workloads, such as processing
large 16KB frames from a USB modem, repeatedly hitting the generic
kernel slab allocator introduces unnecessary lock contention and high
CPU overhead.

Replace alloc_skb() with netdev_alloc_skb() on the deaggregation path.
netdev_alloc_skb() is specifically designed for the network receive hot
path, utilizing a fast, lockless per-CPU NAPI cache. Furthermore, it
automatically provisions standard network padding (NET_SKB_PAD), ensuring
that the newly extracted IP headers are optimally cache-aligned for
efficient processing by the upper networking stack.

This change significantly reduces memory allocation overhead, prevents
slab allocator contention, and improves overall throughput when receiving
large bursts of aggregated traffic.

Signed-off-by: Koen Vandeputte <koen.vandeputte@citymesh.com>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 2eafb1d969c1..ccdfde3711db 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -390,7 +390,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
 	if (!packet_len)
 		return NULL;
 
-	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
+	skbn = netdev_alloc_skb(skb->dev, packet_len + RMNET_MAP_DEAGGR_SPACING);
 	if (!skbn)
 		return NULL;
 
-- 
2.43.0

[PATCH net-next 3/4] net: rmnet: conditionally expand skb headroom in ingress handler

[Koen Vandeputte]

Currently, when the rmnet driver processes ingress packets for an Ethernet
device (ARPHRD_ETHER), it unconditionally calls pskb_expand_head() to
ensure there is enough room to push the MAC header. This forces a costly
memory reallocation and payload copy for every single packet on the RX
hotpath, severely degrading throughput and increasing CPU overhead.

In many modern architectures, the underlying physical driver (e.g., USB)
can be configured to pre-allocate this extra ETH_HLEN headroom when
minting the initial SKB.

Optimize the ingress path by checking if skb_headroom(skb) < ETH_HLEN
before triggering the expansion. If the packet arrives with sufficient
headroom, the driver now skips the reallocation entirely and safely pushes
the header. The expensive pskb_expand_head() operation is now strictly a
fallback, allowing properly configured hardware to achieve zero-copy MAC
header insertion.

Testing this using a temporary print in the new condition showed that
the expansion is not triggered as enough space is already available.

Signed-off-by: Koen Vandeputte <koen.vandeputte@citymesh.com>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index aa5523f4618e..95c3e3934fd3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -114,9 +114,11 @@ rmnet_map_ingress_handler(struct sk_buff *skb,
 	u32 data_format;
 
 	if (skb->dev->type == ARPHRD_ETHER) {
-		if (pskb_expand_head(skb, ETH_HLEN, 0, GFP_ATOMIC)) {
-			kfree_skb(skb);
-			return;
+		if (skb_headroom(skb) < ETH_HLEN) {
+			if (pskb_expand_head(skb, ETH_HLEN, 0, GFP_ATOMIC)) {
+				kfree_skb(skb);
+				return;
+			}
 		}
 
 		skb_push(skb, ETH_HLEN);
-- 
2.43.0

[PATCH net-next 4/4] net: rmnet: optimize rx handler by replacing skb_linearize with pskb_may_pull

[Koen Vandeputte]

The rmnet RX handler currently calls skb_linearize() unconditionally
on every incoming packet.

When receiving large, hardware-aggregated frames
(e.g., 16KB or 32KB from USB modems via qmi_wwan),
this forces the network stack to allocate a massive
contiguous buffer and memcpy the entire scattered payload
out of paged memory.

This severe performance bottleneck is completely unnecessary
because the RX handler only needs to inspect the initial
4-byte MAP header to determine the logical endpoint
and route the packet.

Replace skb_linearize() with pskb_may_pull().
This targeted check ensures the MAP header is safely accessible
in the linear data area while allowing the bulky payload to
remain untouched in paged memory fragments.
This preserves the zero-copy architecture for aggregated frames,
drastically reducing CPU overhead, cache thrashing,
and memory allocations on the RX hotpath.

On a slower embedded board, this improves throughput by ~10%.

Signed-off-by: Koen Vandeputte <koen.vandeputte@citymesh.com>
---
 drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index 95c3e3934fd3..a2379df69a6d 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -217,7 +217,7 @@ rx_handler_result_t rmnet_rx_handler(struct sk_buff **pskb)
 	if (!skb)
 		goto done;
 
-	if (skb_linearize(skb)) {
+	if (!pskb_may_pull(skb, sizeof(struct rmnet_map_header))) {
 		kfree_skb(skb);
 		goto done;
 	}
-- 
2.43.0

Re: [PATCH net-next 4/4] net: rmnet: optimize rx handler by replacing skb_linearize with pskb_may_pull

[Sean Tranchetti]

Hi Koen,

Unfortunately this patch only works in MAP v1 configuration.
In either MAPv4 or MAPv5 with checksum offload support, the driver needs
to read more than just the standard 4 byte MAP header.

Re: [PATCH net-next 4/4] net: rmnet: optimize rx handler by replacing skb_linearize with pskb_may_pull

[Koen Vandeputte]

On Fri, Oct 2, 2026 at 10:48 PM Sean Tranchetti
<sean.tranchetti@oss.qualcomm.com> wrote:
>
> Hi Koen,
>
> Unfortunately this patch only works in MAP v1 configuration.
> In either MAPv4 or MAPv5 with checksum offload support, the driver needs
> to read more than just the standard 4 byte MAP header.

Hi Sean,

Thanks a lot for checking this.
You are right .. I did test it on V5 also, but got lucky here that
data was already aligned by nature.

I'll check if this can be fixed and submit a v2

Appreciated!

Koen

Re: [PATCH net-next 1/4] net: rmnet: use fast monotonic time for tx aggregation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 3 (2) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes the new clock source wrongly and
  gives no measurements for its performance claims.
- [Medium] The patch calls ktime_get_mono_fast_ns() from a driver and
  describes it as monotonic.
- [Medium] rmnet: stale timestamp assigned to agg_time causes premature
  tx flush
- [Low] Dropping the `diff.tv_sec > 0` term quietly changes the in-path
  flush behaviour of rmnet_map_tx_aggregate() for tx_aggr_time_usecs of…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a
  time-of-check/time-of-use race in rmnet_map_tx_aggregate() that leaks…
- [Medium] This problem wasn't introduced by this patch, but
  rmnet_map_update_ul_agg_config() computes…
- [Medium] This problem wasn't introduced by this patch, but both flush
  branches of rmnet_map_tx_aggregate() (lines 637-645 and 658-667) call…

--

net: rmnet: use fast monotonic time for tx aggregation

This commit replaces ktime_get_real_ts64() with ktime_get_mono_fast_ns()
in rmnet_map_tx_aggregate(). It also changes agg_time and agg_last in
struct rmnet_port from struct timespec64 to u64 nanoseconds, so the bypass
and flush age checks become plain u64 subtractions.

> Calling a wall-clock time function on the transmit hot path introduces
> significant performance bottlenecks, causing cacheline bouncing and pipeline
> stalls when processing high packet volumes.

[ ... ]

> Replace ktime_get_real_ts64() with ktime_get_mono_fast_ns(). This reads a
> lockless, per-CPU timestamp directly in nanoseconds, returning a simple u64.

[ ... ]

> This optimization significantly reduces CPU overhead, eliminates struct
> timespec64 math, and provides a much faster, cache-friendly timekeeping
> mechanism for high-throughput egress traffic.

[Severity: Medium]
Is this description of ktime_get_mono_fast_ns() accurate? In
kernel/time/timekeeping.c it reads from a single global object:

static struct tk_fast tk_fast_mono ____cacheline_aligned = {

u64 notrace ktime_get_mono_fast_ns(void)
{
	return __ktime_get_fast_ns(&tk_fast_mono);
}

__ktime_get_fast_ns() then runs a latch seqcount retry loop and reads the
hardware clocksource. ktime_get_real_ts64() does the same:

	do {
		seq = read_seqcount_latch(&tkf->seq);
		tkr = tkf->base + (seq & 0x01);
		now = ktime_to_ns(tkr->base);
		now += timekeeping_get_ns(tkr);
	} while (read_seqcount_latch_retry(&tkf->seq, seq));

Nothing in this path is per-CPU. Seqcount readers also never write the
shared cacheline, so the old ktime_get_real_ts64() reader path wouldn't
cause cacheline bouncing either.

The real savings appear to be a latch seqcount in place of a plain
seqcount, plus no timespec64 normalisation and no two-field compare.

Could the commit message describe that instead? Could it also include
measurements to support "significantly reduces CPU overhead"?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 39d6d084e73f0..2eafb1d969c1a 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -591,21 +591,23 @@ static enum hrtimer_restart rmnet_map_flush_tx_packet_queue(struct hrtimer *t)
>  unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
>  				    struct net_device *orig_dev)
>  {

[ ... ]

>  	spin_lock_bh(&port->agg_lock);
> -	memcpy(&last, &port->agg_last, sizeof(struct timespec64));
> -	ktime_get_real_ts64(&port->agg_last);
> +	last = port->agg_last;
> +
> +	now = ktime_get_mono_fast_ns();
> +	port->agg_last = now;

[Severity: Medium]
Is ktime_get_mono_fast_ns() a good choice for a driver? Its kernel-doc
says it is not strictly monotonic:

 * This timestamp is not guaranteed to be monotonic across an update.
 ...
 * So reader 6 will observe time going backwards versus reader 5.
 ...
 * Callers need to be aware of that and deal with it.

Documentation/core-api/timekeeping.rst also says:

	machine check reporting, but most drivers should never call them,
	since the time is allowed to jump under certain conditions.

port->agg_last and port->agg_time are shared across CPUs.
rmnet_vnd_setup() sets rmnet_dev->lltx = true, and several muxed rmnet
devices can share one rmnet_port. As a result, rmnet_map_tx_aggregate()
runs on several CPUs at once. agg_lock serialises the transmitters, but
it does not serialise them against update_fast_timekeeper().

Suppose CPU A reads now from the old base and stores it in agg_last. CPU B
then reads from the new base after the latch switch. B's now can be
smaller than last.

Neither unsigned subtraction guards against that:

	diff = now - last;
	...
	diff = now - port->agg_time;

In that case diff wraps to roughly U64_MAX. That either forces the
sparse traffic bypass (diff > RMNET_AGG_BYPASS_TIME_NSEC) or flushes a
partial aggregate early (diff > egress_agg_params.time_nsec).

The effect is small: one packet skips aggregation, or one aggregate is
flushed early. The old timespec64_sub() code took the same paths on a
negative delta. Even so, the commit message calls this clock monotonic.

Would ktime_get_ns() fit better here? It is monotonic, returns a u64, and
still avoids the timespec64 math.

>  
>  	if (!port->skbagg_head) {
>  		/* Check to see if we should agg first. If the traffic is very
>  		 * sparse, don't aggregate.
>  		 */
>  new_packet:
> -		diff = timespec64_sub(port->agg_last, last);
> +		diff = now - last;
>  		size = port->egress_agg_params.bytes - skb->len;

[ ... ]

> @@ -625,11 +626,12 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
>  		dev_kfree_skb_any(skb);
>  		port->skbagg_head->protocol = htons(ETH_P_MAP);
>  		port->agg_count = 1;
> -		ktime_get_real_ts64(&port->agg_time);
> +		port->agg_time = now;
>  		skb_frag_list_init(port->skbagg_head);
>  		goto schedule;
>  	}
> -	diff = timespec64_sub(port->agg_last, port->agg_time);
> +
> +	diff = now - port->agg_time;
>  	size = port->egress_agg_params.bytes - port->skbagg_head->len;
>  
>  	if (skb->len > size) {

[Severity: Medium]
Does this change when a new aggregate's age starts counting on the
goto new_packet path?

now is read once, at the top of rmnet_map_tx_aggregate(). Sometimes
the incoming skb does not fit in the current aggregate. The function
then drops agg_lock, calls hrtimer_cancel() and rmnet_send_skb() for
the old aggregate, re-takes agg_lock and jumps back to new_packet. A
new aggregate started there gets agg_time set to the timestamp read
before all of that.

The old code called ktime_get_real_ts64(&port->agg_time) at this
point, so agg_time was a fresh reading taken after the flush. With
this patch, agg_time is backdated by however long hrtimer_cancel()
and the nested dev_queue_xmit() took. hrtimer_cancel() can spin until
a running rmnet_map_flush_tx_packet_queue() finishes.
dev_queue_xmit() on the real device can contend on the qdisc lock.

The next packet computes now - port->agg_time against that stale
value. It is therefore more likely to exceed
egress_agg_params.time_nsec, so the new aggregate is flushed earlier
than configured. The hrtimer armed at schedule: uses an expiry
relative to the current time. The timer and the age check therefore
no longer agree on when the aggregate started.

This path runs on every size-triggered flush. Under sustained traffic
that overflows aggregates, it seems to work against the throughput
goal of this patch.

Could agg_time take a fresh ktime_get_mono_fast_ns() reading after
agg_lock is re-acquired on this path, as the old code effectively
did? The bypass check at new_packet compares against last. It used
the entry-time agg_last in the old code too, so only agg_time seems
to need the fresh value.

[Severity: High]
This isn't a bug introduced by this patch, but can this branch leak an
aggregate that another CPU built?

	if (skb->len > size) {
		agg_skb = port->skbagg_head;
		reset_aggr_params(port);
		spin_unlock_bh(&port->agg_lock);
		hrtimer_cancel(&port->hrtimer);
		rmnet_send_skb(port, agg_skb);
		spin_lock_bh(&port->agg_lock);
		goto new_packet;
	}

The new_packet label is inside the if (!port->skbagg_head) block. After
agg_lock is retaken, the goto skips that check.

With lltx = true and several muxed devices sharing one rmnet_port,
another CPU can run while the lock is dropped:

CPU A                              CPU B
reset_aggr_params()
  skbagg_head = NULL
spin_unlock_bh()
hrtimer_cancel()                   spin_lock_bh()
rmnet_send_skb()                   sees skbagg_head == NULL
                                   small diff, A just set agg_last
                                   skbagg_head = skb_copy_expand(...)
                                   spin_unlock_bh()
spin_lock_bh()
goto new_packet

If A's diff is small when it resumes, A overwrites the head without
checking it first:

		port->skbagg_head = skb_copy_expand(skb, 0, size, GFP_ATOMIC);

Nothing references B's head skb or any frag_list entries appended to it
after that, so they leak. Those packets were already counted as
transmitted in rmnet_vnd_tx_fixup_len(), but they are never sent.

If A's diff exceeds the bypass threshold instead, A sends its skb directly
ahead of B's pending aggregate, so packets are reordered.

Could port->skbagg_head be re-checked after the lock is re-acquired? For
example, the goto could target a label placed before the
if (!port->skbagg_head) test.

[ ... ]

> @@ -653,7 +655,7 @@ unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port
>  	port->skbagg_tail = skb;
>  	port->agg_count++;
>  
> -	if (diff.tv_sec > 0 || diff.tv_nsec > port->egress_agg_params.time_nsec ||
> +	if (diff > port->egress_agg_params.time_nsec ||

[Severity: Low]
Is it intended that removing the diff.tv_sec > 0 term changes flush
behaviour for timeouts of one second or more?

tv_nsec used to be normalised below 1e9, so the next packet flushed any
aggregate that was at least 1s old. In effect, a configured time_nsec of
1s or more was capped at 1s when a packet arrived.

The full configured timeout now applies. rmnet_set_coalesce() passes
tx_aggr_time_usecs through without an upper bound, and the netlink
policy for ETHTOOL_A_COALESCE_TX_AGGR_TIME_USECS is plain NLA_U32.

With a 2s timeout, a packet that arrives when the aggregate is 1.1s old
used to trigger a flush and no longer does.

The new behaviour matches the hrtimer, which was always armed with the
full time_nsec. Could the commit message mention this change?

[Severity: Medium]
This isn't a bug introduced by this patch, but on 32-bit builds the
time_nsec value compared here can be wrong.
rmnet_map_update_ul_agg_config() does:

	port->egress_agg_params.time_nsec = time * NSEC_PER_USEC;

time is u32 and NSEC_PER_USEC is 1000L. On 32-bit, long is 32 bits, so
the product is computed as a 32-bit unsigned long. It wraps before it is
stored in the u64 time_nsec.

rmnet_set_coalesce() bounds tx_aggr_max_frames and tx_aggr_max_bytes,
but not tx_aggr_time_usecs. Any value above about 4294967 usec wraps.
For example, 5000000 usec is stored as 705032704 ns, and
rmnet_get_coalesce() reports 705032 usec back.

Both the hrtimer and this age check use the wrapped value.

>  	    port->agg_count >= port->egress_agg_params.count ||
>  	    port->skbagg_head->len == port->egress_agg_params.bytes) {
>  		agg_skb = port->skbagg_head;

[Severity: Medium]
This is a pre-existing issue, but can a new aggregate end up with no timer
armed? Both flush branches do this:

		reset_aggr_params(port);
		spin_unlock_bh(&port->agg_lock);
		hrtimer_cancel(&port->hrtimer);

reset_aggr_params() sets agg_state to 0, and the lock is released while
the old hrtimer is still queued.

Suppose CPU B creates a new aggregate in that window and reaches
schedule:. The old timer is still active, so this check fails:

	if (!hrtimer_active(&port->hrtimer) && port->agg_state != -EINPROGRESS) {

B then neither arms a timer nor sets agg_state. CPU A cancels the old
timer next, which leaves B's aggregate with no pending hrtimer and no
queued work. It stays buffered until the next egress packet on the port.

If the port is torn down first, rmnet_map_tx_aggregate_exit() only frees
skbagg_head when agg_state is -EINPROGRESS:

	if (port->agg_state == -EINPROGRESS) {
		if (port->skbagg_head) {
			dev_kfree_skb_any(port->skbagg_head);

In that case the stranded aggregate also leaks.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002143529.3217189-1-koen.vandeputte%40citymesh.com

Re: [PATCH net-next 2/4] net: rmnet: optimize rx deaggregation memory allocation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes the allocator change incorrectly.
- [Low] The commit message says netdev_alloc_skb()'s built-in NET_SKB_PAD
  is 'ensuring that the newly extracted IP headers are optimally cache-…
- [Low] In rmnet_map_deaggregate()
  (drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:397), `skbn->dev…

--

net: rmnet: optimize rx deaggregation memory allocation

This commit changes rmnet_map_deaggregate() to allocate the per-packet
skbs with netdev_alloc_skb() instead of alloc_skb(GFP_ATOMIC). The
commit message says this uses a lockless per-CPU NAPI cache, avoids slab
allocator contention, and leaves the IP headers cache-aligned through
NET_SKB_PAD.

> Replace alloc_skb() with netdev_alloc_skb() on the deaggregation path.
> netdev_alloc_skb() is specifically designed for the network receive hot
> path, utilizing a fast, lockless per-CPU NAPI cache. Furthermore, it

[Severity: Medium]

Is this an accurate description of netdev_alloc_skb()?

In __netdev_alloc_skb(), small heads and heads larger than a page still
use the same kmalloc path as before:

net/core/skbuff.c:__netdev_alloc_skb() {
    ...
	if (len <= SKB_WITH_OVERHEAD(SKB_SMALL_HEAD_CACHE_SIZE) ||
	    len > SKB_WITH_OVERHEAD(PAGE_SIZE) ||
	    (gfp_mask & (__GFP_DIRECT_RECLAIM | GFP_DMA))) {
		skb = __alloc_skb(len, gfp_mask, SKB_ALLOC_RX, NUMA_NO_NODE);
    ...
}

RMNET_MAX_PACKET_SIZE is 16384. On 4KB pages, packets above roughly
3.7KB take that branch, and so do small packets.

Only mid-size packets take the page_frag path. On that path only the
head comes from napi_alloc_cache.page, under local_bh_disable() and
local_lock_nested_bh(). The struct sk_buff still comes from the slab:

net/core/skbuff.c:__build_skb() {
    ...
	skb = kmem_cache_alloc(net_hotdata.skbuff_cache,
			       GFP_ATOMIC | __GFP_NOWARN);
    ...
}

The old alloc_skb(..., GFP_ATOMIC) path could already take the struct
sk_buff from the per-CPU NAPI skb cache when it had entries:

net/core/skbuff.c:__alloc_skb() {
    ...
	} else if (!in_hardirq() && !irqs_disabled()) {
		local_bh_disable();
		skb = napi_skb_cache_get(false);
		local_bh_enable();
	}
    ...
}

The page_frag path through __build_skb() never uses that cache.

Every packet still makes at least one slab allocation. So is it correct
to say this change "prevents slab allocator contention"? The description
seems to fit napi_alloc_skb() better than netdev_alloc_skb().

The commit message also says the change "significantly reduces memory
allocation overhead" and "improves overall throughput". Could some
measurements be added to support that?

> automatically provisions standard network padding (NET_SKB_PAD), ensuring
> that the newly extracted IP headers are optimally cache-aligned for
> efficient processing by the upper networking stack.

[Severity: Low]

Can the IP header actually end up cache-aligned here?

__netdev_alloc_skb() already does skb_reserve(skb, NET_SKB_PAD).
rmnet_map_deaggregate() then reserves its own headroom on top of that:

	skbn->dev = skb->dev;
	skb_reserve(skbn, RMNET_MAP_DEAGGR_HEADROOM);
	skb_put(skbn, packet_len);

Here RMNET_MAP_DEAGGR_HEADROOM is 32. After that,
__rmnet_map_ingress_handler() pulls the 4 byte MAP header:

		skb_pull(skb, sizeof(*map_header));

That puts the IP header at NET_SKB_PAD + 32 + 4 bytes from the head.
For an L1_CACHE_BYTES of 32, 64 or 128 that is 68, 100 or 164 bytes,
and none of these is cache line aligned. The old offset of 36 was not
aligned either.

The netdev_alloc_skb() kerneldoc says users should allocate the headroom
they need without counting the built in space. RMNET_MAP_DEAGGR_SPACING
and RMNET_MAP_DEAGGR_HEADROOM were left as they were, so each
deaggregated skb now carries NET_SKB_PAD extra bytes of headroom that
nothing uses.

Could the commit message be corrected, or the headroom handling changed
so that the alignment claim holds?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 2eafb1d969c1a..ccdfde3711db8 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -390,7 +390,7 @@ struct sk_buff *rmnet_map_deaggregate(struct sk_buff *skb,
>  	if (!packet_len)
>  		return NULL;
>  
> -	skbn = alloc_skb(packet_len + RMNET_MAP_DEAGGR_SPACING, GFP_ATOMIC);
> +	skbn = netdev_alloc_skb(skb->dev, packet_len + RMNET_MAP_DEAGGR_SPACING);
>  	if (!skbn)
>  		return NULL;
>  

[Severity: Low]

This isn't a bug, but the skbn->dev = skb->dev assignment that follows
in rmnet_map_deaggregate() is now redundant. __netdev_alloc_skb() sets
skb->dev on every success path, including the __alloc_skb() fallback:

skb_success:
	skb_reserve(skb, NET_SKB_PAD);
	skb->dev = dev;

Could that assignment be removed as part of this conversion?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002143529.3217189-1-koen.vandeputte%40citymesh.com

Re: [PATCH net-next 3/4] net: rmnet: conditionally expand skb headroom in ingress handler

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch drops the implicit unclone that the old code did for
  every ARPHRD_ETHER skb.
- [Medium] On architectures where NET_IP_ALIGN is 2, removing the per-
  packet pskb_expand_head() leaves the inner IP header 2 bytes off…
- [Low] The commit message misdescribes what the patch changes.

--

net: rmnet: conditionally expand skb headroom in ingress handler

This patch changes rmnet_map_ingress_handler() so that pskb_expand_head()
is called only when skb_headroom() is less than ETH_HLEN. Before, it was
called for every ARPHRD_ETHER skb. The goal is to avoid a reallocation and
copy on the RX hot path when the lower driver already provides enough
headroom.

> In many modern architectures, the underlying physical driver (e.g., USB)
> can be configured to pre-allocate this extra ETH_HLEN headroom when
> minting the initial SKB.

[ ... ]

> The expensive pskb_expand_head() operation is now strictly a
> fallback, allowing properly configured hardware to achieve zero-copy MAC
> header insertion.

[Severity: Low]
Is this description accurate? For an ARPHRD_ETHER lower device, the lower
driver has already called eth_type_trans() before rmnet_rx_handler() runs:

net/ethernet/eth.c:eth_type_trans() {
    ...
	eth = eth_skb_pull_mac(skb);
    ...
}

That pulls ETH_HLEN bytes, and those bytes are really the start of the
MAP frame. The skb_push(skb, ETH_HLEN) in rmnet_map_ingress_handler()
just gives them back, so skb->data points at the MAP header again. No MAC
header is inserted.

Because those bytes were already pulled, skb_headroom(skb) >= ETH_HLEN
always holds at this point. That means the new branch can't be taken,
which matches the test result in the commit message:

  "the expansion is not triggered as enough space is already available"

If headroom ever were below ETH_HLEN, expanding and then pushing would
leave 14 uninitialized bytes where the MAP header is expected. So the
fallback wouldn't give a usable frame either.

In practice, does this patch remove the pskb_expand_head() call for every
ARPHRD_ETHER skb, whatever the lower driver's configuration?

If so, could the commit message say that directly? It would also help to
describe what else the call was doing: it made the skb data private and
shifted the payload by ETH_HLEN. Commit ec405641e2b7 ("net: qualcomm:
rmnet: Fix incorrect allocation flag in receive path") describes the call
as needed only when headroom is insufficient.

The "severely degrading throughput" claim also has no numbers. Could some
measurements be added?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index aa5523f4618eb..95c3e3934fd3c 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -114,9 +114,11 @@ rmnet_map_ingress_handler(struct sk_buff *skb,
>  	u32 data_format;
>  
>  	if (skb->dev->type == ARPHRD_ETHER) {
> -		if (pskb_expand_head(skb, ETH_HLEN, 0, GFP_ATOMIC)) {
> -			kfree_skb(skb);
> -			return;
> +		if (skb_headroom(skb) < ETH_HLEN) {
> +			if (pskb_expand_head(skb, ETH_HLEN, 0, GFP_ATOMIC)) {
> +				kfree_skb(skb);
> +				return;
> +			}
>  		}

[Severity: Medium]
Can this let rmnet_map_send_ack() write into a data buffer that another
skb also uses?

Before this patch, pskb_expand_head() ran for every ARPHRD_ETHER skb. As
a side effect it gave the skb a private head (skb->cloned = 0,
dataref = 1). Headroom is now always >= ETH_HLEN here, so that no longer
happens.

Nothing earlier in rmnet_rx_handler() unclones the skb either.
skb_linearize() does nothing for a linear skb, and the pskb_may_pull()
that replaces it later in the series doesn't unclone.

The skb can arrive cloned when a ptype_all tap or a tc mirred mirror is
active on the lower device:

__netif_receive_skb_core()
  deliver_skb()
    packet_rcv()
      skb_clone(), clone queued to the socket
  rmnet_rx_handler()
    rmnet_map_ingress_handler()
      __rmnet_map_ingress_handler()
        rmnet_map_command()
          rmnet_map_send_ack()

With RMNET_FLAGS_INGRESS_DEAGGREGATION clear and
RMNET_FLAGS_INGRESS_MAP_COMMANDS set, an acknowledged FLOW_ENABLE or
FLOW_DISABLE command ends up here:

rmnet_map_command.c:rmnet_map_send_ack() {
    ...
	cmd = (struct rmnet_map_control_command *)(map_header + 1);
	cmd->cmd_type = type & 0x03;

	netif_tx_lock(dev);
	dev->netdev_ops->ndo_start_xmit(skb, dev);
    ...
}

This write changes the packet already queued to the AF_PACKET socket,
after the socket filter has accepted it. It can also race with a
recvmsg() copying the same buffer on another CPU.

Raw-IP lower devices never had the implicit unclone, but for ARPHRD_ETHER
this looks new.

Would skb_cow_head(skb, ETH_HLEN) work here instead? It reallocates only
when headroom is short or the header is cloned, so the fast path stays.
Another option is for rmnet_map_send_ack() to make the skb writable, for
example with skb_ensure_writable() or skb_unclone(), before the write.

>  
>  		skb_push(skb, ETH_HLEN);

[Severity: Medium]
Could skipping the copy leave the inner IP header misaligned on
architectures where NET_IP_ALIGN is 2?

The generic NET_IP_ALIGN in include/linux/skbuff.h is 2. Only x86, arm64
and powerpc set it to 0.

A lower Ethernet driver that uses NET_IP_ALIGN lays out its RX buffer for
a 14 byte Ethernet header before L3. Here, though, the frame starts with a
4 byte MAP header, plus 4 more bytes for a MAPv5 csum header.

With NET_SKB_PAD of 64 and NET_IP_ALIGN of 2:

  before: pskb_expand_head() copied into a fresh head shifted by ETH_HLEN
          MAP header at head+80, IP header at head+84 (4-byte aligned)

  after:  frame is parsed in place
          MAP header at head+66, IP header at head+70 or +74 (2 mod 4)

In non-deaggregation mode, the checksum code then does word loads on that
header:

rmnet_map_data.c:rmnet_map_ipv4_dl_csum_trailer() {
    ...
	if (ip_fast_csum(ip4h, ip4h->ihl)) {
    ...
}

The skb then goes to the IP stack. On architectures without
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, the stack expects an aligned
iphdr.

On strict-alignment CPUs such as MIPS, wouldn't these loads trap and be
emulated on every packet? That would make this change a slowdown rather
than a speedup.

The deaggregation path is not affected, because rmnet_map_deaggregate()
copies each packet into a new skb.

Which architecture and ingress data format was this tested with?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002143529.3217189-1-koen.vandeputte%40citymesh.com

Re: [PATCH net-next 4/4] net: rmnet: optimize rx handler by replacing skb_linearize with pskb_may_pull

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 1 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] Regression in rmnet_rx_handler()
  (drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:220): the patch…
- [Medium] The commit message misstates what the RX path needs and what
  the change does, and it hides that the patch undoes a crash fix.
- [Medium] Shared skb data modified without copy-on-write for non-linear
  cloned MAP command packets.

--

net: rmnet: optimize rx handler by replacing skb_linearize with pskb_may_pull

This patch replaces the unconditional skb_linearize() in rmnet_rx_handler()
with pskb_may_pull() of sizeof(struct rmnet_map_header). After the change,
only the 4-byte MAP header is guaranteed to be in the linear area. The goal
is to avoid copying large aggregated frames on the RX path.

> When receiving large, hardware-aggregated frames
> (e.g., 16KB or 32KB from USB modems via qmi_wwan),
> this forces the network stack to allocate a massive
> contiguous buffer and memcpy the entire scattered payload
> out of paged memory.

[ ... ]

> On a slower embedded board, this improves throughput by ~10%.

[Severity: Medium]
Can the throughput gain come from this change in the qmi_wwan setup?
skb_linearize() is:

	return skb_is_nonlinear(skb) ? __skb_linearize(skb) : 0;

That makes it a no-op for linear skbs. The usbnet rx_submit() path used by
qmi_wwan allocates linear skbs with __netdev_alloc_skb().

The only skbs this change affects are non-linear ones. Those are the skbs
that then reach the linear-only parsers described below.

> This severe performance bottleneck is completely unnecessary
> because the RX handler only needs to inspect the initial
> 4-byte MAP header to determine the logical endpoint
> and route the packet.

Is this description accurate? The same RX path goes on into
rmnet_map_deaggregate(), rmnet_map_validate_packet_len(),
rmnet_map_process_next_hdr_packet(), rmnet_map_checksum_downlink_packet()
and rmnet_map_command(). All of them access the full frame through
skb->data.

> This preserves the zero-copy architecture for aggregated frames,
> drastically reducing CPU overhead, cache thrashing,
> and memory allocations on the RX hotpath.

The aggregated path doesn't look zero-copy. rmnet_map_deaggregate()
allocates a new skb for each record and copies into it:

drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:rmnet_map_deaggregate() {
    ...
	skbn = netdev_alloc_skb(skb->dev, packet_len + RMNET_MAP_DEAGGR_SPACING);
    ...
	memcpy(skbn->data, skb->data, packet_len);
	skb_pull(skb, packet_len);
    ...
}

The commit message also leaves out commit d698e6a00a60 ("net: qualcomm:
rmnet: Fix rx_handler for non-linear skbs"). That commit added this
skb_linearize() to fix crashes with non-linear skbs. Should it be
mentioned here?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index 95c3e3934fd3c..a2379df69a6d0 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> @@ -217,7 +217,7 @@ rx_handler_result_t rmnet_rx_handler(struct sk_buff **pskb)
>  	if (!skb)
>  		goto done;
>  
> -	if (skb_linearize(skb)) {
> +	if (!pskb_may_pull(skb, sizeof(struct rmnet_map_header))) {

[Severity: Critical]
Does this bring back the crash fixed by commit d698e6a00a60 ("net:
qualcomm: rmnet: Fix rx_handler for non-linear skbs")? That commit says:

  There is no guarantee that rmnet rx_handler is only fed with linear
  skbs, but current rmnet implementation does not check that, leading
  to crash in case of non linear skbs processed as linear ones.

Now only 4 bytes are guaranteed to be linear. The downstream parsers still
treat skb->data as covering the whole frame. They check lengths only
against skb->len, which includes paged and frag_list data.

Non-linear input does happen in-tree. For example, mhi_net_skb_agg() in
drivers/net/mhi_net.c builds frag_list skbs that end up in rmnet.

With RMNET_FLAGS_INGRESS_DEAGGREGATION:

rmnet_rx_handler()
  rmnet_map_ingress_handler()
    rmnet_map_deaggregate()
      rmnet_map_validate_packet_len()
          if (skb->len < packet_len)
              return 0;
      memcpy(skbn->data, skb->data, packet_len);
      skb_pull(skb, packet_len);

Won't the memcpy() read past skb->tail into tailroom and skb_shared_info?
That memory would then be copied into the packet delivered to the stack.

When packet_len > skb_headlen(skb), __skb_pull() would also hit:

	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();

The MAPv5 next_hdr at data + sizeof(*maph) in
rmnet_map_validate_packet_len() is also read without a pull.

Without deaggregation, __rmnet_map_ingress_handler() has similar problems:

- MAPv5: rmnet_map_process_next_hdr_packet() reads next_hdr at
  skb->data + sizeof(struct rmnet_map_header). Then 4 + 4 bytes are
  pulled in total, so BUG() fires when the linear head is shorter than
  8 bytes.

- MAPv4: rmnet_set_skb_proto() reads skb->data[0] after the MAP header
  has been pulled. rmnet_map_checksum_downlink_packet() dereferences
  skb->data + len, and len comes from the device-supplied pkt_len. The
  IPv4/IPv6 helpers also read IP and L4 headers at raw skb->data offsets.
  Could the CHECKSUM_UNNECESSARY verdict end up computed from
  out-of-bounds memory?

- skb_trim(skb, len) goes through __skb_trim()->__skb_set_length(). That
  does WARN_ON(skb_is_nonlinear(skb)) and returns without trimming, so MAP
  padding and the checksum trailer would reach the IP stack.

With RMNET_FLAGS_INGRESS_MAP_COMMANDS, rmnet_map_command() reads
cmd->command_name at map_header + 1, and rmnet_map_send_ack() writes:

	cmd = (struct rmnet_map_control_command *)(map_header + 1);
	cmd->cmd_type = type & 0x03;

Is this an out-of-bounds write when only the MAP header is linear? The
skb_trim() in rmnet_map_send_ack() would also hit the nonlinear WARN.

The aggregation section of
Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst also
says the MAP packets are "delivered to rmnet in a single linear skb".

Would it be better to keep the linearization? The alternative is to make
every parser frag-aware:

- per-record pskb_may_pull() or skb_copy_bits(), or clone plus
  pskb_trim(), in deaggregation
- pulls for the csum header, the trailer and the command
- pskb_trim() instead of skb_trim()

A single 4-byte pull at the top doesn't seem to be enough.

[Severity: Medium]
Can this leave the head of a cloned non-linear skb shared when rmnet later
writes to it?

If a packet tap is active on the real device, packet_rcv() in
net/packet/af_packet.c sees skb_shared() and queues a skb_clone(). The skb
that rmnet_rx_handler() then receives is skb_cloned().

Before this patch, skb_linearize() on a non-linear skb went through
__pskb_pull_tail(), which does:

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))

That gave rmnet a private head. pskb_may_pull(skb, 4) returns early when
the MAP header is already linear, so the head stays shared.

With RMNET_FLAGS_INGRESS_MAP_COMMANDS and without deaggregation,
rmnet_map_command()->rmnet_map_send_ack() then does:

	cmd->cmd_type = type & 0x03;

There is no skb_cow() or skb_ensure_writable() before this write. Would
the packet socket reader then see the captured command request changed
into an ACK? The netif_tx_lock() taken afterwards doesn't serialize
against that reader.

The same write already happened for linear cloned skbs before this patch.
The non-linear cloned case is what this patch adds.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002143529.3217189-1-koen.vandeputte%40citymesh.com