Re: [PATCH net-next 1/3] net: rmnet: annotate data-races around port->data_format

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

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

- [Medium] Even with a single per-packet snapshot of port->data_format,
  the individual stages still resolve the checksum-version bits…
- [Low] The commit message of "net: rmnet: annotate data-races around
  port->data_format" documents concrete, user-visible wire-format…

Pre-existing issues:
- [Medium] port->rmnet_mode and port->bridge_ep are written with plain
  stores under RTNL (rmnet_newlink() at rmnet_config.c:163-164,…
- [Medium] rmnet_register_real_device() publishes the zero-initialized
  struct rmnet_port through netdev_rx_handler_register()…

--

net: rmnet: annotate data-races around port->data_format

This adds READ_ONCE()/WRITE_ONCE() annotations around
port->data_format, which is written under RTNL from rmnet_newlink() and
rmnet_changelink() but read from the RX and TX data paths without any
lock. The value is now sampled once per packet in
rmnet_map_ingress_handler() and rmnet_map_egress_handler() and passed
down to the MAP helpers instead of being re-read by each of them.

A couple of questions below about the commit message and about the
remaining asymmetry between the stages that consume the sampled value.

[Severity: Low]
The changelog describes concrete wire-format failures rather than just a
KCSAN annotation cleanup:

so a concurrent rmnet_changelink() could produce
a MAP header announcing a v5 csum header that was neither reserved
nor written.

and

rmnet_map_send_ack() could trim a dl csum
trailer that the ingress path never accounted for.

Should this carry a Fixes: tag then?  Candidates would be
b6e5d27e32ef60 ("net: ethernet: rmnet: Add support for MAPv5 egress
packets"), which added the second read of port->data_format in
rmnet_map_add_map_header(), and 14452ca3b5ce ("net: qualcomm: rmnet:
Export mux_id and flags to netlink"), which made the flags runtime
configurable.

Without a tag, stable tooling will not pick this up.  If the intent is
net-next hardening instead (the third patch in the series,
"net: rmnet: no longer rely on RTNL in rmnet_fill_info()", is clearly a
refactor), would it be better to word the two paragraphs above as
theoretical rather than as observed packet corruption?

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but the caller of
rmnet_map_ingress_handler() reads two more fields of the same struct
with plain loads while RTNL writers store them plainly:

drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c:rmnet_rx_handler() {
	switch (port->rmnet_mode) {
	case RMNET_EPMODE_VND:
		rmnet_map_ingress_handler(skb, port);
		break;
	case RMNET_EPMODE_BRIDGE:
		rmnet_bridge_handler(skb, port->bridge_ep);
		break;
	}

done:
	return RX_HANDLER_CONSUMED;
}

The writers update the pair in two separate stores, in opposite orders.
rmnet_add_bridge() does:

	port->rmnet_mode = RMNET_EPMODE_BRIDGE;
	port->bridge_ep = slave_dev;

while rmnet_unregister_bridge() does:

	port->bridge_ep = NULL;
	port->rmnet_mode = RMNET_EPMODE_VND;

Can a concurrent receive observe rmnet_mode == RMNET_EPMODE_BRIDGE
together with bridge_ep == NULL?  rmnet_bridge_handler() only acts
inside "if (bridge_dev)", yet rmnet_rx_handler() still returns
RX_HANDLER_CONSUMED, so does the skb leak in that window?

Since the RX path being annotated here is the same one that reads these
fields, would it make sense to annotate rmnet_mode and bridge_ep in the
same series?  They are still plain accesses at the end of the series
(94ca9c4e3244).

[Severity: Medium]
This is a pre-existing issue, but it is closely related to the note in
the changelog that "the rx_handler is live and traffic can already be
flowing".  rmnet_register_real_device() publishes the zero-filled port
through the rx_handler before any receive-visible state is set:

drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_register_real_device() {
	port = kzalloc_obj(*port);
	...
	rc = netdev_rx_handler_register(real_dev, rmnet_rx_handler, port);
	...
	for (entry = 0; entry < RMNET_MAX_LOGICAL_EP; entry++)
		INIT_HLIST_HEAD(&port->muxed_ep[entry]);
}

port->rmnet_mode is only assigned later, in rmnet_newlink(), after
rmnet_vnd_newlink() and netdev_upper_dev_link().  The valid values are
RMNET_EPMODE_VND (1) and RMNET_EPMODE_BRIDGE (2), so during that window
rmnet_mode is 0, the switch in rmnet_rx_handler() matches neither case
and there is no default, but RX_HANDLER_CONSUMED is still returned.

Is a packet arriving on the real_dev in that window leaked?  Would it be
better to install a valid rmnet_mode before netdev_rx_handler_register()?

[Severity: Medium]
The snapshot removes the race, but the stages still resolve the
checksum-version bits differently, so is the symptom named in the
changelog ("a MAP header announcing a v5 csum header that was neither
reserved nor written") still reachable when both egress bits are set,
now without any race?

Nothing appears to reject that combination: rmnet_policy only checks the
size of struct ifla_rmnet_flags, rmnet_rtnl_validate() only validates
the MUX ID, and both rmnet_newlink() and rmnet_changelink() apply
flags->flags & flags->mask verbatim.  RMNET_FLAGS_EGRESS_MAP_CKSUMV4
and RMNET_FLAGS_EGRESS_MAP_CKSUMV5 are independent uAPI bits.

On TX, this function picks V4 first, so
rmnet_map_checksum_uplink_packet() -> rmnet_map_v4_checksum_uplink_packet()
pushes a v4 header:

	ul_header = (struct rmnet_map_ul_csum_header *)
		    skb_push(skb, sizeof(struct rmnet_map_ul_csum_header));

while rmnet_map_add_map_header() looks at the raw V5 bit of the same
sampled word:

	/* Set next_hdr bit for csum offload packets */
	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5)
		map_header->flags |= MAP_NEXT_HEADER_FLAG;

Since the only caller passes pad == RMNET_MAP_NO_PAD_BYTES, the early
return keeps that flag.  Would passing the already-resolved csum_type
(or rejecting both version bits at configuration time) be preferable to
threading the raw bitmask, which only closes the race?

The same asymmetry exists on RX: rmnet_map_validate_packet_len() gives
INGRESS_MAP_CKSUMV4 precedence and adds 8 bytes of dl csum trailer,
__rmnet_map_ingress_handler() gives V5 precedence when
MAP_NEXT_HEADER_FLAG is set and consumes a 4-byte v5 header, and
rmnet_map_send_ack() trims 8 bytes purely on the V4 bit.  With both
ingress bits set, is a v5 frame rejected as 4 bytes short, and inside an
aggregate does rmnet_map_deaggregate() advance its cursor 4 bytes into
the following MAP header, breaking every later split point?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916132231.1780091-1-edumazet%40google.com