[PATCH net-next 0/7] Add HW GRO handling in rmnet

[PATCH net-next 0/7] Add HW GRO handling in rmnet

[Subash Abhinov Kasiviswanathan]

This patch series adds support for processing coalesced packets
in rmnet driver.

Hardware can coalesce TCP and UDP flows up to a maximum size of 65k.
Up to 48 total packets can be coalesced into a single frame and
are limited to 6 different consecutive packet sizes. As kernel
supports a single packet size (gso_size), the coalescing frame
will need to be broken down and queued per size to the network stack.

Hardware can also continue to coalesce in case the transport checksum
was invalid so such segments will need to discarded in the driver.
The exact segments which have bad checksum are identified through a
bitmap in the coalescing packet header.

Patch 1 adds the UAPI flag for the coalescing packet format.
Patch 2 is in preparation for the coalescing format packet handling
where packets maybe delivered as a list to the network stack as
mentioned above in case of different sizes or checksum errors.
Patch 3 is in preparation for the coalescing format flags to ensure
that there is no invalid MAP format combination to process the packets.
Patch 4 adds the core coalescing handling.
Patch 5 is for handling some quirks observed in the coalescing
hardware.
Patch 6 adds coalescing statistics in ethtool.
Patch 7 adds documentation about the coalescing feature.

Subash Abhinov Kasiviswanathan (7):
  uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE
  net: qualcomm: rmnet: Process MAPv5 frames as a list
  net: qualcomm: rmnet: Restrict supported MAP checksum configurations
  net: qualcomm: rmnet: Add DL packet coalescing support
  net: qualcomm: rmnet: Work around coalescing hardware quirks
  net: qualcomm: rmnet: Add ethtool stats for DL coalescing
  docs: networking: Add documentation for the coalescing support in
    rmnet

 .../cellular/qualcomm/rmnet.rst               | 115 ++-
 .../ethernet/qualcomm/rmnet/rmnet_config.c    |  70 +-
 .../ethernet/qualcomm/rmnet/rmnet_config.h    |  20 +
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |  25 +-
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  20 +-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 715 +++++++++++++++++-
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  21 +
 include/linux/if_rmnet.h                      |  31 +
 include/uapi/linux/if_link.h                  |   1 +
 9 files changed, 958 insertions(+), 60 deletions(-)

-- 
2.34.1

[PATCH net-next 1/7] uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE

[Subash Abhinov Kasiviswanathan]

Add bit 6 of the rmnet ingress flags to allow rmnet to process incoming
packets in the MAPv5 coalescing header format.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 include/uapi/linux/if_link.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 245b36204525..349fcddc6622 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -2022,6 +2022,7 @@ enum {
 #define RMNET_FLAGS_EGRESS_MAP_CKSUMV4            (1U << 3)
 #define RMNET_FLAGS_INGRESS_MAP_CKSUMV5           (1U << 4)
 #define RMNET_FLAGS_EGRESS_MAP_CKSUMV5            (1U << 5)
+#define RMNET_FLAGS_INGRESS_COALESCE              (1U << 6)
 
 enum {
 	IFLA_RMNET_UNSPEC,
-- 
2.34.1

Re: [PATCH net-next 1/7] uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Retained for independent validation only; my baseline decision is
  FALSE.

--

uapi: if_link: Add RMNET_FLAGS_INGRESS_COALESCE

This commit adds bit 6 to the rmnet ingress flags in the uAPI header. The
bit lets rmnet process incoming packets in the MAPv5 coalescing header
format.

> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 245b362045253..349fcddc6622f 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -2022,6 +2022,7 @@ enum {
>  #define RMNET_FLAGS_EGRESS_MAP_CKSUMV4            (1U << 3)
>  #define RMNET_FLAGS_INGRESS_MAP_CKSUMV5           (1U << 4)
>  #define RMNET_FLAGS_EGRESS_MAP_CKSUMV5            (1U << 5)
> +#define RMNET_FLAGS_INGRESS_COALESCE              (1U << 6)

[Severity: Low]
This is a minor point about bisection. Nothing in the kernel reads the new
bit at this commit, so it is accepted without any effect.

rmnet_newlink() and rmnet_changelink() in
drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c merge the requested
flags without checking them:

rmnet_changelink() {
    ...
		data_format = old_data_format & ~flags->mask;
		data_format |= flags->flags & flags->mask;
		WRITE_ONCE(port->data_format, data_format);
    ...
}

rmnet_fill_info() then reports port->data_format back. The bit shows as
enabled, but __rmnet_map_ingress_handler() and
rmnet_map_process_next_hdr_packet() never check it at this point.

Later in the series, "net: qualcomm: rmnet: Add DL packet coalescing
support" adds the code that reads this bit in the ingress path and in
rmnet_map_data.c. It also adds rmnet_config_data_format_valid(), which
rejects RMNET_FLAGS_INGRESS_COALESCE when RMNET_FLAGS_INGRESS_MAP_CKSUMV5
is not set. So the gap only exists in the middle of the series.

Would it make sense to fold this define into that patch? The bit would
then never be accepted by a kernel that ignores it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930051345.857443-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next 2/7] net: qualcomm: rmnet: Process MAPv5 frames as a list

[Subash Abhinov Kasiviswanathan]

Add support for handling the MAP packets as a list in preparation for
the subsequent patch in the series  which adds support for handling the
coalescing packets. This is needed as a single coalescing MAP packet could
yield multiple IP packets.

There is no functional change in the handling of the previously
supported MAP packet formats.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  | 22 +++++--
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  4 +-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 57 ++++++++++++-------
 3 files changed, 58 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index aa5523f4618e..ab1dfbd833e3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -50,6 +50,16 @@ rmnet_deliver_skb(struct sk_buff *skb)
 	gro_cells_receive(&priv->gro_cells, skb);
 }
 
+static void rmnet_deliver_skb_list(struct sk_buff_head *head)
+{
+	struct sk_buff *skb;
+
+	while ((skb = __skb_dequeue(head))) {
+		rmnet_set_skb_proto(skb);
+		rmnet_deliver_skb(skb);
+	}
+}
+
 /* MAP handler */
 
 static void
@@ -59,6 +69,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 {
 	struct rmnet_map_header *map_header = (void *)skb->data;
 	struct rmnet_endpoint *ep;
+	struct sk_buff_head list;
 	u16 len, pad;
 	u8 mux_id;
 
@@ -83,12 +94,12 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 
 	skb->dev = ep->egress_dev;
 
+	__skb_queue_head_init(&list);
+
 	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
 	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
-		if (rmnet_map_process_next_hdr_packet(skb, len))
+		if (rmnet_map_process_next_hdr_packet(skb, &list, len))
 			goto free_skb;
-		skb_pull(skb, sizeof(*map_header));
-		rmnet_set_skb_proto(skb);
 	} else {
 		/* Subtract MAP header */
 		skb_pull(skb, sizeof(*map_header));
@@ -96,10 +107,11 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 		if (data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4 &&
 		    !rmnet_map_checksum_downlink_packet(skb, len + pad))
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		skb_trim(skb, len);
+		__skb_queue_tail(&list, skb);
 	}
 
-	skb_trim(skb, len);
-	rmnet_deliver_skb(skb);
+	rmnet_deliver_skb_list(&list);
 	return;
 
 free_skb:
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index 0977e495f591..ef738ce015d4 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -53,7 +53,9 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 				      struct rmnet_port *port,
 				      struct net_device *orig_dev,
 				      int csum_type);
-int rmnet_map_process_next_hdr_packet(struct sk_buff *skb, u16 len);
+int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
+				      struct sk_buff_head *list,
+				      u16 len);
 unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
 				    struct net_device *orig_dev);
 void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 39d6d084e73f..577f2758e385 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -497,35 +497,54 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 	}
 }
 
-/* Process a MAPv5 packet header */
+static struct rmnet_map_v5_csum_header *
+rmnet_map_get_next_hdr(struct sk_buff *skb)
+{
+	return (struct rmnet_map_v5_csum_header *)(skb->data +
+						   sizeof(struct rmnet_map_header));
+}
+
+static u8 rmnet_map_get_next_hdr_type(struct sk_buff *skb)
+{
+	struct rmnet_map_v5_csum_header *hdr = rmnet_map_get_next_hdr(skb);
+
+	return u8_get_bits(hdr->header_info, MAPV5_HDRINFO_HDR_TYPE_FMASK);
+}
+
+static bool rmnet_map_get_csum_valid(struct sk_buff *skb)
+{
+	struct rmnet_map_v5_csum_header *hdr = rmnet_map_get_next_hdr(skb);
+
+	return !!(hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG);
+}
+
 int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
+				      struct sk_buff_head *list,
 				      u16 len)
 {
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
-	struct rmnet_map_v5_csum_header *next_hdr;
-	u8 nexthdr_type;
 
-	next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data +
-			sizeof(struct rmnet_map_header));
+	switch (rmnet_map_get_next_hdr_type(skb)) {
+	case RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD:
+		if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
+			priv->stats.csum_sw++;
+		} else if (rmnet_map_get_csum_valid(skb)) {
+			priv->stats.csum_ok++;
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		} else {
+			priv->stats.csum_valid_unset++;
+		}
 
-	nexthdr_type = u8_get_bits(next_hdr->header_info,
-				   MAPV5_HDRINFO_HDR_TYPE_FMASK);
+		skb_pull(skb, sizeof(struct rmnet_map_header) +
+			      sizeof(struct rmnet_map_v5_csum_header));
+		skb_trim(skb, len);
+		__skb_queue_tail(list, skb);
+		break;
 
-	if (nexthdr_type != RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
+	default:
 		return -EINVAL;
-
-	if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
-		priv->stats.csum_sw++;
-	} else if (next_hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG) {
-		priv->stats.csum_ok++;
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
-	} else {
-		priv->stats.csum_valid_unset++;
 	}
 
-	/* Pull csum v5 header */
-	skb_pull(skb, sizeof(*next_hdr));
-
 	return 0;
 }
 
-- 
2.34.1

[PATCH net-next 3/7] net: qualcomm: rmnet: Restrict supported MAP checksum configurations

[Subash Abhinov Kasiviswanathan]

RmNet only supports three MAP data format configurations: MAPv1
(no checksum offload), MAPv4 (v4 checksum offload) and MAPv5 (v5
checksum offload). QMAP command support is orthogonal and may be
combined with any of the three. Mixing the v4 and v5 checksum
offload flags together is not a valid configuration.

Validate the requested data format in both rmnet_newlink() and
rmnet_changelink() and reject any combination that sets both the
v4 and v5 checksum offload flags at the same time. This is in
preparation for the next patch where coalescing support needs to be
allowed with MAPv5 format only.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 .../ethernet/qualcomm/rmnet/rmnet_config.c    | 62 ++++++++++++++-----
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 61b04c6c0390..8051aef01ae3 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -26,6 +26,22 @@ static int rmnet_is_real_dev_registered(const struct net_device *real_dev)
 	return rcu_access_pointer(real_dev->rx_handler) == rmnet_rx_handler;
 }
 
+/* Only three MAP configurations are supported: MAPv1 (no checksum
+ * offload), MAPv4 (v4 checksum offload) and MAPv5 (v5 checksum
+ * offload). QMAP command support is orthogonal and permitted with
+ * any of the three. Mixing v4 and v5 checksum offload flags together
+ * is not a supported configuration.
+ */
+static bool rmnet_config_data_format_valid(u32 data_format)
+{
+	u32 v4_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV4 |
+		      RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
+	u32 v5_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV5 |
+		      RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
+
+	return !(data_format & v4_mask) || !(data_format & v5_mask);
+}
+
 /* Needs rtnl lock */
 struct rmnet_port*
 rmnet_get_port_rtnl(const struct net_device *real_dev)
@@ -143,6 +159,20 @@ static int rmnet_newlink(struct net_device *dev,
 		return -ENODEV;
 	}
 
+	if (data[IFLA_RMNET_FLAGS]) {
+		struct ifla_rmnet_flags *flags;
+
+		flags = nla_data(data[IFLA_RMNET_FLAGS]);
+		data_format &= ~flags->mask;
+		data_format |= flags->flags & flags->mask;
+	}
+
+	if (!rmnet_config_data_format_valid(data_format)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "unsupported MAP checksum flag combination");
+		return -EINVAL;
+	}
+
 	ep = kzalloc_obj(*ep);
 	if (!ep)
 		return -ENOMEM;
@@ -167,14 +197,6 @@ static int rmnet_newlink(struct net_device *dev,
 
 	hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]);
 
-	if (data[IFLA_RMNET_FLAGS]) {
-		struct ifla_rmnet_flags *flags;
-
-		flags = nla_data(data[IFLA_RMNET_FLAGS]);
-		data_format &= ~flags->mask;
-		data_format |= flags->flags & flags->mask;
-	}
-
 	netdev_dbg(dev, "data format [0x%08X]\n", data_format);
 	WRITE_ONCE(port->data_format, data_format);
 
@@ -301,8 +323,11 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 			    struct netlink_ext_ack *extack)
 {
 	struct rmnet_priv *priv = netdev_priv(dev);
+	struct ifla_rmnet_flags *flags;
 	struct net_device *real_dev;
 	struct rmnet_port *port;
+	u32 old_data_format;
+	u32 data_format;
 	u16 mux_id;
 
 	if (!dev)
@@ -320,6 +345,19 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 
 	port = rmnet_get_port_rtnl(real_dev);
 
+	if (data[IFLA_RMNET_FLAGS]) {
+		old_data_format = READ_ONCE(port->data_format);
+		flags = nla_data(data[IFLA_RMNET_FLAGS]);
+		data_format = old_data_format & ~flags->mask;
+		data_format |= flags->flags & flags->mask;
+
+		if (!rmnet_config_data_format_valid(data_format)) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "unsupported MAP checksum flag combination");
+			return -EINVAL;
+		}
+	}
+
 	if (data[IFLA_RMNET_MUX_ID]) {
 		mux_id = nla_get_u16(data[IFLA_RMNET_MUX_ID]);
 
@@ -346,14 +384,6 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
 	}
 
 	if (data[IFLA_RMNET_FLAGS]) {
-		struct ifla_rmnet_flags *flags;
-		u32 old_data_format;
-		u32 data_format;
-
-		old_data_format = port->data_format;
-		flags = nla_data(data[IFLA_RMNET_FLAGS]);
-		data_format = old_data_format & ~flags->mask;
-		data_format |= flags->flags & flags->mask;
 		WRITE_ONCE(port->data_format, data_format);
 
 		if (rmnet_vnd_update_dev_mtu(port, real_dev)) {
-- 
2.34.1

Re: [PATCH net-next 3/7] net: qualcomm: rmnet: Restrict supported MAP checksum configurations

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · 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).

- [Medium] rmnet_config_data_format_valid() in
  drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c puts the ingress…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  rmnet_newlink() in drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c…

--

net: qualcomm: rmnet: Restrict supported MAP checksum configurations

This patch adds rmnet_config_data_format_valid() and calls it from
rmnet_newlink() and rmnet_changelink(). A requested data format that has
both v4 and v5 checksum offload flags set is now rejected with -EINVAL.
The commit message says this prepares for a follow-up patch that allows
coalescing only with MAPv5.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index 61b04c6c0390e..8051aef01ae31 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> @@ -26,6 +26,22 @@ static int rmnet_is_real_dev_registered(const struct net_device *real_dev)
>  	return rcu_access_pointer(real_dev->rx_handler) == rmnet_rx_handler;
>  }
>
> +/* Only three MAP configurations are supported: MAPv1 (no checksum
> + * offload), MAPv4 (v4 checksum offload) and MAPv5 (v5 checksum
> + * offload). QMAP command support is orthogonal and permitted with
> + * any of the three. Mixing v4 and v5 checksum offload flags together
> + * is not a supported configuration.
> + */
> +static bool rmnet_config_data_format_valid(u32 data_format)
> +{
> +	u32 v4_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV4 |
> +		      RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> +	u32 v5_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV5 |
> +		      RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> +
> +	return !(data_format & v4_mask) || !(data_format & v5_mask);
> +}

