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

[netdev-bot+sashiko@kernel.org]

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?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> index bed6f63facf25..59ef8b4ce5321 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c

[ ... ]

> @@ -342,14 +342,16 @@ 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]);
> -		port->data_format &= ~flags->mask;
> -		port->data_format |= flags->flags & flags->mask;
> +		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)) {
> -			port->data_format = old_data_format;
> +			WRITE_ONCE(port->data_format, old_data_format);
>  			NL_SET_ERR_MSG_MOD(extack, "Invalid MTU on real dev");
>  			return -EINVAL;
>  		}

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> index d055a2628d8c9..d4fec2fff227c 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c

[ ... ]

> @@ -82,7 +83,7 @@ __rmnet_map_ingress_handler(struct sk_buff *skb,
>  
>  	skb->dev = ep->egress_dev;
>  
> -	if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
> +	if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
>  	    (map_header->flags & MAP_NEXT_HEADER_FLAG)) {
>  		if (rmnet_map_process_next_hdr_packet(skb, len))
>  			goto free_skb;

[ ... ]

> @@ -120,14 +122,16 @@ rmnet_map_ingress_handler(struct sk_buff *skb,
>  		skb_push(skb, ETH_HLEN);
>  	}
>  
> -	if (port->data_format & RMNET_FLAGS_INGRESS_DEAGGREGATION) {
> -		while ((skbn = rmnet_map_deaggregate(skb, port)) != NULL)
> -			__rmnet_map_ingress_handler(skbn, port);
> +	data_format = READ_ONCE(port->data_format);
> +
> +	if (data_format & RMNET_FLAGS_INGRESS_DEAGGREGATION) {
> +		while ((skbn = rmnet_map_deaggregate(skb, data_format)) != NULL)
> +			__rmnet_map_ingress_handler(skbn, port, data_format);
>  
>  		consume_skb(skb);
>  	} else {
> -		if (rmnet_map_validate_packet_len(skb, port))
> -			__rmnet_map_ingress_handler(skb, port);
> +		if (rmnet_map_validate_packet_len(skb, data_format))
> +			__rmnet_map_ingress_handler(skb, port, data_format);
>  		else
>  			kfree_skb(skb);
>  	}

[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()?

> @@ -139,14 +143,16 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>  {
>  	int required_headroom, additional_header_len, csum_type = 0;
>  	struct rmnet_map_header *map_header;
> +	u32 data_format;
>  
>  	additional_header_len = 0;
>  	required_headroom = sizeof(struct rmnet_map_header);
>  
> -	if (port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV4) {
> +	data_format = READ_ONCE(port->data_format);
> +	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 (port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
> +	} else if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5) {
>  		additional_header_len = sizeof(struct rmnet_map_v5_csum_header);
>  		csum_type = RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
>  	}
> @@ -161,7 +167,7 @@ static int rmnet_map_egress_handler(struct sk_buff *skb,
>  						 csum_type);
>  
>  	map_header = rmnet_map_add_map_header(skb, additional_header_len,
> -					      port, 0);
> +					      data_format, 0);
>  	if (!map_header)
>  		return -ENOMEM;

[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?

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> index add0f5ade2e61..d334727e1f525 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_command.c
> @@ -48,13 +48,13 @@ static u8 rmnet_map_do_flow_control(struct sk_buff *skb,
>  
>  static void rmnet_map_send_ack(struct sk_buff *skb,
>  			       unsigned char type,
> -			       struct rmnet_port *port)
> +			       u32 data_format)
>  {
>  	struct rmnet_map_header *map_header = (void *)skb->data;
>  	struct rmnet_map_control_command *cmd;
>  	struct net_device *dev = skb->dev;
>  
> -	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
> +	if (data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4)
>  		skb_trim(skb,
>  			 skb->len - sizeof(struct rmnet_map_dl_csum_trailer));
>  

> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> index e6f48dc9fe64d..39d6d084e73f0 100644
> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
> @@ -306,7 +306,7 @@ struct rmnet_map_header *rmnet_map_add_map_header(struct sk_buff *skb,
>  	memset(map_header, 0, sizeof(struct rmnet_map_header));
>  
>  	/* Set next_hdr bit for csum offload packets */
> -	if (port->data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5)
> +	if (data_format & RMNET_FLAGS_EGRESS_MAP_CKSUMV5)
>  		map_header->flags |= MAP_NEXT_HEADER_FLAG;
>  
>  	if (pad == RMNET_MAP_NO_PAD_BYTES) {
> @@ -351,9 +351,9 @@ u32 rmnet_map_validate_packet_len(struct sk_buff *skb, struct rmnet_port *port)
>  
>  	packet_len = ntohs(maph->pkt_len) + sizeof(*maph);
>  
> -	if (port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
> +	if (data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV4) {
>  		packet_len += sizeof(struct rmnet_map_dl_csum_trailer);
> -	} else if ((port->data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
> +	} else if ((data_format & RMNET_FLAGS_INGRESS_MAP_CKSUMV5) &&
>  		   !(maph->flags & MAP_CMD_FLAG)) {
>  		/* Mapv5 data pkt without csum hdr is invalid */
>  		if (!(maph->flags & MAP_NEXT_HEADER_FLAG))

[ ... ]

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