Re: [PATCH net] net: qualcomm: rmnet: avoid stale VND access on TX drop

["Jérémy Jean" <jeremy.jean@oss.cyber.gouv.fr>]

On 2026-11-01 23:11, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · 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).
> 
> - [High] The fix is incomplete: the stale port->rmnet_dev UAF described
>   in the commit message can still be reached.
> 
> Pre-existing issues:
> - [High] Teardown race: rmnet_port can be freed while its hrtimer is
>   still armed.
> 
> --
> 
> net: qualcomm: rmnet: avoid stale VND access on TX drop
> 
> This commit changes rmnet_send_skb() so that a TX drop after a failed
> __skb_linearize() is counted on the lower device with
> dev_core_stats_tx_dropped_inc(). Before, the drop was counted through
> port->rmnet_dev, which can still point at a VND that has been deleted.
> 
>> diff --git a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c 
>> b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>> index 39d6d084e73f0..bd68d60b50d35 100644
>> --- a/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>> +++ b/drivers/net/ethernet/qualcomm/rmnet/rmnet_map_data.c
>> @@ -543,10 +543,7 @@ static void rmnet_send_skb(struct rmnet_port 
>> *port, struct sk_buff *skb)
>>  {
>>  	if (skb_needs_linearize(skb, port->dev->features)) {
>>  		if (unlikely(__skb_linearize(skb))) {
>> -			struct rmnet_priv *priv;
>> -
>> -			priv = netdev_priv(port->rmnet_dev);
>> -			this_cpu_inc(priv->pcpu_stats->stats.tx_drops);
> 
> [Severity: High]
> This removes one reader of the stale port->rmnet_dev. Can the stale
> pointer still be reached another way?
> 
> rmnet_newlink() is the only function that writes port->rmnet_dev.
> rmnet_dellink() does not clear or reassign it when the VND it points to
> is deleted. rmnet_unregister_bridge() still uses it on the real-device
> branch:
> 
> drivers/net/ethernet/qualcomm/rmnet/rmnet_config.c:rmnet_unregister_bridge() 
> {
>     ...
> 	rmnet_dev = port->rmnet_dev;
> 	if (!port->nr_rmnet_devs) {
>     ...
> 	} else {
> 		/* real device */
> 		bridge_dev = port->bridge_ep;
>     ...
> 	}
> 
> 	netdev_upper_dev_unlink(bridge_dev, rmnet_dev);
>     ...
> }
> 
> This sequence seems to reach it without any race:
> 
> 1. Create VNDs A and B on real device R, so port->rmnet_dev = B.
> 
> 2. Delete B. nr_rmnet_devs drops to 1 and B is unregistered and freed
>    (needs_free_netdev), but port->rmnet_dev still points at B.
> 
> 3. Run "ip link set S master A". rmnet_add_bridge() only rejects
>    nr_rmnet_devs > 1, so the call succeeds. It puts R's port into
>    bridge mode but does not update R's port->rmnet_dev.
> 
> 4. Unregister R:
> 
>    rmnet_config_notify_cb(NETDEV_UNREGISTER)
>      rmnet_force_unassociate_device(R)
>        rmnet_unregister_bridge(port)
>          netdev_upper_dev_unlink(S, B)   <- B already freed
>            __netdev_upper_dev_unlink()
> 
> __netdev_upper_dev_unlink() passes B to the PRECHANGEUPPER and
> CHANGEUPPER notifiers. It then calls __netdev_update_lower_level(B),
> which writes to B, and __netdev_walk_all_upper_dev(B), which walks B's
> adjacency lists.
> 
> The real S->A adjacency is also never removed. Does that leak the
> reference on A, so that unregistering A later hangs in
> netdev_wait_allrefs()?
> 
> Should port->rmnet_dev be kept valid in rmnet_dellink() instead? The
> other option is to stop using it in rmnet_unregister_bridge() as well,
> for example by using the slave port's rmnet_dev or
> netdev_master_upper_dev_get(bridge_ep).
> 
>> +			dev_core_stats_tx_dropped_inc(port->dev);
> 
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. Can
> rmnet_send_skb() run on an rmnet_port that has already been freed? If
> so, both port->dev->features and the new
> dev_core_stats_tx_dropped_inc(port->dev) read freed memory.
> 
> When the last VND is deleted, rmnet_dellink() tears down the port 
> before
> it unregisters the VND:
> 
> 	netdev_upper_dev_unlink(real_dev, dev);
> 	rmnet_unregister_real_device(real_dev);
> 	unregister_netdevice_queue(dev, head);
> 
> rmnet_unregister_real_device() then does:
> 
> 	rmnet_map_tx_aggregate_exit(port);
> 
> 	netdev_rx_handler_unregister(real_dev);
> 
> 	kfree(port);
> 
> Nothing marks the port as shutting down. With TX aggregation enabled
> (egress_agg_params.count > 1, set through ethtool coalesce), a transmit
> on another CPU at the same time could go like this:
> 
> CPU1
> rmnet_vnd_start_xmit()
>   rmnet_egress_handler()
>     port = rmnet_get_port_rcu(skb->dev);   <- before rx_handler is NULL
>     rmnet_map_egress_handler()
>       rmnet_map_tx_aggregate()
>         spin_lock_bh(&port->agg_lock);     <- waits for CPU2
> 
> CPU2
> rmnet_unregister_real_device()
>   rmnet_map_tx_aggregate_exit()
>     hrtimer_cancel(&port->hrtimer);
>     cancel_work_sync(&port->agg_wq);
>     spin_lock_bh(&port->agg_lock);
>     ... frees skbagg_head ...
>     spin_unlock_bh(&port->agg_lock);
> 
> CPU1 then takes agg_lock, sets port->skbagg_head with skb_copy_expand()
> and re-arms the timer:
> 
> 	hrtimer_start(&port->hrtimer,
> 		      ns_to_ktime(port->egress_agg_params.time_nsec),
> 		      HRTIMER_MODE_REL);
> 
> CPU2 continues:
> 
>   netdev_rx_handler_unregister()   <- synchronize_net() waits for CPU1
>   kfree(port);                     <- hrtimer still queued
> 
> When the timer fires, rmnet_map_flush_tx_packet_queue() calls
> schedule_work(&port->agg_wq) on the freed port. The work then reaches
> rmnet_send_skb() through rmnet_map_flush_tx_packet_work(). The
> aggregated skb also seems to leak.
> 
> synchronize_net() waits for the reader to finish, but nothing cancels
> the timer that the reader armed. Would a shutdown flag checked under
> agg_lock work here, or a second cancel after
> netdev_rx_handler_unregister()?
> 
> This looks like it comes from the TX aggregation code added by the
> commit in the Fixes: tag.
> 
>>  			dev_kfree_skb_any(skb);
>>  			return;
>>  		}

Both comments from Sashiko seem valid, thanks.
I will make a v2 for the first one, and create a dedicated patch for
the second one.

Jérémy

pw-bot: cr