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

[netdev-bot+sashiko@kernel.org]

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