[Severity: Medium]
Does this reject more than the commit needs? Each mask covers both the
ingress and egress bits. That means cross-direction setups are refused
along with same-direction mixes, for example:

    RMNET_FLAGS_INGRESS_MAP_CKSUMV5 | RMNET_FLAGS_EGRESS_MAP_CKSUMV4
    RMNET_FLAGS_INGRESS_MAP_CKSUMV4 | RMNET_FLAGS_EGRESS_MAP_CKSUMV5

Before this patch those were accepted. The datapath reads only the bits
for its own direction.

On egress, rmnet_map_egress_handler() only checks the EGRESS_* bits:

    data_format = READ_ONCE(port->data_format);
    if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4) {
        ...
    } else if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
        ...
    }

rmnet_map_add_map_header() and rmnet_vnd_headroom() also check only the
EGRESS_* bits.

On ingress, __rmnet_map_ingress_handler(), rmnet_map_validate_packet_len()
and rmnet_map_send_ack() check only the INGRESS_* bits.

After this patch, RTM_NEWLINK through rmnet_newlink() and RTM_SETLINK
through rmnet_changelink() return -EINVAL for these configurations. Could
this break existing userspace that negotiates the UL and DL formats
separately?

Coalescing is ingress-only. The follow-up patch checks it on its own with
(RMNET_FLAGS_INGRESS_COALESCE && !RMNET_FLAGS_INGRESS_MAP_CKSUMV5). Would
it be enough to reject only same-direction mixes here, i.e. INGRESS v4
with INGRESS v5, and EGRESS v4 with EGRESS v5?

The commit message says:

    Mixing the v4 and v5 checksum offload flags together is not a valid
    configuration.

It does not say that cross-direction combinations were accepted and
worked before this change. Could the message mention that, if the
restriction is intentional?

[ ... ]

> @@ -167,14 +197,6 @@ static int rmnet_newlink(struct net_device *dev,
>
>  	hlist_add_head_rcu(&ep->hlnode, &port->muxed_ep[mux_id]);
>
> -	if (data[IFLA_RMNET_FLAGS]) {
> -		struct ifla_rmnet_flags *flags;
> -
> -		flags = nla_data(data[IFLA_RMNET_FLAGS]);
> -		data_format &= ~flags->mask;
> -		data_format |= flags->flags & flags->mask;
> -	}
> -
>  	netdev_dbg(dev, "data format [0x%08X]\n", data_format);
>  	WRITE_ONCE(port->data_format, data_format);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Now that
data_format is computed before rmnet_vnd_newlink(), it may be worth
looking at.

rmnet_newlink() calls rmnet_vnd_newlink() before port->data_format is
written. rmnet_vnd_newlink() sizes the new device's MTU from the old port
format:

drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c:rmnet_vnd_newlink() {
    ...
	headroom = rmnet_vnd_headroom(port);

	if (rmnet_vnd_change_mtu(rmnet_dev, real_dev->mtu - headroom)) {
    ...
}

rmnet_vnd_headroom() reads port->data_format &
RMNET_FLAGS_EGRESS_MAP_CKSUMV4.

rmnet_register_real_device() creates a fresh port with data_format == 0.
If the first newlink on that port requests EGRESS_MAP_CKSUMV4, the MTU
ends up sizeof(struct rmnet_map_ul_csum_header) too large. Can uplink
frames then go over the real device's MTU?

On a shared port, the WRITE_ONCE() also changes the framing for every
existing rmnet device on that port. Their MTUs are not re-checked the way
rmnet_changelink() does with rmnet_vnd_update_dev_mtu(). Should the new
format be applied, or at least checked against the MTUs, before
rmnet_vnd_newlink() runs?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930051345.857443-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next 4/7] net: qualcomm: rmnet: Add DL packet coalescing support

[Subash Abhinov Kasiviswanathan]

Hardware can coalesce multiple IP packets of a flow into a single MAP
frame carrying a MAPv5 coalescing header and a set of Number Length
Objects (NLOs) that describe the count and length of the individual
segments within. The MAP coalescing header encapsulates a single IP
packet of type IPv4 and IPv6 for protocols TCP and UDP. Up to 48 IP
packets can be coalesced into a single coalescing packet.

This means that hardware can coalesce up to 6 different sizes within a
single IP packet. Additionally, hardware can continue to coalesce
packets even though it may have detected an issue with the checksum. The
checksum bitmap will give information on the exact segments which
happened to encounter a bad checksum. In these cases, the packets need
to be segmented out from the whole IP packet and only the packets with
same size need to be queued to the stack for valid checksum packets
while invalid checksum packets are dropped in the driver.

Any packet queued to the network stack would have the gso information
populated in the skb and partial checksum set. If a packet is segmented,
the IP length and checksum are also updated accordingly.

DL packet coalescing relies on the MAPv5 checksum header being present
on ingress, so reject any configuration which requests coalescing
without also enabling MAPv5 ingress checksum offload.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 .../ethernet/qualcomm/rmnet/rmnet_config.c    |  12 +-
 .../ethernet/qualcomm/rmnet/rmnet_handlers.c  |   5 +-
 .../net/ethernet/qualcomm/rmnet/rmnet_map.h   |  18 +-
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 512 +++++++++++++++++-
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |   1 +
 include/linux/if_rmnet.h                      |  31 ++
 6 files changed, 563 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
index 8051aef01ae3..555659c2edc2 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
@@ -30,7 +30,8 @@ static int rmnet_is_real_dev_registered(const struct net_device *real_dev)
  * offload), MAPv4 (v4 checksum offload) and MAPv5 (v5 checksum
  * offload). QMAP command support is orthogonal and permitted with
  * any of the three. Mixing v4 and v5 checksum offload flags together
- * is not a supported configuration.
+ * is not a supported configuration. DL packet coalescing additionally
+ * requires a MAPv5 configuration.
  */
 static bool rmnet_config_data_format_valid(u32 data_format)
 {
@@ -39,7 +40,14 @@ static bool rmnet_config_data_format_valid(u32 data_format)
 	u32 v5_mask = RMNET_FLAGS_INGRESS_MAP_CKSUMV5 |
 		      RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
 
-	return !(data_format & v4_mask) || !(data_format & v5_mask);
+	if ((data_format & v4_mask) && (data_format & v5_mask))
+		return false;
+
+	if ((data_format & RMNET_FLAGS_INGRESS_COALESCE) &&
+	    !(data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5))
+		return false;
+
+	return true;
 }
 
 /* Needs rtnl lock */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
index ab1dfbd833e3..bb867ceb01c1 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
@@ -96,9 +96,10 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
 
 	__skb_queue_head_init(&list);
 
-	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
+	if ((data_format &
+	     (RMNET_FLAGS_INGRESS_MAP_CKSUMV5 | RMNET_FLAGS_INGRESS_COALESCE)) &&
 	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
-		if (rmnet_map_process_next_hdr_packet(skb, &list, len))
+		if (rmnet_map_process_next_hdr_packet(skb, &list, len, data_format))
 			goto free_skb;
 	} else {
 		/* Subtract MAP header */
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
index ef738ce015d4..77ddac6023c0 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map.h
@@ -6,6 +6,22 @@
 #define _RMNET_MAP_H_
 #include <linux/if_rmnet.h>
 
+enum rmnet_map_v5_close_type {
+	RMNET_MAP_COAL_CLOSE_NON_COAL,
+	RMNET_MAP_COAL_CLOSE_IP_MISS,
+	RMNET_MAP_COAL_CLOSE_TRANS_MISS,
+	RMNET_MAP_COAL_CLOSE_HW,
+	RMNET_MAP_COAL_CLOSE_COAL,
+};
+
+enum rmnet_map_v5_close_value {
+	RMNET_MAP_COAL_CLOSE_HW_NL,
+	RMNET_MAP_COAL_CLOSE_HW_PKT,
+	RMNET_MAP_COAL_CLOSE_HW_BYTE,
+	RMNET_MAP_COAL_CLOSE_HW_TIME,
+	RMNET_MAP_COAL_CLOSE_HW_EVICT,
+};
+
 struct rmnet_map_control_command {
 	u8  command_name;
 	u8  cmd_type:2;
@@ -55,7 +71,7 @@ void rmnet_map_checksum_uplink_packet(struct sk_buff *skb,
 				      int csum_type);
 int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 				      struct sk_buff_head *list,
-				      u16 len);
+				      u16 len, u32 data_format);
 unsigned int rmnet_map_tx_aggregate(struct sk_buff *skb, struct rmnet_port *port,
 				    struct net_device *orig_dev);
 void rmnet_map_tx_aggregate_init(struct rmnet_port *port);
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 577f2758e385..bb88e19e28d8 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -7,7 +7,11 @@
 #include <linux/netdevice.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/tcp.h>
+#include <linux/udp.h>
+#include <net/ip.h>
 #include <net/ip6_checksum.h>
+#include <net/ipv6.h>
 #include <linux/bitfield.h>
 #include "rmnet_config.h"
 #include "rmnet_map.h"
@@ -17,6 +21,19 @@
 #define RMNET_MAP_DEAGGR_SPACING  64
 #define RMNET_MAP_DEAGGR_HEADROOM (RMNET_MAP_DEAGGR_SPACING / 2)
 
+struct rmnet_map_coal_metadata {
+	void *ip_header;
+	void *trans_header;
+	u16 ip_len;
+	u16 trans_len;
+	u16 data_offset;
+	u16 data_len;
+	u8 ip_proto;
+	u8 trans_proto;
+	u8 pkt_count;
+	bool zero_csum;
+};
+
 static __sum16 *rmnet_map_get_csum_field(unsigned char protocol,
 					 const void *txporthdr)
 {
@@ -337,8 +354,8 @@ u32 rmnet_map_validate_packet_len(struct sk_buff *skb, u32 data_format)
 {
 	struct rmnet_map_v5_csum_header *next_hdr = NULL;
 	struct rmnet_map_header *maph;
-	void *data = skb->data;
 	u32 packet_len;
+	u8 hdr_type;
 
 	if (skb->len < sizeof(*maph))
 		return 0;
@@ -353,24 +370,28 @@ u32 rmnet_map_validate_packet_len(struct sk_buff *skb, u32 data_format)
 
 	if (data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
 		packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
-	} else if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
+	} else if ((data_format &
+		    (RMNET_FLAGS_INGRESS_MAP_CKSUMV5 | RMNET_FLAGS_INGRESS_COALESCE)) &&
 		   !(maph->flags & MAP_CMD_FLAG)) {
-		/* Mapv5 data pkt without csum hdr is invalid */
 		if (!(maph->flags & MAP_NEXT_HEADER_FLAG))
 			return 0;
 
-		packet_len += sizeof(*next_hdr);
-		next_hdr = data + sizeof(*maph);
+		if (skb->len < sizeof(*maph) + sizeof(*next_hdr))
+			return 0;
+
+		next_hdr = (struct rmnet_map_v5_csum_header *)(skb->data + sizeof(*maph));
+		hdr_type = u8_get_bits(next_hdr->header_info,
+				       MAPV5_HDRINFO_HDR_TYPE_FMASK);
+
+		if (hdr_type == RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
+			packet_len += sizeof(*next_hdr);
+		else if (hdr_type != RMNET_MAP_HEADER_TYPE_COALESCING)
+			return 0;
 	}
 
 	if (skb->len < packet_len)
 		return 0;
 
-	if (next_hdr &&
-	    u8_get_bits(next_hdr->header_info, MAPV5_HDRINFO_HDR_TYPE_FMASK) !=
-	    RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD)
-		return 0;
-
 	return packet_len;
 }
 
@@ -518,13 +539,482 @@ static bool rmnet_map_get_csum_valid(struct sk_buff *skb)
 	return !!(hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG);
 }
 
