Re: [PATCH net-next v3 4/8] net: qualcomm: rmnet: Restrict supported MAP checksum configurations

[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] The commit message presents the new check in
  rmnet_config_data_format_valid()…

--

net: qualcomm: rmnet: Restrict supported MAP checksum configurations

This commit adds rmnet_config_data_format_valid(). It is called from
rmnet_newlink() and rmnet_changelink() to reject any data format that sets
both a v4 and a v5 MAP checksum offload flag, in either direction. The
commit message presents this as preparation for a following coalescing
patch.

> 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, regardless of direction. This
> is in preparation for the next patch where coalescing support needs to be
> allowed with MAPv5 format only.

[Severity: Low]
Is this only preparation, or does it also fix a regression in the current
code? Before this patch, IFLA_RMNET_FLAGS could set both the v4 and v5
checksum flags through rmnet_newlink() or rmnet_changelink(). The datapath
does not handle that state consistently.

On egress, with both RMNET_FLAGS_EGRESS_MAP_CKSUMV4 and
RMNET_FLAGS_EGRESS_MAP_CKSUMV5 set, rmnet_map_egress_handler() picks v4
first:

drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:rmnet_map_egress_handler() {
    ...
	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4) {
		additional_header_len = sizeof(struct rmnet_map_ul_csum_header);
		csum_type = RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
	} else if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
    ...
}

However, rmnet_map_add_map_header() still sets the next header flag:

drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c:rmnet_map_add_map_header() {
    ...
	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5)
		map_header->flags |= MAP_NEXT_HEADER_FLAG;
    ...
}

The MAP header then says a v5 next header follows, but a v4 UL checksum
header is what actually follows. Would the modem receive malformed frames
in that case?

On ingress, with both ingress flags set, rmnet_map_validate_packet_len()
checks v4 first and adds the v4 DL trailer length.
__rmnet_map_ingress_handler() checks v5 first:

drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:__rmnet_map_ingress_handler() {
    ...
	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
    ...
}

So the length check and the parser disagree on how the frame is laid out.

This patch closes off those states. Should the commit message describe it
as a fix and include a Fixes: tag, so stable maintainers can consider a
backport?

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