Re: [PATCH net] net: ena: clean up XDP TX queues when regular TX setup fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] set_io_rings_size() fails to resize XDP TX queues, breaking the memory backoff loop.
- [High] Improper synchronization of per-device and per-ring u64_stats_sync counters.
- [Medium] XDP TX buffers mapped with `dma_map_single` are unmapped with `dma_unmap_page`.
--

commit a770b28356c829d571213a6237ea259eef94fd54
Author: Dawei Feng <dawei.feng@seu.edu.cn>

net: ena: clean up XDP TX queues when regular TX setup fails

create_queues_with_size_backoff() creates XDP TX queues before setting
up the regular TX path. If the subsequent allocation or creation of
regular TX queues fails, the error handling paths omit the teardown of the
XDP TX queues, leading to a resource leak.

> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 92d149d4f0917..5d05020a6d058 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
[ ... ]
> @@ -2078,14 +2090,21 @@ static int create_queues_with_size_backoff(struct ena_adapter *adapter)
>  		rc = ena_setup_tx_resources_in_range(adapter,
>  						     0,
>  						     adapter->num_io_queues);
> -		if (rc)
> +		if (rc) {
> +			ena_destroy_xdp_tx_queues(adapter);
> +			ena_free_all_io_tx_resources_in_range(adapter,
> +							      adapter->xdp_first_ring,
> +							      adapter->xdp_num_queues);
>  			goto err_setup_tx;
> +		}
>  
>  		rc = ena_create_io_tx_queues_in_range(adapter,
>  						      0,
>  						      adapter->num_io_queues);
> -		if (rc)
> +		if (rc) {
> +			ena_destroy_xdp_tx_queues(adapter);
>  			goto err_create_tx_queues;
> +		}

[Severity: High]
This is a pre-existing issue, but does set_io_rings_size() properly resize
the XDP TX queues during this backoff loop?

If memory allocation fails with -ENOMEM in this function, the driver halves
the requested ring sizes and calls set_io_rings_size() to apply them before
retrying:

ena_netdev.c:set_io_rings_size() {
    for (i = 0; i < adapter->num_io_queues; i++) {
        adapter->tx_ring[i].ring_size = new_tx_size;
    ...
}

Because it only iterates over regular IO queues and ignores the XDP TX
queues (which start at index num_io_queues), when the retry loop calls
ena_setup_and_create_all_xdp_queues(), won't it continue to use the original,
unscaled ring size?

Could this cause the driver to repeatedly attempt to allocate the same
large memory block for XDP TX queues, guaranteeing persistent -ENOMEM
failures until the regular queues shrink below ENA_MIN_RING_SIZE?

[Severity: Medium]
This is a pre-existing issue, but does the driver unmap XDP TX buffers
using the correct DMA API?

When mapping the XDP frame, it uses dma_map_single():

ena_xdp.c:ena_xdp_tx_map_frame() {
    dma = dma_map_single(tx_ring->dev, data, size, DMA_TO_DEVICE);
    ...
    tx_info->map_linear_data = 0;
}

However, because tx_info->map_linear_data is explicitly set to 0, the
teardown path seems to skip the dma_unmap_single() block and falls through
to dma_unmap_page():

ena_netdev.c:ena_unmap_tx_buff() {
    if (tx_info->map_linear_data) {
        dma_unmap_single(...);
    ...
    for (i = 0; i < cnt; i++) {
        dma_unmap_page(...);
    }
}

Since the DMA API requires buffers mapped with dma_map_single() to be
unmapped with dma_unmap_single(), will this mismatch trigger runtime
warnings when CONFIG_DMA_API_DEBUG is enabled?

[Severity: High]
This is another pre-existing issue, but is it safe to update the
u64_stats_sync structures from process context without disabling
preemption?

The driver updates stats via a helper:

ena_netdev.h:ena_increase_stat() {
    u64_stats_update_begin(syncp);
    (*statp) += cnt;
    u64_stats_update_end(syncp);
}

This helper is called from process context, such as modifying the
interface_up stat:

ena_netdev.c:ena_up() {
    ...
    ena_increase_stat(&adapter->dev_stats.interface_up, 1, &adapter->syncp);
}

If a reader (like ndo_get_stats64) preempts this writer while the seqcount
is odd, could the reader enter an infinite retry loop on 32-bit systems?

Additionally, tx_ring->syncp appears to be written concurrently by
different CPUs (e.g., ena_start_xmit() updating queue_wakeup while holding
netif_tx_lock, and ena_clean_tx_irq() updating bad_req_id from NAPI softirq).
Does this violate the requirement that u64_stats_sync writers must be
mutually exclusive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616142424.4005130-1-dawei.feng@seu.edu.cn?part=1