-int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
+/* Stamp GSO metadata so the network stack can segment a coalesced SKB. */
+static void rmnet_map_gso_stamp(struct sk_buff *skb,
+				struct rmnet_map_coal_metadata *coal_meta)
+{
+	struct skb_shared_info *shinfo = skb_shinfo(skb);
+
+	if (coal_meta->trans_proto == IPPROTO_TCP)
+		shinfo->gso_type = (coal_meta->ip_proto == 4) ?
+				   SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
+	else
+		shinfo->gso_type = SKB_GSO_UDP_L4;
+
+	shinfo->gso_size = coal_meta->data_len;
+	shinfo->gso_segs = coal_meta->pkt_count;
+}
+
+/* Set the transport checksum to the pseudo-header checksum and request
+ * partial checksum offload, letting the NIC or stack finish it.
+ */
+static void rmnet_map_partial_csum(struct sk_buff *skb,
+				   struct rmnet_map_coal_metadata *coal_meta)
+{
+	u16 pkt_len = skb->len - coal_meta->ip_len;
+	unsigned char *data = skb->data;
+	__sum16 pseudo;
+
+	if (coal_meta->ip_proto == 4) {
+		struct iphdr *iph = (struct iphdr *)data;
+
+		pseudo = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
+					    pkt_len, coal_meta->trans_proto, 0);
+	} else {
+		struct ipv6hdr *ip6h = (struct ipv6hdr *)data;
+
+		pseudo = ~csum_ipv6_magic(&ip6h->saddr, &ip6h->daddr,
+					  pkt_len, coal_meta->trans_proto, 0);
+	}
+
+	if (coal_meta->trans_proto == IPPROTO_TCP) {
+		struct tcphdr *tp = (struct tcphdr *)(data + coal_meta->ip_len);
+
+		tp->check = pseudo;
+		skb->csum_offset = offsetof(struct tcphdr, check);
+	} else {
+		struct udphdr *up = (struct udphdr *)(data + coal_meta->ip_len);
+
+		up->check = pseudo;
+		skb->csum_offset = offsetof(struct udphdr, check);
+	}
+
+	skb->ip_summed = CHECKSUM_PARTIAL;
+	skb->csum_start = skb->data + coal_meta->ip_len - skb->head;
+}
+
+/* Carve one logical segment from a coalesced SKB and append it to the list.
+ * Adjusts TCP sequence numbers, IP IDs/lengths, and checksum state.
+ */
+static void
+__rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
+			     struct rmnet_map_coal_metadata *coal_meta,
+			     struct sk_buff_head *list, u8 pkt_id,
+			     bool csum_valid)
+{
+	u32 dlen = coal_meta->data_len * coal_meta->pkt_count;
+	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+	struct sk_buff *skbn;
+
+	/* RFC 768: UDP checksum is optional for IPv4, and is 0 if unused.
+	 * Such packets are never actually bad, regardless of what the
+	 * checksum bitmap says.
+	 */
+	if (!csum_valid && coal_meta->zero_csum)
+		csum_valid = true;
+
+	if (!csum_valid)
+		goto next_pkt;
+
+	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
+	if (!skbn)
+		goto next_pkt;
+
+	skb_reserve(skbn, hlen + RMNET_MAP_DEAGGR_HEADROOM);
+	skb_put_data(skbn,
+		     coal_skb->data + coal_meta->ip_len + coal_meta->trans_len +
+		     coal_meta->data_offset,
+		     dlen);
+
+	/* Restore transport header */
+	skb_push(skbn, coal_meta->trans_len);
+	memcpy(skbn->data, coal_meta->trans_header, coal_meta->trans_len);
+	skb_reset_transport_header(skbn);
+
+	if (coal_meta->trans_proto == IPPROTO_TCP) {
+		struct tcphdr *th = tcp_hdr(skbn);
+
+		th->seq = htonl(ntohl(th->seq) + coal_meta->data_offset);
+		/* Strip dangerous flags from non-final segments */
+		if ((th->fin || th->psh) &&
+		    hlen + coal_meta->data_offset + dlen < coal_skb->len) {
+			th->fin = 0;
+			th->psh = 0;
+		}
+	} else if (coal_meta->trans_proto == IPPROTO_UDP) {
+		struct udphdr *uh = udp_hdr(skbn);
+
+		uh->len = htons(skbn->len);
+	}
+
+	/* Restore IP header */
+	skb_push(skbn, coal_meta->ip_len);
+	memcpy(skbn->data, coal_meta->ip_header, coal_meta->ip_len);
+	skb_reset_network_header(skbn);
+
+	if (coal_meta->ip_proto == 4) {
+		struct iphdr *iph = ip_hdr(skbn);
+
+		iph->id = htons(ntohs(iph->id) + pkt_id);
+		iph->tot_len = htons(skbn->len);
+		iph->check = 0;
+		iph->check = ip_fast_csum(iph, iph->ihl);
+	} else {
+		ipv6_hdr(skbn)->payload_len =
+			htons(skbn->len - sizeof(struct ipv6hdr));
+	}
+
+	rmnet_map_partial_csum(skbn, coal_meta);
+
+	skbn->dev = coal_skb->dev;
+
+	if (coal_meta->pkt_count > 1)
+		rmnet_map_gso_stamp(skbn, coal_meta);
+
+	__skb_queue_tail(list, skbn);
+
+next_pkt:
+	coal_meta->data_offset += dlen;
+	coal_meta->pkt_count = 0;
+}
+
+/* Parse the IP header of the coalesced frame and perform basic
+ * validation of header fields.
+ */
+static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
+					struct rmnet_map_coal_metadata *meta,
+					bool *gro)
+{
+	struct ipv6hdr *ip6h;
+	struct iphdr *iph;
+	__be16 frag_off;
+	u8 protocol;
+	int ret;
+
+	if (coal_skb->len < sizeof(*iph))
+		return false;
+
+	iph = (struct iphdr *)coal_skb->data;
+
+	if (iph->version == 4) {
+		meta->ip_proto = 4;
+		meta->ip_len = iph->ihl * 4;
+		meta->trans_proto = iph->protocol;
+		meta->ip_header = iph;
+		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len)
+			return false;
+
+		if (ip_is_fragment(iph))
+			return false;
+
+		if (iph->ihl != 5)
+			*gro = false;
+	} else if (iph->version == 6) {
+		if (coal_skb->len < sizeof(*ip6h))
+			return false;
+
+		ip6h = (struct ipv6hdr *)iph;
+		protocol = ip6h->nexthdr;
+		meta->ip_proto = 6;
+		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
+				       &frag_off);
+		if (ret < 0 || frag_off)
+			return false;
+
+		meta->ip_len = (u16)ret;
+		meta->trans_proto = protocol;
+		meta->ip_header = ip6h;
+		if (meta->ip_len > sizeof(*ip6h))
+			*gro = false;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+
+/* Parse the transport header following the IP header into coal_meta. The
+ * available length is checked before any field is read.
+ */
+static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
+					   struct rmnet_map_coal_metadata *meta)
+{
+	struct udphdr *uh;
+	struct tcphdr *th;
+	u32 avail;
+	u8 *base;
+
+	base = (u8 *)meta->ip_header + meta->ip_len;
+	avail = coal_skb->len - meta->ip_len;
+
+	if (meta->trans_proto == IPPROTO_TCP) {
+		if (avail < sizeof(*th))
+			return false;
+
+		th = (struct tcphdr *)base;
+		meta->trans_len = th->doff * 4;
+		meta->trans_header = th;
+		if (meta->trans_len < sizeof(*th) || avail < meta->trans_len)
+			return false;
+	} else if (meta->trans_proto == IPPROTO_UDP) {
+		if (avail < sizeof(*uh))
+			return false;
+
+		uh = (struct udphdr *)base;
+		meta->trans_len = sizeof(*uh);
+		meta->trans_header = uh;
+		if (meta->ip_proto == 4 && !uh->check)
+			meta->zero_csum = true;
+	} else {
+		return false;
+	}
+
+	return true;
+}
+
+/* Reject the frame if the total data bytes claimed by all NLOs exceed
+ * the actual payload in the SKB.  Each pkt_len covers IP+transport
+ * headers plus per-packet data. Headers appear once, so subtract hlen
+ * per packet and check the running sum against available data.
+ */
+static bool rmnet_map_coal_validate_bounds(struct sk_buff *coal_skb,
+					   struct rmnet_map_v5_coal_header *coal_hdr,
+					   u8 num_nlos, u32 hlen)
+{
+	u32 total_data = 0;
+	u32 nlo_len;
+	u16 plen;
+	u8 i;
+
+	for (i = 0; i < num_nlos; i++) {
+		plen = ntohs(coal_hdr->nl_pairs[i].pkt_len);
+
+		if (plen < hlen)
+			return false;
+
+		nlo_len = (u32)(plen - hlen) * coal_hdr->nl_pairs[i].num_packets;
+		if (total_data + nlo_len > coal_skb->len - hlen)
+			return false;
+
+		total_data += nlo_len;
+	}
+
+	return true;
+}
+
+/* Attempt the GRO-friendly fast path for a single-NLO, checksum-valid frame
+ * by reusing the original SKB and stamping GSO metadata instead of copying
+ * out each segment.  Returns true if the frame was consumed via the fast
+ * path, or false if the caller should fall back to full segmentation.
+ */
+static bool rmnet_map_coal_gro_fast_path(struct sk_buff *coal_skb,
+					 struct rmnet_map_v5_coal_header *coal_hdr,
+					 struct rmnet_map_coal_metadata *coal_meta,
+					 struct sk_buff_head *list,
+					 u8 num_nlos, bool gro)
+{
+	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+
+	if (!gro || num_nlos != 1 ||
+	    !(coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG))
+		return false;
+
+	coal_meta->data_len = ntohs(coal_hdr->nl_pairs[0].pkt_len) - hlen;
+	coal_meta->pkt_count = coal_hdr->nl_pairs[0].num_packets;
+
+	coal_skb->ip_summed = CHECKSUM_UNNECESSARY;
+	if (coal_meta->pkt_count > 1) {
+		rmnet_map_partial_csum(coal_skb, coal_meta);
+		rmnet_map_gso_stamp(coal_skb, coal_meta);
+	}
+
+	__skb_queue_tail(list, coal_skb);
+	return true;
+}
+
+/* NLO packet lengths are already bounds-checked by
+ * rmnet_map_coal_validate_bounds() so no further validation is needed here.
+ */
+static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
+					struct rmnet_map_v5_coal_header *coal_hdr,
+					struct rmnet_map_coal_metadata *coal_meta,
+					struct sk_buff_head *list,
+					u64 nlo_err_mask, bool gro, u8 num_nlos)
+{
+	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
+	u8 pkt, total_pkt = 0;
+	bool csum_err;
+	u16 pkt_len;
+	u8 nlo;
+
+	for (nlo = 0; nlo < num_nlos; nlo++) {
+		pkt_len = ntohs(coal_hdr->nl_pairs[nlo].pkt_len);
+		pkt_len -= hlen;
+		coal_meta->data_len = pkt_len;
+
+		/* nlo_err_mask is one flat bitstream across all NLOs. Shift
+		 * it once per packet in absolute frame order and do not
+		 * re-align at the NLO boundary above. See the comment on
+		 * rmnet_map_data_check_coal_header() for why.
+		 */
+		for (pkt = 0; pkt < coal_hdr->nl_pairs[nlo].num_packets;
+		     pkt++, total_pkt++, nlo_err_mask >>= 1) {
+			csum_err = nlo_err_mask & 1;
+
+			if (!gro) {
+				coal_meta->pkt_count = 1;
+				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
+							     list, total_pkt,
+							     !csum_err);
+				continue;
+			}
+
+			if (csum_err) {
+				if (coal_meta->pkt_count)
+					__rmnet_map_segment_coal_skb(coal_skb,
+								     coal_meta,
+								     list,
+								     total_pkt,
+								     true);
+				coal_meta->pkt_count = 1;
+				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
+							     list, total_pkt,
+							     false);
+			} else {
+				coal_meta->pkt_count++;
+			}
+		}
+
+		/* Flush remaining packets from this NLO */
+		if (coal_meta->pkt_count)
+			__rmnet_map_segment_coal_skb(coal_skb, coal_meta, list,
+						     total_pkt, true);
+	}
+}
+
+/* Expand a coalesced SKB into individual IP packets placed on the list.
+ * NLOs with checksum errors are dropped. __rmnet_map_ingress_handler will
+ * free the SKB in the error case.
+ */
+static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
+				      u64 nlo_err_mask,
 				      struct sk_buff_head *list,
 				      u16 len)
