Re: [net,v2] net: wwan: qcom_bam_dmux: account network packets

[netdev-bot+sashiko]

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

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

- [High] Use-after-free write of the netdevice's core-allocated per-CPU
  statistics from the asynchronous BAM TX completion callback.

Pre-existing issues:
- [Medium] The driver allocates netdevs with alloc_netdev() in
  bam_dmux_register_netdev_work() but bam_dmux_remove() only calls…
- [Low] Drop paths remain uncounted after the patch: the `drop:` label in
  bam_dmux_netdev_start_xmit() (drivers/net/wwan/qcom_bam_dmux.c,…

--

net: wwan: qcom_bam_dmux: account network packets

The patch makes the BAM-DMUX driver maintain rx/tx packet and byte counters
using the core per-CPU software statistics (NETDEV_PCPU_STAT_TSTATS plus
dev_get_tstats64()). TX is accounted in the DMA completion callback and RX
after the BAM-DMUX header and padding are stripped.

Link: https://lore.kernel.org/netdev/20260830085400.2542956-1-sinyavin@xxxxxxxxx/ (v1)

> diff --git a/drivers/net/wwan/qcom_bam_dmux.c b/drivers/net/wwan/qcom_bam_dmux.c
> index cc6ace8d6437..5bcac2ccaabd 100644
> --- a/drivers/net/wwan/qcom_bam_dmux.c
> +++ b/drivers/net/wwan/qcom_bam_dmux.c
> @@ -177,8 +177,12 @@ static void bam_dmux_tx_callback(void *data)
>  {
>  	struct bam_dmux_skb_dma *skb_dma = data;
>  	struct sk_buff *skb = skb_dma->skb;
> +	struct net_device *netdev = skb->dev;
>  
>  	bam_dmux_tx_done(skb_dma);
> +	if (netdev)
> +		dev_sw_netstats_tx_add(netdev, 1,
> +				       ((struct bam_dmux_hdr *)skb->data)->len);
>  	dev_consume_skb_any(skb);
>  }

[Severity: High]
Can this write to freed per-CPU memory?  bam_dmux_tx_callback() runs from
the BAM dmaengine completion (vchan tasklet), and skb->dev is not a
counted reference.  dev_sw_netstats_tx_add() then does:

include/linux/netdevice.h:dev_sw_netstats_tx_add() {
	struct pcpu_sw_netstats *tstats = this_cpu_ptr(dev->tstats);

	u64_stats_update_begin(&tstats->syncp);
	u64_stats_add(&tstats->tx_bytes, len);
	...
}

With dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS, dev->tstats is
allocated by register_netdevice() and released at unregistration time,
independent of needs_free_netdev/free_netdev:

net/core/dev.c:netdev_run_todo() {
	...
		netdev_do_free_pcpu_stats(dev);
		if (dev->priv_destructor)
			dev->priv_destructor(dev);
		if (dev->needs_free_netdev)
			free_netdev(dev);
	...
}

In bam_dmux_remove() the TX channel is still armed when that happens:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_remove() {
	...
	unregister_netdevice_many(&list);
	rtnl_unlock();			/* netdev_run_todo() frees tstats */
	cancel_work_sync(&dmux->tx_wakeup_work);
	...
	if (!wait_event_timeout(dmux->pc_wait, !dmux->rx, BAM_DMUX_REMOTE_TIMEOUT))
	...
	disable_irq(dmux->pc_irq);
	bam_dmux_power_off(dmux);	/* dmaengine_terminate_sync(dmux->tx) */
	bam_dmux_free_skbs(dmux->tx_skbs, DMA_TO_DEVICE);
}

The only synchronous TX quiesce point is dmaengine_terminate_sync(dmux->tx)
inside bam_dmux_power_off(), which is reached after rtnl_unlock(),
cancel_work_sync(), the pm_runtime teardown and a wait_event_timeout() of
up to BAM_DMUX_REMOTE_TIMEOUT.  bam_dmux_netdev_stop() only does
netif_stop_queue() plus a CLOSE command, so it neither terminates nor
drains outstanding descriptors:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_netdev_stop() {
	netif_stop_queue(netdev);
	bam_dmux_send_cmd(bndev, BAM_DMUX_CMD_CLOSE);
	return 0;
}

Does any TX descriptor still in flight during that window end up doing
this_cpu_ptr() on an already freed (possibly recycled) tstats pointer?

The "if (netdev)" test only filters the driver's own command skbs, which
come from alloc_skb() and leave skb->dev NULL, so it does not say anything
about registration state.

Would accounting TX in bam_dmux_netdev_start_xmit(), where the netdev is
provably live and hdr->len is already known, avoid this?  Alternatively,
would terminating/draining the DMA channels and clearing dmux->netdevs[]
before unregister_netdevice_many() close the window?

[ ... ]

> @@ -421,6 +426,7 @@ static void bam_dmux_netdev_setup(struct net_device *dev)
>  	dev->needed_headroom = sizeof(struct bam_dmux_hdr);
>  	dev->needed_tailroom = sizeof(u32); /* word-aligned */
>  	dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
> +	dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but it
interacts with the new stats memory, so it is worth mentioning here.
bam_dmux_register_netdev_work() allocates the netdev with alloc_netdev(),
bam_dmux_netdev_setup() never sets dev->needs_free_netdev, and
bam_dmux_remove() only calls unregister_netdevice_queue() /
unregister_netdevice_many() with no free_netdev().

net/core/dev.c:netdev_run_todo() {
	...
		if (dev->needs_free_netdev)
			free_netdev(dev);

		cnt++;

		/* Free network device */
		kobject_put(&dev->dev.kobj);
	...
}

The kobject_put() there only balances the kobject_get() taken in
netdev_unregister_kobject(), so the device_initialize() reference is never
dropped and netdev_release() (kvfree(dev)) never runs.  Does this leak the
struct net_device, its private data and its queue arrays on every driver
removal?

Would setting dev->needs_free_netdev = true here (next to the new
pcpu_stat_type assignment), or pairing the alloc_netdev() with an explicit
free_netdev() in bam_dmux_remove(), be the right fix?

Note also that the still-allocated, still-readable net_device is what makes
the freed dev->tstats access above silent rather than an obvious fault.

> @@ -533,6 +539,7 @@ static void bam_dmux_cmd_data(struct bam_dmux_skb_dma *skb_dma)
>  		break;
>  	}
>  
> +	dev_sw_netstats_rx_add(netdev, skb->len);
>  	netif_receive_skb(skb);
>  }

[Severity: Low]
This is a pre-existing gap rather than something this patch introduces, but
now that rx/tx counters become non-zero the missing drop counters are more
visible.

The TX error path frees the skb and reports success without any
tx_dropped/tx_errors accounting:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_netdev_start_xmit() {
	...
drop:
	bam_dmux_tx_done(skb_dma);
	dev_kfree_skb_any(skb);
	return NETDEV_TX_OK;
}

and the RX early returns are not counted either:

drivers/net/wwan/qcom_bam_dmux.c:bam_dmux_cmd_data() {
	...
	if (!netdev || !netif_running(netdev)) {
		dev_warn(dmux->dev, "Data for inactive channel %u\n", hdr->ch);
		return;
	}

	if (hdr->len > BAM_DMUX_MAX_DATA_SIZE) {
		...
		return;
	}
	...
}

Would it make sense to also bump rx_dropped/tx_dropped on these paths, so
that ip -s link can explain the loss instead of showing traffic counters
with permanently zero drops?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831181006.1382372-1-sinyavin%40gmail.com