+{
+	bool gro = coal_skb->dev->features & NETIF_F_GRO_HW;
+	struct rmnet_map_v5_coal_header *coal_hdr;
+	struct rmnet_map_coal_metadata coal_meta;
+	u8 num_nlos;
+	u32 hlen;
+
+	memset(&coal_meta, 0, sizeof(coal_meta));
+
+	/* Drop any MAP frame padding. The coal header is counted in len */
+	skb_pull(coal_skb, sizeof(struct rmnet_map_header));
+	skb_trim(coal_skb, len);
+	coal_hdr = (struct rmnet_map_v5_coal_header *)coal_skb->data;
+	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
+	skb_pull(coal_skb, sizeof(*coal_hdr));
+
+	if (!rmnet_map_coal_parse_ip_hdr(coal_skb, &coal_meta, &gro))
+		return -EINVAL;
+
+	if (!rmnet_map_coal_parse_trans_hdr(coal_skb, &coal_meta))
+		return -EINVAL;
+
+	hlen = coal_meta.ip_len + coal_meta.trans_len;
+
+	if (!rmnet_map_coal_validate_bounds(coal_skb, coal_hdr, num_nlos, hlen))
+		return -EINVAL;
+
+	if (rmnet_map_coal_gro_fast_path(coal_skb, coal_hdr, &coal_meta, list,
+					 num_nlos, gro))
+		return 0;
+
+	rmnet_map_coal_segment_loop(coal_skb, coal_hdr, &coal_meta, list,
+				    nlo_err_mask, gro, num_nlos);
+
+	return 0;
+}
+
+/* Validate the coalescing header and build the checksum error mask.
+ *
+ * Checks performed:
+ *  - MAP pkt_len accommodates the coal header (coal header is counted in
+ *    pkt_len. pkt_len < sizeof(*coal_hdr) means no payload is possible).
+ *  - num_nlos is in [1, RMNET_MAP_V5_MAX_NLOS].
+ *  - Total packet count does not exceed RMNET_MAP_V5_MAX_PACKETS.
+ *
+ * nlo_err_mask is NOT six independent per-NLO bitmaps. Each nl_pairs
+ * slot only has room for an 8 bit csum_error_bitmap, but a single NLO
+ * can carry more than 8 packets (up to RMNET_MAP_V5_MAX_PACKETS), so
+ * hardware spills a wide NLO's error bits into the csum_error_bitmap
+ * bytes of the following slots rather than truncating them. The
+ * six bitmap bytes are therefore always concatenated in slot order
+ * into one flat RMNET_MAP_V5_MAX_NLOS * 8 = RMNET_MAP_V5_MAX_PACKETS
+ * bit value, addressed by a packet's absolute position in the frame,
+ * regardless of how many NLOs are actually in use. Receivers must
+ * walk it as a single contiguous stream and must not expect it to
+ * align it at NLO boundaries.
+ */
+static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
+					    u64 *nlo_err_mask)
+{
+	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
+	struct rmnet_map_v5_coal_header *coal_hdr;
+	u8 num_nlos, pkts = 0;
+	u64 mask = 0;
+	int i;
+
+	/* coal header is counted in pkt_len */
+	if (ntohs(maph->pkt_len) < sizeof(*coal_hdr))
+		return -EINVAL;
+
+	coal_hdr = (struct rmnet_map_v5_coal_header *)(skb->data + sizeof(*maph));
+	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
+
+	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS)
+		return -EINVAL;
+
+	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
+		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
+		u8 pkt = coal_hdr->nl_pairs[i].num_packets;
+
+		mask |= ((u64)err) << (8 * i);
+		pkts += pkt;
+		if (pkts > RMNET_MAP_V5_MAX_PACKETS)
+			return -EINVAL;
+	}
+
+	*nlo_err_mask = mask;
+	return 0;
+}
+
+int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
+				      struct sk_buff_head *list,
+				      u16 len, u32 data_format)
 {
 	struct rmnet_priv *priv = netdev_priv(skb->dev);
+	u64 nlo_err_mask;
+	int rc;
 
 	switch (rmnet_map_get_next_hdr_type(skb)) {
+	case RMNET_MAP_HEADER_TYPE_COALESCING:
+		if (!(data_format & RMNET_FLAGS_INGRESS_COALESCE))
+			return -EINVAL;
+
+		rc = rmnet_map_data_check_coal_header(skb, &nlo_err_mask);
+		if (rc)
+			return rc;
+
+		rc = rmnet_map_segment_coal_skb(skb, nlo_err_mask, list, len);
+		if (rc)
+			return rc;
+
+		if (skb_peek(list) != skb)
+			consume_skb(skb);
+		break;
+
 	case RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD:
 		if (unlikely(!(skb->dev->features & NETIF_F_RXCSUM))) {
 			priv->stats.csum_sw++;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index 5f921cddf82b..e1e319683a55 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -323,6 +323,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
 	rmnet_dev->hw_features = NETIF_F_RXCSUM;
 	rmnet_dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
 	rmnet_dev->hw_features |= NETIF_F_SG;
+	rmnet_dev->hw_features |= NETIF_F_GRO_HW;
 
 	priv->real_dev = real_dev;
 
diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
index c44bf6e80ecb..5df4d417f3d2 100644
--- a/include/linux/if_rmnet.h
+++ b/include/linux/if_rmnet.h
@@ -70,5 +70,36 @@ struct rmnet_map_v5_csum_header {
 #define MAPV5_HDRINFO_HDR_TYPE_FMASK	GENMASK(7, 1)
 #define MAPV5_CSUMINFO_VALID_FLAG	BIT(7)
 
+#define RMNET_MAP_HEADER_TYPE_COALESCING   1
 #define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2
+
+/* MAPv5 coalescing header */
+#define RMNET_MAP_V5_MAX_NLOS		6
+#define RMNET_MAP_V5_MAX_PACKETS	48
+
+/* One Number-Length Object pair: per-NLO packet count and length. */
+struct rmnet_map_v5_nl_pair {
+	__be16 pkt_len;
+	u8  csum_error_bitmap;
+	u8  num_packets;
+} __aligned(1);
+
+/* MAPv5 coalescing header: describes up to RMNET_MAP_V5_MAX_NLOS NLOs.
+ * The header immediately follows the MAP header in the frame and is
+ * included in the MAP pkt_len field.
+ */
+struct rmnet_map_v5_coal_header {
+	u8  header_info;	/* MAPV5_HDRINFO_NXT_HDR_FLAG, MAPV5_HDRINFO_HDR_TYPE_FMASK */
+	u8  coal_info;		/* MAPV5_COALINFO_* */
+	u8  close_info;		/* MAPV5_CLOSEINFO_* */
+	u8  veid_info;		/* MAPV5_VEIDINFO_* */
+	struct rmnet_map_v5_nl_pair nl_pairs[RMNET_MAP_V5_MAX_NLOS];
+} __aligned(1);
+
+#define MAPV5_COALINFO_NUM_NLOS_FMASK		GENMASK(6, 4)
+#define MAPV5_COALINFO_CSUM_VALID_FLAG		BIT(7)
+#define MAPV5_CLOSEINFO_CLOSE_TYPE_FMASK	GENMASK(3, 0)
+#define MAPV5_CLOSEINFO_CLOSE_VALUE_FMASK	GENMASK(7, 4)
+#define MAPV5_VEIDINFO_VEID_FMASK		GENMASK(3, 0)
+
 #endif /* !(_LINUX_IF_RMNET_H_) */
-- 
2.34.1

Re: [PATCH net-next 4/7] net: qualcomm: rmnet: Add DL packet coalescing support

[netdev-bot+sashiko]

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

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

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] In the IPv6 branch of rmnet_map_coal_parse_ip_hdr(),
  meta->ip_len is taken directly from the return value of…
- [Medium] rmnet_map_data_check_coal_header() adds up
  nl_pairs[i].num_packets in a `u8 pkts`.
- [Medium] When NETIF_F_GRO_HW is enabled, rmnet_map_coal_segment_loop()
  flushes a run of good packets by calling…
- [Medium] The GRO fast path modifies the original coalesced skb in place
  without first checking skb_cloned() or calling skb_unclone().
- [Medium] The COALESCING case of rmnet_map_process_next_hdr_packet()
  never checks NETIF_F_RXCSUM.
- [Medium] rmnet_map_gso_stamp() copies device-supplied NLO geometry
  straight into gso_size/gso_segs.
- [Low] rmnet_map_coal_validate_bounds() rejects only `plen < hlen`, so
  an NLO with plen == hlen (zero payload per packet) is accepted for any…
- [Low] In rmnet_map_process_next_hdr_packet(), the coalesced skb is
  freed with consume_skb() whenever it is not the head of the output…
- [Low] Drops in the coalescing path are not fully counted.
- [Low] The commit message says every packet queued to the stack gets GSO
  information and a partial checksum set.
- [Low] Documentation and implementation still disagree on the coalescing
  wire format.

--

net: qualcomm: rmnet: Add DL packet coalescing support

This adds support for MAPv5 coalescing frames, where hardware merges
multiple TCP or UDP packets of one flow into a single MAP frame. The frame
is described by up to 6 NLOs and a checksum error bitmap. The driver either
reuses the frame directly with GSO metadata (GRO_HW fast path) or rebuilds
the individual segments, dropping those with checksum errors. The patch
also adds NETIF_F_GRO_HW to hw_features and requires MAPv5 ingress checksum
offload whenever coalescing is enabled.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 577f2758e3855..bb88e19e28d82 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -518,13 +539,482 @@ static bool rmnet_map_get_csum_valid(struct sk_buff *skb)
>  	return !!(hdr->csum_info & MAPV5_CSUMINFO_VALID_FLAG);
>  }
>  
> -int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
> +/* Stamp GSO metadata so the network stack can segment a coalesced SKB. */
> +static void rmnet_map_gso_stamp(struct sk_buff *skb,
> +				struct rmnet_map_coal_metadata *coal_meta)
> +{
> +	struct skb_shared_info *shinfo = skb_shinfo(skb);
> +
> +	if (coal_meta->trans_proto == IPPROTO_TCP)
> +		shinfo->gso_type = (coal_meta->ip_proto == 4) ?
> +				   SKB_GSO_TCPV4 : SKB_GSO_TCPV6;
> +	else
> +		shinfo->gso_type = SKB_GSO_UDP_L4;
> +
> +	shinfo->gso_size = coal_meta->data_len;
> +	shinfo->gso_segs = coal_meta->pkt_count;

[Severity: Medium]

Can gso_size and gso_segs end up inconsistent with skb->len here? In
rmnet_map_coal_gro_fast_path() both values come straight from the NLO:

    coal_meta->data_len = ntohs(coal_hdr->nl_pairs[0].pkt_len) - hlen;
    coal_meta->pkt_count = coal_hdr->nl_pairs[0].num_packets;

rmnet_map_coal_validate_bounds() only checks that the claimed data fits in
the skb (<=). The fast path then reuses the original skb without trimming
it or checking that skb->len == hlen + data_len * pkt_count.

For example, data_len 1 and pkt_count 2 on a 64KB skb gives gso_size 1 and
gso_segs 2. Software GSO or TSO on the forwarding path would then produce
tens of thousands of segments.

SKB_GSO_DODGY is not set, so the dev->gso_max_segs check and qdisc
accounting trust gso_segs. Should the length consistency be checked, or
should SKB_GSO_DODGY be set, since this geometry is supplied by the device?

> +}

[ ... ]

> +static void
> +__rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
> +			     struct rmnet_map_coal_metadata *coal_meta,
> +			     struct sk_buff_head *list, u8 pkt_id,
> +			     bool csum_valid)
> +{

[ ... ]

> +	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
> +	if (!skbn)
> +		goto next_pkt;

[Severity: Low]

Should the drops in the coalescing path be counted? In this patch none of
these update a counter:

- checksum-error segments
- alloc_skb() failures here
- whole frames rejected by rmnet_map_data_check_coal_header(),
  rmnet_map_coal_parse_ip_hdr(), rmnet_map_coal_parse_trans_hdr() or
  rmnet_map_coal_validate_bounds()

The existing MAPv4/MAPv5 checksum code in this file counts each outcome.

The later commit "Add ethtool stats for DL coalescing" adds coal_csum_drop
and counters for header errors and invalid IP/transport headers. At the
end of the series, two cases are still uncounted: segments lost to
alloc_skb() failure, and frames rejected by
rmnet_map_coal_validate_bounds().

[ ... ]

> +	if (iph->version == 4) {
> +		meta->ip_proto = 4;
> +		meta->ip_len = iph->ihl * 4;
> +		meta->trans_proto = iph->protocol;
> +		meta->ip_header = iph;
> +		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len)
> +			return false;

[ ... ]

> +	} else if (iph->version == 6) {
> +		if (coal_skb->len < sizeof(*ip6h))
> +			return false;
> +
> +		ip6h = (struct ipv6hdr *)iph;
> +		protocol = ip6h->nexthdr;
> +		meta->ip_proto = 6;
> +		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
> +				       &frag_off);
> +		if (ret < 0 || frag_off)
> +			return false;
> +
> +		meta->ip_len = (u16)ret;

[Severity: Critical]

Is meta->ip_len checked against coal_skb->len anywhere in the IPv6 branch?
The IPv4 branch above checks coal_skb->len < meta->ip_len, but here the
return value of ipv6_skip_exthdr() is used as is.

ipv6_skip_exthdr() only reads the 2-byte generic header of each extension
header. It then does start += hdrlen and returns start without comparing
it with skb->len.

Suppose the last extension header is a Hop-by-Hop header with hdrlen 255
and nexthdr TCP or UDP. ip_len then ends up about 2KB past the end of the
packet.

After that, both of these underflow as u32 and the checks pass.

rmnet_map_coal_parse_trans_hdr():

    avail = coal_skb->len - meta->ip_len;

rmnet_map_coal_validate_bounds():

    if (total_data + nlo_len > coal_skb->len - hlen)

As a result, th->doff and uh->check are read out of bounds.

Because ip_len > sizeof(struct ipv6hdr) forces gro = false, the frame goes
through rmnet_map_coal_segment_loop()->__rmnet_map_segment_coal_skb(),
which does:

	skb_put_data(skbn,
		     coal_skb->data + coal_meta->ip_len + coal_meta->trans_len +
		     coal_meta->data_offset,
		     dlen);
	...
	memcpy(skbn->data, coal_meta->ip_header, coal_meta->ip_len);

Could this copy adjacent kernel heap memory into new skbs that
rmnet_deliver_skb_list() then hands to the stack, where a packet socket on
the rmnet device can read it?

The input comes from modem firmware, or from any lower device rmnet is
stacked on, including the ARPHRD_ETHER path. rmnet links can be created
over rtnetlink with CAP_NET_ADMIN, which is available inside a user
namespace.

This code is unchanged at the end of the series. The later patches only
add stat counters to this function.

[ ... ]

> +static bool rmnet_map_coal_validate_bounds(struct sk_buff *coal_skb,
> +					   struct rmnet_map_v5_coal_header *coal_hdr,
> +					   u8 num_nlos, u32 hlen)
> +{
> +	u32 total_data = 0;
> +	u32 nlo_len;
> +	u16 plen;
> +	u8 i;
> +
> +	for (i = 0; i < num_nlos; i++) {
> +		plen = ntohs(coal_hdr->nl_pairs[i].pkt_len);
> +
> +		if (plen < hlen)
> +			return false;

[Severity: Low]

Should plen == hlen be rejected as well? That NLO has zero bytes of
payload per packet, and it passes for any num_packets.

data_len then becomes 0. In the fast path with pkt_count > 1,
rmnet_map_gso_stamp() sets gso_size 0 and gso_segs N, plus a TCP/UDP
gso_type, on a CHECKSUM_PARTIAL skb. The GRO segmentation path likewise
merges consecutive zero-payload packets into one header-only skb with
gso_size 0.

skb_is_gso() only looks at gso_size, so the stack treats these as single
packets. N empty UDP datagrams become one, the rest are lost, and the skb
is left with gso_type and gso_segs set while gso_size is 0.

> +
> +		nlo_len = (u32)(plen - hlen) * coal_hdr->nl_pairs[i].num_packets;
> +		if (total_data + nlo_len > coal_skb->len - hlen)
> +			return false;

[ ... ]

> +static bool rmnet_map_coal_gro_fast_path(struct sk_buff *coal_skb,
> +					 struct rmnet_map_v5_coal_header *coal_hdr,
> +					 struct rmnet_map_coal_metadata *coal_meta,
> +					 struct sk_buff_head *list,
> +					 u8 num_nlos, bool gro)
> +{

[ ... ]

> +	coal_skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	if (coal_meta->pkt_count > 1) {
> +		rmnet_map_partial_csum(coal_skb, coal_meta);
> +		rmnet_map_gso_stamp(coal_skb, coal_meta);
> +	}

[Severity: Medium]

Is it safe to modify coal_skb in place here without checking skb_cloned()
or calling skb_unclone()?

rmnet_map_partial_csum() writes tp->check or up->check into the packet
data. rmnet_map_gso_stamp() writes gso_type, gso_size and gso_segs into
skb_shinfo().

rmnet_config_data_format_valid() allows RMNET_FLAGS_INGRESS_COALESCE
without RMNET_FLAGS_INGRESS_DEAGGREGATION. In that case this is the skb
received from the real device:

rmnet_map_ingress_handler() {
    ...
    } else {
        if (rmnet_map_validate_packet_len(skb, data_format))
            __rmnet_map_ingress_handler(skb, port, data_format);
    ...
}

rmnet_rx_handler() only calls skb_linearize(), which does nothing for a
linear skb. There is no skb_share_check() on this path either.

With a ptype_all tap such as af_packet active on the real device,
packet_rcv() holds a clone that shares the same data and shinfo. Would
that clone then see the rewritten L4 checksum and GSO fields?

[ ... ]

> +			if (csum_err) {
> +				if (coal_meta->pkt_count)
> +					__rmnet_map_segment_coal_skb(coal_skb,
> +								     coal_meta,
> +								     list,
> +								     total_pkt,
> +								     true);
> +				coal_meta->pkt_count = 1;
> +				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
> +							     list, total_pkt,
> +							     false);
> +			} else {
> +				coal_meta->pkt_count++;
> +			}
> +		}
> +
> +		/* Flush remaining packets from this NLO */
> +		if (coal_meta->pkt_count)
> +			__rmnet_map_segment_coal_skb(coal_skb, coal_meta, list,
> +						     total_pkt, true);

[Severity: Medium]

Is total_pkt the right pkt_id for these two flushes of a good run? At both
points total_pkt is the index of the packet after the run, either the
checksum-error packet or one past the end of the NLO. It is not the index
of the first packet in the run.

__rmnet_map_segment_coal_skb() uses it directly:

    iph->id = htons(ntohs(iph->id) + pkt_id);

So an NLO0 with 5 packets gets base+5 instead of base+0, and an NLO1 with
3 packets gets base+8 instead of base+5.

The !gro path passes each packet's own index with pkt_count 1, which
suggests base+k is the intended mapping.

These skbs are stamped SKB_GSO_TCPV4 or SKB_GSO_UDP_L4 without
SKB_GSO_TCP_FIXEDID. Any later resegmentation, for example when
forwarding, would produce overlapping IP IDs.

Should this be total_pkt - coal_meta->pkt_count?

[ ... ]

> +static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
> +					    u64 *nlo_err_mask)
> +{
> +	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
> +	struct rmnet_map_v5_coal_header *coal_hdr;
> +	u8 num_nlos, pkts = 0;

[ ... ]

> +	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
> +		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
> +		u8 pkt = coal_hdr->nl_pairs[i].num_packets;
> +
> +		mask |= ((u64)err) << (8 * i);
> +		pkts += pkt;
> +		if (pkts > RMNET_MAP_V5_MAX_PACKETS)
> +			return -EINVAL;

[Severity: Medium]

Can pkts wrap before this comparison? It is a u8, so num_packets of
[48, 208] sums to 0. [48, 255, 255, 255, 255, 255] never exceeds 48 after
truncation, even though it describes 1323 packets.

rmnet_map_coal_validate_bounds() only limits payload bytes. With pkt_len
equal to hlen, or a small data_len, any count gets through.

rmnet_map_coal_segment_loop() then walks every claimed packet:

- after 48 shifts nlo_err_mask is zero, so every later packet is treated
  as checksum-valid
- the u8 total_pkt wraps, which corrupts the IPv4 ID offsets
- with GRO_HW off, or when error bits are set, it does one
  alloc_skb(GFP_ATOMIC) per packet in softirq context

This loop also sums all RMNET_MAP_V5_MAX_NLOS slots instead of only the
first num_nlos. At the end of the series, priv->stats.coal_pkts += pkts
records the wrapped value.

Could pkts use a wider type, and could the loop stop at num_nlos?

> +	}
> +
> +	*nlo_err_mask = mask;
> +	return 0;
> +}
> +
> +int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
> +				      struct sk_buff_head *list,
> +				      u16 len, u32 data_format)
>  {
>  	struct rmnet_priv *priv = netdev_priv(skb->dev);
> +	u64 nlo_err_mask;
> +	int rc;
>  
>  	switch (rmnet_map_get_next_hdr_type(skb)) {
> +	case RMNET_MAP_HEADER_TYPE_COALESCING:
> +		if (!(data_format & RMNET_FLAGS_INGRESS_COALESCE))
> +			return -EINVAL;
> +

[Severity: Medium]

Should this case check NETIF_F_RXCSUM, as the
RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD case below does?

rmnet never sets dev->features, so RXCSUM is off by default.
netdev_fix_features() also clears GRO_HW when RXCSUM is off. Every
coalesced frame then goes through
rmnet_map_coal_segment_loop()->__rmnet_map_segment_coal_skb()->
rmnet_map_partial_csum(). That path sets CHECKSUM_PARTIAL and keeps or
drops packets based only on the hardware error bitmap.

The segmentation path also ignores MAPV5_COALINFO_CSUM_VALID_FLAG, which
the fast path requires.

As a result, turning rx-checksum off has no effect on coalesced traffic.
The later commit "Work around coalescing hardware quirks" shows that the
hardware verdict can be wrong, and the RXCSUM check is still missing at
the end of the series.

> +		rc = rmnet_map_data_check_coal_header(skb, &nlo_err_mask);
> +		if (rc)
> +			return rc;
> +
> +		rc = rmnet_map_segment_coal_skb(skb, nlo_err_mask, list, len);
> +		if (rc)
> +			return rc;
> +
> +		if (skb_peek(list) != skb)
> +			consume_skb(skb);

[Severity: Low]

What happens when segmentation queues nothing? If every segment had a
checksum error, or every alloc_skb() in __rmnet_map_segment_coal_skb()
failed, the list is empty and skb_peek() returns NULL. The frame was
dropped, but consume_skb() reports it as normally consumed, so drop
monitor and the kfree_skb tracepoints never see it.

Should this use kfree_skb(), or kfree_skb_reason(), when the list is
empty?

Segments lost to alloc_skb() failure are also not counted, even after
"Add ethtool stats for DL coalescing", which only adds coal_csum_drop.

> +		break;
> +

[ ... ]

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index 5f921cddf82b4..e1e319683a558 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> @@ -323,6 +323,7 @@ int rmnet_vnd_newlink(u8 id, struct net_device *rmnet_dev,
>  	rmnet_dev->hw_features = NETIF_F_RXCSUM;
>  	rmnet_dev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
>  	rmnet_dev->hw_features |= NETIF_F_SG;
> +	rmnet_dev->hw_features |= NETIF_F_GRO_HW;

[Severity: Low]

The commit message says:

  Any packet queued to the network stack would have the gso information
  populated in the skb and partial checksum set.

Does that match the code? The commit message does not mention this new
GRO_HW toggle. It is only added to hw_features, nothing in rmnet sets
dev->features, and GRO_HW also depends on RXCSUM, so it starts off.

With GRO_HW off, rmnet_map_coal_segment_loop() sets pkt_count to 1 for
every packet, so this never runs:

    if (coal_meta->pkt_count > 1)
        rmnet_map_gso_stamp(skbn, coal_meta);

With GRO_HW on, single-packet fast-path frames get CHECKSUM_UNNECESSARY,
not CHECKSUM_PARTIAL.

Could the commit message describe the GRO_HW feature and how each mode
behaves?

>  
>  	priv->real_dev = real_dev;
>  
> diff --git a/include/linux/if_rmnet.h b/include/linux/if_rmnet.h
> index c44bf6e80ecb4..5df4d417f3d2c 100644
> --- a/include/linux/if_rmnet.h
> +++ b/include/linux/if_rmnet.h
> @@ -70,5 +70,36 @@ struct rmnet_map_v5_csum_header {
>  #define MAPV5_HDRINFO_HDR_TYPE_FMASK	GENMASK(7, 1)
>  #define MAPV5_CSUMINFO_VALID_FLAG	BIT(7)
>  
> +#define RMNET_MAP_HEADER_TYPE_COALESCING   1
>  #define RMNET_MAP_HEADER_TYPE_CSUM_OFFLOAD 2

[Severity: Low]

At this commit,
Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst still
lists next header type 1 as Reserved and says the next header can only be
the checksum header.

The later commit "docs: networking: Add documentation for the coalescing
support in rmnet" fixes the type table. However, it describes the checksum
error bitmap as per-NLO:

  CSUM error bitmap is a per-packet bitmask. Bit N is set when packet N
  in this NLO has a bad checksum.

The comment on rmnet_map_data_check_coal_header() says the opposite: one
flat stream across the NLO slots, indexed by the packet's absolute
position in the frame. rmnet_map_coal_segment_loop() walks it that way.

The docs also don't mention that the coalescing header is counted in
pkt_len. Which interpretation of the bitmap is correct?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930051345.857443-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next 5/7] net: qualcomm: rmnet: Work around coalescing hardware quirks

[Subash Abhinov Kasiviswanathan]

On certain hardware, the checksum valid indication from the bitmap
inside a MAPv5 coalescing frame cannot be trusted when the frame contains
exactly one packet and hardware completes the coalescing due to a
TCP FIN or PSH flag, a packet count limit, a byte count limit or a time
limit.  The hardware sets CSUM_VALID incorrectly in these cases, causing
the driver to mark packets CHECKSUM_UNNECESSARY when the checksum may in
fact be wrong.

Instead, set ip_summed to CHECKSUM_NONE so that the network stack verifies
the checksum rather than trusting the hardware indication.

The single NLO, single packet determination this fix depends on cannot
be based on the num_nlos field declared in the coalescing header, since
on certain simulation hardware configurations that field can be reported
incorrectly even though the per-NLO num_packets fields are accurate.
Recompute the true NLO count from the actual nl_pairs[] content before
applying the fixup so the determination is reliable regardless of
whether num_nlos itself can be trusted.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index bb88e19e28d8..1f9e592e24b6 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -593,6 +593,61 @@ static void rmnet_map_partial_csum(struct sk_buff *skb,
 	skb->csum_start = skb->data + coal_meta->ip_len - skb->head;
 }
 
+/* On some hardware, num_nlos in the coalescing header can be reported
+ * incorrectly under certain conditions even though the per-NLO num_packets
+ * fields it is meant to summarize are correct. Recompute the true NLO count
+ * directly from the nl_pairs[] content rather than trusting the declared
+ * value, so that rmnet_map_v5_csum_fixup()'s single NLO, single packet check
+ * is reliable.
+ */
+static void rmnet_map_v5_fixup_num_nlos(struct rmnet_map_v5_coal_header *coal_hdr)
+{
+	u8 nlos = 0;
+	int i;
+
+	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
+		if (coal_hdr->nl_pairs[i].num_packets)
+			nlos++;
+	}
+
+	coal_hdr->coal_info = u8_encode_bits(nlos, MAPV5_COALINFO_NUM_NLOS_FMASK) |
+			      (coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG);
+}
+
+/* The checksum valid indication for a single NLO, single packet coalescing
+ * frame cannot be trusted when the close reason is a TCP FIN/PSH, a packet
+ * count limit, a byte count limit or a time limit.
+ */
+static bool rmnet_map_v5_csum_fixup(struct rmnet_map_v5_coal_header *coal_hdr)
+{
+	u8 close_value = u8_get_bits(coal_hdr->close_info,
+				     MAPV5_CLOSEINFO_CLOSE_VALUE_FMASK);
+	u8 close_type = u8_get_bits(coal_hdr->close_info,
+				    MAPV5_CLOSEINFO_CLOSE_TYPE_FMASK);
+	u8 num_nlos = u8_get_bits(coal_hdr->coal_info,
+				  MAPV5_COALINFO_NUM_NLOS_FMASK);
+
+	/* Only applies to single NLO, single packet frames */
+	if (num_nlos != 1 || coal_hdr->nl_pairs[0].num_packets != 1)
+		return false;
+
+	/* TCP FIN or PSH triggered the close */
+	if (close_type == RMNET_MAP_COAL_CLOSE_COAL)
+		return true;
+
+	/* Hit a hardware limit */
+	if (close_type == RMNET_MAP_COAL_CLOSE_HW) {
+		switch (close_value) {
+		case RMNET_MAP_COAL_CLOSE_HW_PKT:
+		case RMNET_MAP_COAL_CLOSE_HW_BYTE:
+		case RMNET_MAP_COAL_CLOSE_HW_TIME:
+			return true;
+		}
+	}
+
+	return false;
+}
+
 /* Carve one logical segment from a coalesced SKB and append it to the list.
  * Adjusts TCP sequence numbers, IP IDs/lengths, and checksum state.
  */
@@ -913,6 +968,7 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	skb_pull(coal_skb, sizeof(struct rmnet_map_header));
 	skb_trim(coal_skb, len);
 	coal_hdr = (struct rmnet_map_v5_coal_header *)coal_skb->data;
+	rmnet_map_v5_fixup_num_nlos(coal_hdr);
 	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
 	skb_pull(coal_skb, sizeof(*coal_hdr));
 
@@ -927,6 +983,12 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	if (!rmnet_map_coal_validate_bounds(coal_skb, coal_hdr, num_nlos, hlen))
 		return -EINVAL;
 
+	if (rmnet_map_v5_csum_fixup(coal_hdr) && !coal_meta.zero_csum) {
+		coal_skb->ip_summed = CHECKSUM_NONE;
+		__skb_queue_tail(list, coal_skb);
+		return 0;
+	}
+
 	if (rmnet_map_coal_gro_fast_path(coal_skb, coal_hdr, &coal_meta, list,
 					 num_nlos, gro))
 		return 0;
-- 
2.34.1

Re: [PATCH net-next 5/7] net: qualcomm: rmnet: Work around coalescing hardware quirks

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 3 · 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).

- [High] Out-of-bounds read while parsing IPv6 coalescing frames.
- [Medium] The recount does not meet the commit's claim that the
  determination is reliable 'regardless of whether num_nlos itself can…
- [Medium] rmnet_map_v5_fixup_num_nlos() (lines 603-615) sets num_nlos to
  the number of the six nl_pairs[] slots whose num_packets is non-zero,…
- [Medium] rmnet_map_v5_fixup_num_nlos() writes coal_hdr->coal_info (line
  613) directly into the data of the received skb.
- [Low] The commit message mixes up two different hardware checksum
  indications.

--

net: qualcomm: rmnet: Work around coalescing hardware quirks

This patch works around hardware that reports a bad checksum valid
indication for single-NLO, single-packet MAPv5 coalescing frames with
some close reasons. It hands those frames to the stack as CHECKSUM_NONE.
It also recomputes num_nlos from the per-slot num_packets fields, because
the declared value can be wrong on some simulation hardware.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index bb88e19e28d82..1f9e592e24b65 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -593,6 +593,61 @@ static void rmnet_map_partial_csum(struct sk_buff *skb,

[ ... ]

> +static void rmnet_map_v5_fixup_num_nlos(struct rmnet_map_v5_coal_header *coal_hdr)
> +{
> +	u8 nlos = 0;
> +	int i;
> +
> +	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
> +		if (coal_hdr->nl_pairs[i].num_packets)
> +			nlos++;
> +	}

[Severity: Medium]
This counts the slots with a non-zero num_packets anywhere in nl_pairs[].
The users of num_nlos, however, read it as the length of a prefix.

rmnet_map_coal_validate_bounds() and rmnet_map_coal_segment_loop() walk
nl_pairs[0..num_nlos-1]. rmnet_map_v5_csum_fixup() and
rmnet_map_coal_gro_fast_path() look at nl_pairs[0] when num_nlos == 1.

Do these two readings only agree when the used slots start at slot 0,
have no gaps, and unused slots read as zero?

For example, num_packets of [0, 1, 0, 0, 0, 0] gives a recount of 1.
rmnet_map_v5_csum_fixup() then hits:

	if (num_nlos != 1 || coal_hdr->nl_pairs[0].num_packets != 1)
		return false;

With GRO enabled and CSUM_VALID set, rmnet_map_coal_gro_fast_path() then
queues coal_skb as CHECKSUM_UNNECESSARY. That is the indication this patch
is trying to stop trusting. Without GRO, nothing is emitted and the frame
is consumed.

With [1, 0, 1, ...] the recount is 2. The packet in slot 2 is then lost
without any error, or the frame is rejected if slot 1's pkt_len is below
hlen.

A leftover non-zero num_packets in an unused trailing slot of a frame
declared with num_nlos = 1 would also raise the recount. That skips the
fixup and brings undeclared slots into validation and segmentation. Before
this patch, only the declared prefix was read.

The comment above rmnet_map_data_check_coal_header() already says that
hardware writes into the csum_error_bitmap bytes of unused slots. Is it
guaranteed that num_packets in unused slots is always zero?

Would it be more robust to use the index of the last non-zero slot plus
one, or to stop at the first empty slot and reject gaps? Another option is
for rmnet_map_v5_csum_fixup() to check directly that slot 0 holds one
packet and all other slots are empty.

> +
> +	coal_hdr->coal_info = u8_encode_bits(nlos, MAPV5_COALINFO_NUM_NLOS_FMASK) |
> +			      (coal_hdr->coal_info & MAPV5_COALINFO_CSUM_VALID_FLAG);

[Severity: Medium]
This writes coal_info in place, in the data of the received skb.

On a raw-IP real device without RMNET_FLAGS_INGRESS_DEAGGREGATION,
rmnet_map_ingress_handler() passes the original rx skb straight through:

	if (rmnet_map_validate_packet_len(skb, data_format))
		__rmnet_map_ingress_handler(skb, port, data_format);

rmnet_rx_handler() only calls skb_linearize(), which does nothing for a
linear skb. Nothing on this path calls skb_share_check(), skb_unshare() or
skb_cow().

Before the rx_handler runs, __netif_receive_skb_core() delivers the skb to
ptype_all taps. packet_rcv() then queues a clone on the AF_PACKET socket,
and that clone shares this data buffer.

Can this rewrite NUM_NLOS, and clear the reserved low bits of coal_info,
in a buffer that an AF_PACKET reader may be copying to user space at the
same time?

Only num_nlos is needed, so could the recount be kept in a local variable
instead of being written back into the header? The deaggregation path and
the ARPHRD_ETHER path do not seem to be affected, because they copy or
expand the head first.

Separately, the earlier patch in this series, "net: qualcomm: rmnet: Add
DL packet coalescing support", already writes tp->check in place through
rmnet_map_partial_csum() in the fast path. Would unsharing the skb before
changing it be the more complete fix?

> +}

[ ... ]

> @@ -913,6 +968,7 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
>  	skb_pull(coal_skb, sizeof(struct rmnet_map_header));
>  	skb_trim(coal_skb, len);
>  	coal_hdr = (struct rmnet_map_v5_coal_header *)coal_skb->data;
> +	rmnet_map_v5_fixup_num_nlos(coal_hdr);

[Severity: Medium]
The commit message says the determination is reliable "regardless of
whether num_nlos itself can be trusted". However,
rmnet_map_process_next_hdr_packet() calls
rmnet_map_data_check_coal_header() before this point, and that function
range-checks the raw hardware value:

	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);

	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS)
		return -EINVAL;

The field is 3 bits wide, so 7 is a possible value. If the hardware
reports num_nlos as 0 or 7, is the whole frame still dropped in
__rmnet_map_ingress_handler(), even when the per-slot num_packets are
valid?

The opposite case also seems uncovered. Suppose the raw value is in
[1, 6] but every num_packets is 0. The recount here is then 0, and nothing
range-checks it:

- rmnet_map_coal_validate_bounds() loops zero times.
- rmnet_map_v5_csum_fixup() and the fast path both return false.
- rmnet_map_coal_segment_loop() emits nothing.

rmnet_map_segment_coal_skb() returns 0, and the caller does:

	if (skb_peek(list) != skb)
		consume_skb(skb);

The frame is consumed with no error counted.

Should the recount run before or inside
rmnet_map_data_check_coal_header(), and should a result of 0 be rejected?

>  	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
>  	skb_pull(coal_skb, sizeof(*coal_hdr));

[Severity: High]
This problem is not in this patch. It comes from the earlier patch in
this series, "net: qualcomm: rmnet: Add DL packet coalescing support". It
is still present here, though, and the new CHECKSUM_NONE branch below only
runs after it.

Right after this point, rmnet_map_coal_parse_ip_hdr() handles IPv6 like
this:

		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
				       &frag_off);
		if (ret < 0 || frag_off)
			return false;

		meta->ip_len = (u16)ret;

ipv6_skip_exthdr() is documented as possibly returning an offset past the
end of the packet if the last recognized header is truncated. It reads
only the 2-byte ipv6_opt_hdr and adds ipv6_optlen(hp), which can be up to
2048.

For example, take a 42-byte IPv6 payload with nexthdr HOP or DEST and an
option header of {nexthdr = TCP, hdrlen = 255}. The call returns 2088.

rmnet_map_coal_parse_trans_hdr() then does:

	avail = coal_skb->len - meta->ip_len;

	if (meta->trans_proto == IPPROTO_TCP) {
		if (avail < sizeof(*th))
			return false;

		th = (struct tcphdr *)base;
		meta->trans_len = th->doff * 4;

Can avail wrap to a huge u32 value here? If so, th->doff is read from
ip_header + 2088, past the end of the packet.

If the pkt_len reported by the hardware is also at least hlen, the check
in rmnet_map_coal_validate_bounds() fails too, because
coal_skb->len - hlen also wraps. The segmentation path would then
memcpy() the IP header, TCP header and payload bytes from out-of-bounds
memory into new skbs that go to the stack.

Would it fix this to reject ret > coal_skb->len before storing ip_len, and
ideally also reject ret plus the minimum transport header length > len?

>  
> @@ -927,6 +983,12 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
>  	if (!rmnet_map_coal_validate_bounds(coal_skb, coal_hdr, num_nlos, hlen))
>  		return -EINVAL;
>  
> +	if (rmnet_map_v5_csum_fixup(coal_hdr) && !coal_meta.zero_csum) {
> +		coal_skb->ip_summed = CHECKSUM_NONE;
> +		__skb_queue_tail(list, coal_skb);
> +		return 0;
> +	}

[Severity: Low]
The commit message says:

  On certain hardware, the checksum valid indication from the bitmap
  inside a MAPv5 coalescing frame cannot be trusted ...

  The hardware sets CSUM_VALID incorrectly in these cases, causing
  the driver to mark packets CHECKSUM_UNNECESSARY ...

Does this mix up two different indications?

In the driver, the frame-level MAPV5_COALINFO_CSUM_VALID_FLAG is used only
by rmnet_map_coal_gro_fast_path(), which sets CHECKSUM_UNNECESSARY.

The per-slot csum_error_bitmap feeds nlo_err_mask in
rmnet_map_coal_segment_loop(). There, flagged packets are dropped in
__rmnet_map_segment_coal_skb(), and the rest become CHECKSUM_PARTIAL
through rmnet_map_partial_csum().

This early return skips both paths for qualifying frames. Packets that the
bitmap marks bad are no longer dropped in the driver. They go to the stack
as CHECKSUM_NONE and are dropped there after verification. They are also
not counted in coal_csum_err or coal_csum_drop, including after the stats
patch later in the series.

Could the commit message say which indication is actually wrong? Could it
also mention that the non-GRO, bitmap-based path changes as well?

> +
>  	if (rmnet_map_coal_gro_fast_path(coal_skb, coal_hdr, &coal_meta, list,
>  					 num_nlos, gro))
>  		return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930051345.857443-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next 6/7] net: qualcomm: rmnet: Add ethtool stats for DL coalescing

[Subash Abhinov Kasiviswanathan]

Track coalescing decode activity with per-device counters and expose
them via ethtool. The counters include the coalescing related packet
counts, coalescing errors and reasons for coalescing completion.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 .../ethernet/qualcomm/rmnet/rmnet_config.h    |  20 ++++
 .../ethernet/qualcomm/rmnet/rmnet_map_data.c  | 110 ++++++++++++++++--
 .../net/ethernet/qualcomm/rmnet/rmnet_vnd.c   |  20 ++++
 3 files changed, 138 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..2a34f8a7b527 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
@@ -80,6 +80,26 @@ struct rmnet_priv_stats {
 	u64 csum_skipped;
 	u64 csum_sw;
 	u64 csum_hw;
+	/* DL coalescing */
+	u64 coal_rx;
+	u64 coal_pkts;
+	u64 coal_hdr_nlo_err;
+	u64 coal_hdr_pkt_err;
+	u64 coal_csum_err;
+	u64 coal_csum_drop;
+	u64 coal_reconstruct;
+	u64 coal_ip_invalid;
+	u64 coal_trans_invalid;
+	/* close-reason sub-counters */
+	u64 coal_close_non_coal;
+	u64 coal_close_ip_miss;
+	u64 coal_close_trans_miss;
+	u64 coal_close_hw_nl;
+	u64 coal_close_hw_pkt;
+	u64 coal_close_hw_byte;
+	u64 coal_close_hw_time;
+	u64 coal_close_hw_evict;
+	u64 coal_close_coal;
 };
 
 struct rmnet_priv {
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
index 1f9e592e24b6..2e76bf5a5a90 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
@@ -658,6 +658,7 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 			     bool csum_valid)
 {
 	u32 dlen = coal_meta->data_len * coal_meta->pkt_count;
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
 	struct sk_buff *skbn;
 
@@ -668,8 +669,10 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	if (!csum_valid && coal_meta->zero_csum)
 		csum_valid = true;
 
-	if (!csum_valid)
+	if (!csum_valid) {
+		priv->stats.coal_csum_drop++;
 		goto next_pkt;
+	}
 
 	skbn = alloc_skb(hlen + dlen + RMNET_MAP_DEAGGR_HEADROOM, GFP_ATOMIC);
 	if (!skbn)
@@ -722,6 +725,7 @@ __rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	rmnet_map_partial_csum(skbn, coal_meta);
 
 	skbn->dev = coal_skb->dev;
+	priv->stats.coal_reconstruct++;
 
 	if (coal_meta->pkt_count > 1)
 		rmnet_map_gso_stamp(skbn, coal_meta);
@@ -740,14 +744,17 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 					struct rmnet_map_coal_metadata *meta,
 					bool *gro)
 {
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	struct ipv6hdr *ip6h;
 	struct iphdr *iph;
 	__be16 frag_off;
 	u8 protocol;
 	int ret;
 
-	if (coal_skb->len < sizeof(*iph))
+	if (coal_skb->len < sizeof(*iph)) {
+		priv->stats.coal_ip_invalid++;
 		return false;
+	}
 
 	iph = (struct iphdr *)coal_skb->data;
 
@@ -756,25 +763,33 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 		meta->ip_len = iph->ihl * 4;
 		meta->trans_proto = iph->protocol;
 		meta->ip_header = iph;
-		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len)
+		if (meta->ip_len < sizeof(*iph) || coal_skb->len < meta->ip_len) {
+			priv->stats.coal_ip_invalid++;
 			return false;
+		}
 
-		if (ip_is_fragment(iph))
+		if (ip_is_fragment(iph)) {
+			priv->stats.coal_ip_invalid++;
 			return false;
+		}
 
 		if (iph->ihl != 5)
 			*gro = false;
 	} else if (iph->version == 6) {
-		if (coal_skb->len < sizeof(*ip6h))
+		if (coal_skb->len < sizeof(*ip6h)) {
+			priv->stats.coal_ip_invalid++;
 			return false;
+		}
 
 		ip6h = (struct ipv6hdr *)iph;
 		protocol = ip6h->nexthdr;
 		meta->ip_proto = 6;
 		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
 				       &frag_off);
-		if (ret < 0 || frag_off)
+		if (ret < 0 || frag_off) {
+			priv->stats.coal_ip_invalid++;
 			return false;
+		}
 
 		meta->ip_len = (u16)ret;
 		meta->trans_proto = protocol;
@@ -782,6 +797,7 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 		if (meta->ip_len > sizeof(*ip6h))
 			*gro = false;
 	} else {
+		priv->stats.coal_ip_invalid++;
 		return false;
 	}
 
@@ -794,6 +810,7 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,
 static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
 					   struct rmnet_map_coal_metadata *meta)
 {
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	struct udphdr *uh;
 	struct tcphdr *th;
 	u32 avail;
@@ -803,17 +820,23 @@ static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
 	avail = coal_skb->len - meta->ip_len;
 
 	if (meta->trans_proto == IPPROTO_TCP) {
-		if (avail < sizeof(*th))
+		if (avail < sizeof(*th)) {
+			priv->stats.coal_trans_invalid++;
 			return false;
+		}
 
 		th = (struct tcphdr *)base;
 		meta->trans_len = th->doff * 4;
 		meta->trans_header = th;
-		if (meta->trans_len < sizeof(*th) || avail < meta->trans_len)
+		if (meta->trans_len < sizeof(*th) || avail < meta->trans_len) {
+			priv->stats.coal_trans_invalid++;
 			return false;
+		}
 	} else if (meta->trans_proto == IPPROTO_UDP) {
-		if (avail < sizeof(*uh))
+		if (avail < sizeof(*uh)) {
+			priv->stats.coal_trans_invalid++;
 			return false;
+		}
 
 		uh = (struct udphdr *)base;
 		meta->trans_len = sizeof(*uh);
@@ -821,6 +844,7 @@ static bool rmnet_map_coal_parse_trans_hdr(struct sk_buff *coal_skb,
 		if (meta->ip_proto == 4 && !uh->check)
 			meta->zero_csum = true;
 	} else {
+		priv->stats.coal_trans_invalid++;
 		return false;
 	}
 
@@ -896,6 +920,7 @@ static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
 					struct sk_buff_head *list,
 					u64 nlo_err_mask, bool gro, u8 num_nlos)
 {
+	struct rmnet_priv *priv = netdev_priv(coal_skb->dev);
 	u32 hlen = coal_meta->ip_len + coal_meta->trans_len;
 	u8 pkt, total_pkt = 0;
 	bool csum_err;
@@ -916,6 +941,9 @@ static void rmnet_map_coal_segment_loop(struct sk_buff *coal_skb,
 		     pkt++, total_pkt++, nlo_err_mask >>= 1) {
 			csum_err = nlo_err_mask & 1;
 
+			if (csum_err)
+				priv->stats.coal_csum_err++;
+
 			if (!gro) {
 				coal_meta->pkt_count = 1;
 				__rmnet_map_segment_coal_skb(coal_skb, coal_meta,
@@ -999,6 +1027,49 @@ static int rmnet_map_segment_coal_skb(struct sk_buff *coal_skb,
 	return 0;
 }
 
+/* Log the hardware close-reason counter for a coalescing header. */
+static void rmnet_map_data_log_close_stats(struct rmnet_priv *priv,
+					   u8 type, u8 code)
+{
+	switch (type) {
+	case RMNET_MAP_COAL_CLOSE_NON_COAL:
+		priv->stats.coal_close_non_coal++;
+		break;
+	case RMNET_MAP_COAL_CLOSE_IP_MISS:
+		priv->stats.coal_close_ip_miss++;
+		break;
+	case RMNET_MAP_COAL_CLOSE_TRANS_MISS:
+		priv->stats.coal_close_trans_miss++;
+		break;
+	case RMNET_MAP_COAL_CLOSE_HW:
+		switch (code) {
+		case RMNET_MAP_COAL_CLOSE_HW_NL:
+			priv->stats.coal_close_hw_nl++;
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_PKT:
+			priv->stats.coal_close_hw_pkt++;
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_BYTE:
+			priv->stats.coal_close_hw_byte++;
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_TIME:
+			priv->stats.coal_close_hw_time++;
+			break;
+		case RMNET_MAP_COAL_CLOSE_HW_EVICT:
+			priv->stats.coal_close_hw_evict++;
+			break;
+		default:
+			break;
+		}
+		break;
+	case RMNET_MAP_COAL_CLOSE_COAL:
+		priv->stats.coal_close_coal++;
+		break;
+	default:
+		break;
+	}
+}
+
 /* Validate the coalescing header and build the checksum error mask.
  *
  * Checks performed:
@@ -1023,20 +1094,25 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
 					    u64 *nlo_err_mask)
 {
 	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
+	struct rmnet_priv *priv = netdev_priv(skb->dev);
 	struct rmnet_map_v5_coal_header *coal_hdr;
 	u8 num_nlos, pkts = 0;
 	u64 mask = 0;
 	int i;
 
 	/* coal header is counted in pkt_len */
-	if (ntohs(maph->pkt_len) < sizeof(*coal_hdr))
+	if (ntohs(maph->pkt_len) < sizeof(*coal_hdr)) {
+		priv->stats.coal_hdr_nlo_err++;
 		return -EINVAL;
+	}
 
 	coal_hdr = (struct rmnet_map_v5_coal_header *)(skb->data + sizeof(*maph));
 	num_nlos = u8_get_bits(coal_hdr->coal_info, MAPV5_COALINFO_NUM_NLOS_FMASK);
 
-	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS)
+	if (num_nlos == 0 || num_nlos > RMNET_MAP_V5_MAX_NLOS) {
+		priv->stats.coal_hdr_nlo_err++;
 		return -EINVAL;
+	}
 
 	for (i = 0; i < RMNET_MAP_V5_MAX_NLOS; i++) {
 		u8 err = coal_hdr->nl_pairs[i].csum_error_bitmap;
@@ -1044,10 +1120,19 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
 
 		mask |= ((u64)err) << (8 * i);
 		pkts += pkt;
-		if (pkts > RMNET_MAP_V5_MAX_PACKETS)
+		if (pkts > RMNET_MAP_V5_MAX_PACKETS) {
+			priv->stats.coal_hdr_pkt_err++;
 			return -EINVAL;
+		}
 	}
 
+	priv->stats.coal_pkts += pkts;
+	rmnet_map_data_log_close_stats(priv,
+				       u8_get_bits(coal_hdr->close_info,
+						   MAPV5_CLOSEINFO_CLOSE_TYPE_FMASK),
+				       u8_get_bits(coal_hdr->close_info,
+						   MAPV5_CLOSEINFO_CLOSE_VALUE_FMASK));
+
 	*nlo_err_mask = mask;
 	return 0;
 }
@@ -1065,6 +1150,7 @@ int rmnet_map_process_next_hdr_packet(struct sk_buff *skb,
 		if (!(data_format & RMNET_FLAGS_INGRESS_COALESCE))
 			return -EINVAL;
 
+		priv->stats.coal_rx++;
 		rc = rmnet_map_data_check_coal_header(skb, &nlo_err_mask);
 		if (rc)
 			return rc;
diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
index e1e319683a55..a21d6543c014 100644
--- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
+++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
@@ -183,6 +183,26 @@ static const char rmnet_gstrings_stats[][ETH_GSTRING_LEN] = {
 	"Checksum skipped",
 	"Checksum computed in software",
 	"Checksum computed in hardware",
+	/* DL coalescing */
+	"Coal frames received",
+	"Packets in coal frames",
+	"Coal hdr NLO errors",
+	"Coal hdr pkt count errors",
+	"Coal checksum errors",
+	"Coal packets dropped on csum err",
+	"Coal segments reconstructed",
+	"Coal invalid IP version",
+	"Coal invalid transport",
+	/* close reasons */
+	"Coal closed: non-coal",
+	"Coal closed: IP miss",
+	"Coal closed: transport miss",
+	"Coal closed: hw NL limit",
+	"Coal closed: hw pkt limit",
+	"Coal closed: hw byte limit",
+	"Coal closed: hw time limit",
+	"Coal closed: hw evict",
+	"Coal closed: FIN/PSH",
 };
 
 static void rmnet_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
-- 
2.34.1

Re: [PATCH net-next 6/7] net: qualcomm: rmnet: Add ethtool stats for DL coalescing

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 3 · 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).

- [High] A malformed IPv6 extension-header length can push the parsed
  header length (ip_len) past the end of the received packet.
- [Medium] In rmnet_map_data_check_coal_header()
  (drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c), the per-NLO…
- [Medium] Some of the new ethtool -S counters duplicate statistics that
  already have a standard uAPI.
- [Medium] The 19 new coalescing counters are plain u64 fields in the
  single per-device `struct rmnet_priv_stats priv->stats`.
- [Low] Some ethtool counter labels do not match what the counters
  measure.

--

net: qualcomm: rmnet: Add ethtool stats for DL coalescing

This commit adds per-device counters to struct rmnet_priv_stats. They
track DL coalescing decode activity, header and checksum errors, and
hardware close reasons, and are exposed through ethtool -S.

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
> index 5adda0323dda7..2a34f8a7b5276 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.h
> @@ -80,6 +80,26 @@ struct rmnet_priv_stats {
>  	u64 csum_skipped;
>  	u64 csum_sw;
>  	u64 csum_hw;
> +	/* DL coalescing */
> +	u64 coal_rx;
> +	u64 coal_pkts;

[Severity: Medium]
These new counters are plain u64 fields in the single per-device
priv->stats. The RX softirq path updates them with non-atomic
read-modify-write operations, for example:

rmnet_map_process_next_hdr_packet()
    priv->stats.coal_rx++;

rmnet_map_data_check_coal_header()
    priv->stats.coal_pkts += pkts;

__rmnet_map_segment_coal_skb()
    priv->stats.coal_reconstruct++;

The reader in rmnet_get_ethtool_stats() copies them with a plain memcpy():

    memcpy(data, st, ARRAY_SIZE(rmnet_gstrings_stats) * sizeof(u64));

On 32-bit kernels, can ethtool -S see torn 64-bit values while RX softirq
on another CPU is updating them?

If the real device delivers frames for the same mux_id on more than one
CPU (multi-queue NAPI or RPS), can concurrent increments also be lost?

The existing csum_* counters use the same pattern, but this patch adds
more hot path counters to it. The driver already has struct
rmnet_pcpu_stats with a u64_stats_sync. Would per-CPU counters using
u64_stats_update_begin() and u64_stats_fetch_begin() fit better here?

> +	u64 coal_hdr_nlo_err;
> +	u64 coal_hdr_pkt_err;

[ ... ]

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index 1f9e592e24b65..2e76bf5a5a90c 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c

[ ... ]

> @@ -756,25 +763,33 @@ static bool rmnet_map_coal_parse_ip_hdr(struct sk_buff *coal_skb,

[ ... ]

>  	} else if (iph->version == 6) {
> -		if (coal_skb->len < sizeof(*ip6h))
> +		if (coal_skb->len < sizeof(*ip6h)) {
> +			priv->stats.coal_ip_invalid++;
>  			return false;
> +		}
>  
>  		ip6h = (struct ipv6hdr *)iph;
>  		protocol = ip6h->nexthdr;
>  		meta->ip_proto = 6;
>  		ret = ipv6_skip_exthdr(coal_skb, sizeof(*ip6h), &protocol,
>  				       &frag_off);
> -		if (ret < 0 || frag_off)
> +		if (ret < 0 || frag_off) {
> +			priv->stats.coal_ip_invalid++;
>  			return false;
> +		}
>  
>  		meta->ip_len = (u16)ret;
>  		^^^^

[Severity: High]
This code came in with the earlier commit "net: qualcomm: rmnet: Add DL
packet coalescing support" in this series. It is still present at the
end of the series.

Is meta->ip_len ever checked against coal_skb->len on the IPv6 path in
rmnet_map_coal_parse_ip_hdr()?

ipv6_skip_exthdr() only reads the first two bytes of each extension header
through skb_header_pointer(). It then adds ipv6_optlen() (up to 2048) to
start and returns that value. It never checks that the last extension
header fits in the skb.

For example, take a 48 byte IPv6 packet with a Destination Options header
that has hdrlen=255 and nexthdr=UDP. That gives ip_len = 2088.

rmnet_map_coal_parse_trans_hdr() then does:

    base = (u8 *)meta->ip_header + meta->ip_len;
    avail = coal_skb->len - meta->ip_len;

Here avail wraps to a large u32, so the avail < sizeof(*th) and
avail < sizeof(*uh) checks pass. For TCP, th->doff would then be read
from about 2KB past the data.

rmnet_map_coal_validate_bounds() has the same underflow:

    if (total_data + nlo_len > coal_skb->len - hlen)
        return false;

So an NLO pkt_len just above hlen passes. Because ip_len > 40 sets
gro = false, the fast path is skipped. With a zero checksum error bitmap
and a close type that rmnet_map_v5_csum_fixup() does not catch (such as
NON_COAL), rmnet_map_coal_segment_loop() calls
__rmnet_map_segment_coal_skb(), which does:

    memcpy(skbn->data, coal_meta->ip_header, coal_meta->ip_len);

The earlier skb_put_data() and transport header copies there also read
past the buffer that rmnet_map_deaggregate() allocated.

Can this copy about 2KB of adjacent slab memory into an skb that is
marked CHECKSUM_PARTIAL and passed up the IPv6 stack? Could the stack
then send part of it back out, for example in an ICMPv6 parameter problem
error for unknown TLV options?

The full copy needs an NLO pkt_len larger than the real data, which means
buggy or compromised modem firmware. The out-of-bounds th->doff read in
rmnet_map_coal_parse_trans_hdr() only needs a truncated IPv6/TCP packet
inside a coalescing frame.

>  		meta->trans_proto = protocol;

[ ... ]

> @@ -1023,20 +1094,25 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
>  					    u64 *nlo_err_mask)
>  {
>  	struct rmnet_map_header *maph = (struct rmnet_map_header *)skb->data;
> +	struct rmnet_priv *priv = netdev_priv(skb->dev);
>  	struct rmnet_map_v5_coal_header *coal_hdr;
>  	u8 num_nlos, pkts = 0;

[ ... ]

> @@ -1044,10 +1120,19 @@ static int rmnet_map_data_check_coal_header(struct sk_buff *skb,
>  
>  		mask |= ((u64)err) << (8 * i);
>  		pkts += pkt;
> -		if (pkts > RMNET_MAP_V5_MAX_PACKETS)
> +		if (pkts > RMNET_MAP_V5_MAX_PACKETS) {
> +			priv->stats.coal_hdr_pkt_err++;
>  			return -EINVAL;
> +		}
>  	}
>  
> +	priv->stats.coal_pkts += pkts;

[Severity: Medium]
pkts is a u8, and each nl_pairs[i].num_packets is a u8 supplied by the
device. Can the running sum in pkts += pkt wrap and bypass the
RMNET_MAP_V5_MAX_PACKETS check?

For example, num_packets values of 48, 255, 255, 255, 255, 255 give
running sums of 48, 47, 46, 45, 44 and 43. The check never fires, but the
frame claims 1323 packets.

rmnet_map_coal_validate_bounds() only limits (plen - hlen) * num_packets,
and that is zero when plen == hlen. So it doesn't catch this either.

rmnet_map_coal_segment_loop() then walks every claimed packet. In the
non-GRO path it allocates a GFP_ATOMIC skb for each one. The u8 total_pkt
used for the IPv4 ID adjustment wraps as well.

The new coal_pkts counter above also records the wrapped value.

The u8 accumulator came in with the earlier commit "net: qualcomm: rmnet:
Add DL packet coalescing support" in this series. It is still present at
the end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> index e1e319683a558..a21d6543c014f 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_vnd.c
> @@ -183,6 +183,26 @@ static const char rmnet_gstrings_stats[][ETH_GSTRING_LEN] = {
>  	"Checksum skipped",
>  	"Checksum computed in software",
>  	"Checksum computed in hardware",
> +	/* DL coalescing */
> +	"Coal frames received",
> +	"Packets in coal frames",
> +	"Coal hdr NLO errors",
> +	"Coal hdr pkt count errors",
> +	"Coal checksum errors",
> +	"Coal packets dropped on csum err",

[Severity: Medium]
Do some of these duplicate counters that already have a standard uAPI?

"Coal frames received" (coal_rx) and "Packets in coal frames" (coal_pkts)
look like the qstats rx-hw-gro-packets and rx-hw-gro-wire-packets
counters (hw_gro_packets and hw_gro_wire_packets in struct
netdev_queue_stats_rx). rmnet advertises NETIF_F_GRO_HW and uses it to
gate the coalesced delivery path in rmnet_map_segment_coal_skb().

"Coal packets dropped on csum err" (coal_csum_drop) is an RX drop, which
the standard drop counters already cover. "Coal checksum errors"
(coal_csum_err) partly overlaps rx-csum-bad.

The networking driver guidance limits ethtool -S to counters that have no
standard representation, such as the close reasons and the header NLO and
packet count errors here.

Could these be reported through netdev qstats and the standard drop
counters instead?

> +	"Coal segments reconstructed",
> +	"Coal invalid IP version",

[Severity: Low]
Does this label match what coal_ip_invalid counts?
rmnet_map_coal_parse_ip_hdr() increments it for:

- a truncated IPv4 or IPv6 header
- a bad IHL
- an IPv4 fragment
- an ipv6_skip_exthdr() failure or an IPv6 fragment
- an unknown version

Only the last one is an invalid IP version.

Similarly, rmnet_map_data_check_coal_header() also bumps "Coal hdr NLO
errors" when the MAP pkt_len is too short:

    if (ntohs(maph->pkt_len) < sizeof(*coal_hdr)) {
        priv->stats.coal_hdr_nlo_err++;

That is a length error, not an NLO count error.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930051345.857443-1-subash.a.kasiviswanathan%40oss.qualcomm.com

[PATCH net-next 7/7] docs: networking: Add documentation for the coalescing support in rmnet

[Subash Abhinov Kasiviswanathan]

Add information about the MAPv5 coalescing header covering the layout
and the information from the fields in the header.

Co-developed-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Sean Tranchetti <sean.tranchetti@oss.qualcomm.com>
Signed-off-by: Subash Abhinov Kasiviswanathan <subash.a.kasiviswanathan@oss.qualcomm.com>
---
 .../cellular/qualcomm/rmnet.rst               | 115 ++++++++++++++++--
 1 file changed, 107 insertions(+), 8 deletions(-)

diff --git a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
index 5aedbabb7382..ba8e947d54e6 100644
--- a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
+++ b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
@@ -125,8 +125,8 @@ Command (1)/ Data (0) bit value is to indicate if the packet is a MAP command
 or data packet. Command packet is used for transport level flow control. Data
 packets are standard IP packets.
 
-Next header is used to indicate the presence of another header, currently is
-limited to checksum header.
+Next header is used to indicate the presence of another header, currently
+limited to the checksum and coalescing headers.
 
 Padding is the number of bytes to be appended to the payload to
 ensure 4 byte alignment.
@@ -150,11 +150,11 @@ Header Type is to indicate the type of header, this usually is set to CHECKSUM
 
 Header types
 
-= ===============
+= ======================
 0 Reserved
-1 Reserved
+1 coalescing header
 2 checksum header
-= ===============
+= ======================
 
 Checksum Valid is to indicate whether the header checksum is valid. Value of 1
 implies that checksum is calculated on this packet and is valid, value of 0
@@ -162,8 +162,78 @@ indicates that the calculated packet checksum is invalid.
 
 Reserved bits must be zero when sent and ignored when received.
 
-e. MAP packet v1/v5 (command specific)
---------------------------------------
+e. Coalescing header v5
+------------------------
+
+Hardware can coalesce multiple same-flow IP packets of the same length into
+a single MAP frame to reduce per-packet overhead at high data rates.  The
+coalescing header (header type 1) describes the coalesced content.
+
+Packet format::
+
+  Bit         0 - 6         7          8           9-11         12-15
+  Function  Header Type  Next Header  CSUM valid  Num NLOs    (reserved)
+
+  Bit        16-19        20-23
+  Function  Close value  Close type
+
+  Bit        24-27        28-31
+  Function  (reserved)    VEID
+
+  Bit           32 - 47        48 - 55            56 - 63
+  Function   Packet length  CSUM error bitmap  Num packets (NLO 0)
+
+  ... (up to 6 NLO entries total, same 32-bit format per entry)
+
+Header Type is set to 1 (coalescing).
+
+Num NLOs (Number-Length Objects) is the count of active NLO entries
+(1 – 6).  Each NLO describes a group of consecutive coalesced packets
+that all share the same IP packet length.
+
+CSUM valid (bit 8) indicates whether the hardware checksum is valid
+for all packets in the frame.
+
+Close type and close value encode the hardware reason that coalescing
+was terminated for this frame:
+
+Close type values:
+
+= ==============================
+0 non-coalesced (single packet)
+1 IP flow miss
+2 transport flow miss
+3 hardware limit (see value)
+4 coalescing closed (FIN/PSH)
+= ==============================
+
+Close value (used when close type is 3):
+
+= ==================
+0 NL limit reached
+1 packet limit
+2 byte limit
+3 time limit
+4 eviction
+= ==================
+
+VEID is the virtual endpoint ID of the originating flow.
+
+Each NLO entry::
+
+  Bit         0 - 15        16 - 23            24 - 31
+  Function  Pkt length   CSUM error bitmap   Num packets
+
+Pkt length is the full IP packet length, including the IP header,
+transport header, and payload, for every packet in this NLO group.
+
+CSUM error bitmap is a per-packet bitmask. Bit N is set when packet N
+in this NLO has a bad checksum.
+
+Num packets is the count of coalesced packets described by this NLO.
+
+f. MAP packet v1/v5 (command specific)
+---------------------------------------
 
 Packet format::
 
@@ -187,7 +257,7 @@ Command types
 3 is for error during processing of commands
 = ==========================================
 
-f. Aggregation
+g. Aggregation
 --------------
 
 Aggregation is multiple MAP packets (can be data or command) delivered to
@@ -208,3 +278,32 @@ rmnet userspace configuration is done through netlink using iproute2
 https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/
 
 The driver uses rtnl_link_ops for communication.
+
+The data format flags controlling the ingress and egress processing
+pipeline are set via the ``IFLA_RMNET_FLAGS`` attribute
+(``struct ifla_rmnet_flags``).
+
+Relevant ingress flags:
+
+``RMNET_FLAGS_INGRESS_DEAGGREGATION``
+  Enable MAP frame de-aggregation.
+
+``RMNET_FLAGS_INGRESS_MAP_CKSUMV4``
+  Enable MAPv4 downlink checksum offload.
+
+``RMNET_FLAGS_INGRESS_MAP_CKSUMV5``
+  Enable MAPv5 downlink checksum offload (header type 2).
+
+``RMNET_FLAGS_INGRESS_COALESCE``
+  Enable MAPv5 downlink hardware coalescing (header type 1).
+  When set the driver will decode coalescing headers, reconstruct
+  individual IP packets and will deliver batched GSO SKBs to the
+  stack for efficient processing.
+
+Relevant egress flags:
+
+``RMNET_FLAGS_EGRESS_MAP_CKSUMV4``
+  Enable MAPv4 uplink checksum offload.
+
+``RMNET_FLAGS_EGRESS_MAP_CKSUMV5``
+  Enable MAPv5 uplink checksum offload.
-- 
2.34.1

Re: [PATCH net-next 7/7] docs: networking: Add documentation for the coalescing support in rmnet

[netdev-bot+sashiko]

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

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

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] rmnet.rst gets the MAPv5 coalescing checksum error bitmap
  wrong.
- [Low] The opening sentence of the new "e.
- [Low] The patch adds a "data format flags" section under "3.
- [Low] The new doc treats Num NLOs and CSUM valid as reliable.
- [Low] The layout diagram in rmnet.rst ends with "... (up to 6 NLO
  entries total, same 32-bit format per entry)" and then gives Num NLOs…

--

docs: networking: Add documentation for the coalescing support in rmnet

This patch documents the MAPv5 coalescing header (header type 1) in
rmnet.rst. It covers the header layout, the close type and close value
encodings, and the per-NLO entry format. It also adds a section on the
IFLA_RMNET_FLAGS data format flags, including RMNET_FLAGS_INGRESS_COALESCE.

> diff --git a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
> index 5aedbabb73827..ba8e947d54e68 100644
> --- a/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst
> +++ b/Documentation/networking/device_drivers/cellular/qualcomm/rmnet.rst

[ ... ]

> @@ -162,8 +162,78 @@ indicates that the calculated packet checksum is invalid.
>  
>  Reserved bits must be zero when sent and ignored when received.
>  
> -e. MAP packet v1/v5 (command specific)
> ---------------------------------------
> +e. Coalescing header v5
> +------------------------
> +
> +Hardware can coalesce multiple same-flow IP packets of the same length into
> +a single MAP frame to reduce per-packet overhead at high data rates.  The
> +coalescing header (header type 1) describes the coalesced content.

[Severity: Low]
Is "of the same length" accurate for the whole frame? Each of the up to
six NLO slots has its own pkt_len in struct rmnet_map_v5_nl_pair, and
rmnet_map_coal_segment_loop() reads a separate length for each NLO:

    pkt_len = ntohs(coal_hdr->nl_pairs[nlo].pkt_len);

The DL coalescing commit in this series also says "hardware can coalesce
up to 6 different sizes within a single IP packet".

Only the packets inside one NLO share a length, and the Num NLOs paragraph
further down says exactly that. Could the intro be reworded so the two
paragraphs agree?

> +
> +Packet format::
> +
> +  Bit         0 - 6         7          8           9-11         12-15
> +  Function  Header Type  Next Header  CSUM valid  Num NLOs    (reserved)
> +
> +  Bit        16-19        20-23
> +  Function  Close value  Close type
> +
> +  Bit        24-27        28-31
> +  Function  (reserved)    VEID
> +
> +  Bit           32 - 47        48 - 55            56 - 63
> +  Function   Packet length  CSUM error bitmap  Num packets (NLO 0)
> +
> +  ... (up to 6 NLO entries total, same 32-bit format per entry)

[Severity: Low]
Could this wording suggest that the header length changes with Num NLOs?
The driver treats the coalescing header as a fixed 28-byte structure, with
all six slots always present:

include/linux/if_rmnet.h:
    struct rmnet_map_v5_coal_header {
        ...
        struct rmnet_map_v5_nl_pair nl_pairs[RMNET_MAP_V5_MAX_NLOS];
    } __aligned(1);

rmnet_map_segment_coal_skb() always pulls the full header, whatever Num
NLOs says:

    skb_pull(coal_skb, sizeof(*coal_hdr));

rmnet_map_data_check_coal_header() also reads all six slots. It requires
ntohs(maph->pkt_len) >= sizeof(*coal_hdr), because the coalescing header
is counted in the MAP pkt_len.

Suppose a decoder sizes the header as 4 + 4 * Num NLOs. It would start
parsing the IP packet at the wrong offset whenever fewer than six NLOs are
active.

Could the text say that all six slots are always present, that unused
slots are zero, and that the header is counted in the MAP header's
pkt_len?

> +
> +Header Type is set to 1 (coalescing).
> +
> +Num NLOs (Number-Length Objects) is the count of active NLO entries
> +(1 – 6).  Each NLO describes a group of consecutive coalesced packets
> +that all share the same IP packet length.
> +
> +CSUM valid (bit 8) indicates whether the hardware checksum is valid
> +for all packets in the frame.

[Severity: Low]
Should these two fields mention the hardware quirks that the driver in
this series works around? rmnet_map_v5_fixup_num_nlos() says:

    /* On some hardware, num_nlos in the coalescing header can be reported
     * incorrectly under certain conditions even though the per-NLO num_packets
     * fields it is meant to summarize are correct. ...

It then recomputes the count from the non-zero nl_pairs[].num_packets
slots. That only works if unused slots have num_packets == 0, and the doc
does not state that requirement. According to the "net: qualcomm: rmnet:
Work around coalescing hardware quirks" commit message, the num_nlos
misreporting was seen on simulation hardware.

rmnet_map_v5_csum_fixup() has a similar note:

    /* The checksum valid indication for a single NLO, single packet coalescing
     * frame cannot be trusted when the close reason is a TCP FIN/PSH, a packet
     * count limit, a byte count limit or a time limit.
     */

In those cases rmnet_map_segment_coal_skb() forces CHECKSUM_NONE. The doc
says CSUM valid applies to all packets in the frame and gives none of
these caveats. A receiver written from this text would trust checksums
that the driver deliberately refuses to trust.

[ ... ]

> +Each NLO entry::
> +
> +  Bit         0 - 15        16 - 23            24 - 31
> +  Function  Pkt length   CSUM error bitmap   Num packets
> +
> +Pkt length is the full IP packet length, including the IP header,
> +transport header, and payload, for every packet in this NLO group.
> +
> +CSUM error bitmap is a per-packet bitmask. Bit N is set when packet N
> +in this NLO has a bad checksum.

[Severity: Medium]
Does this match how the driver decodes the bitmap? The comment above
rmnet_map_data_check_coal_header() describes the opposite layout:

 * nlo_err_mask is NOT six independent per-NLO bitmaps. Each nl_pairs
 * slot only has room for an 8 bit csum_error_bitmap, but a single NLO
 * can carry more than 8 packets (up to RMNET_MAP_V5_MAX_PACKETS), so
 * hardware spills a wide NLO's error bits into the csum_error_bitmap
 * bytes of the following slots rather than truncating them. The
 * six bitmap bytes are therefore always concatenated in slot order
 * into one flat RMNET_MAP_V5_MAX_NLOS * 8 = RMNET_MAP_V5_MAX_PACKETS
 * bit value, addressed by a packet's absolute position in the frame,
 * regardless of how many NLOs are actually in use. ...

The code joins all six bytes in slot order, whatever num_nlos is:

    mask |= ((u64)err) << (8 * i);

rmnet_map_coal_segment_loop() then consumes one bit per packet, starting
from the LSB, in absolute frame order. It never re-aligns at an NLO
boundary:

    pkt++, total_pkt++, nlo_err_mask >>= 1) {
        csum_err = nlo_err_mask & 1;

The two readings disagree for most multi-NLO frames. Take a frame where
NLO 0 has 3 packets. The driver maps the first packet of NLO 1 to bit 3 of
slot 0's byte, but the doc maps it to bit 0 of slot 1's byte. An 8-bit
per-NLO field also cannot describe an NLO with more than 8 packets.

The bit order is not stated either. The other diagrams in this section
number bits MSB-first: Header Type is bits 0-6 and Next Header is bit 7,
which matches MAPV5_HDRINFO_HDR_TYPE_FMASK being GENMASK(7, 1) and the
next header flag being BIT(0). A reader would probably take "Bit N" as
MSB-first, but the driver maps packet 0 to the LSB.

Could this paragraph describe the bitmap as a single 48-bit stream across
all six slots? It would be indexed by the packet's position in the frame,
with packet 0 in the LSB of slot 0's byte.

[ ... ]

> @@ -208,3 +278,32 @@ rmnet userspace configuration is done through netlink using iproute2
>  https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/
>  
>  The driver uses rtnl_link_ops for communication.
> +
> +The data format flags controlling the ingress and egress processing
> +pipeline are set via the ``IFLA_RMNET_FLAGS`` attribute
> +(``struct ifla_rmnet_flags``).

[ ... ]

> +``RMNET_FLAGS_INGRESS_COALESCE``
> +  Enable MAPv5 downlink hardware coalescing (header type 1).
> +  When set the driver will decode coalescing headers, reconstruct
> +  individual IP packets and will deliver batched GSO SKBs to the
> +  stack for efficient processing.

[Severity: Low]
Can this flag be used on its own, as described here?
rmnet_config_data_format_valid() rejects it unless the v5 ingress checksum
flag is also set. It also rejects any mix of v4 and v5 checksum flags:

    if ((data_format & v4_mask) && (data_format & v5_mask))
        return false;

    if ((data_format & RMNET_FLAGS_INGRESS_COALESCE) &&
        !(data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5))
        return false;

When this check fails, rmnet_newlink() and rmnet_changelink() return
-EINVAL with the extack "unsupported MAP checksum flag combination".
rmnet_newlink() starts with only RMNET_FLAGS_INGRESS_DEAGGREGATION set, so
a link that asks for just RMNET_FLAGS_INGRESS_COALESCE is rejected.

Would it help to document both of these rules?

The "batched GSO SKBs" part also seems to depend on a feature bit that is
off by default. rmnet_map_segment_coal_skb() checks dev->features:

    bool gro = coal_skb->dev->features & NETIF_F_GRO_HW;

However, rmnet_vnd_newlink() only adds the bit to hw_features:

    rmnet_dev->hw_features |= NETIF_F_GRO_HW;

Nothing in rmnet sets NETIF_F_GRO_HW in dev->features. Unless rx-gro-hw
is enabled by hand, rmnet_map_coal_segment_loop() passes each packet up
as its own non-GSO skb.

Separately, the commit message only covers the coalescing header layout
and its fields. Could it also mention this new IFLA_RMNET_FLAGS section?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930051345.857443-1-subash.a.kasiviswanathan%40oss.qualcomm.com