Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] This patch silently fixes an existing DMA mapping leak in
ibmveth_open().
Pre-existing issues:
- [High] If ibmveth_alloc_buffer_pool() fails after
ibmveth_register_logical_lan() has succeeded, ibmveth_open() jumps to…
- [Medium] In ibmveth_open(), the buffer-pool unwind and the TX LTB
unwind share the loop variable `i`.
- [Medium] ibmveth_update_rx_no_buffer() checks
adapter->buffer_list_addr[0] and then loads it again, with no lock and…
--
ibmveth: Refactor RX resource allocation for MQ RX bring-up
This patch moves the filter list and per-queue RX buffer list and RX ring
allocation out of ibmveth_open() into new helpers, and makes both the
open() error path and close() free them through the same cleanup helpers.
It also NULL-checks buffer_list_addr[0] in ibmveth_update_rx_no_buffer()
and sets rc = -ENOMEM before the TX LTB allocation loop.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 7cb828b476c1b..01efd318baabb 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -390,8 +578,12 @@ static void ibmveth_replenish_buffer_pool(struct
> ibmveth_adapter *adapter,
> */
> static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
> {
> - __be64 *p = adapter->buffer_list_addr[0] + 4096 - 8;
> + __be64 *p;
>
> + if (!adapter->buffer_list_addr[0])
> + return;
> +
> + p = adapter->buffer_list_addr[0] + 4096 - 8;
> adapter->rx_no_buffer = be64_to_cpup(p);
> }
[Severity: Medium]
This is a pre-existing race and was not introduced by this patch, but the
new check makes it a little more visible.
set_csum, set_tso, change_mtu and pool_store all do a driver-internal
close/open. netif_running() stays true during that, and netpoll is not
disabled. That means this path:
ibmveth_poll_controller()->ibmveth_replenish_task()
->ibmveth_update_rx_no_buffer()
can run at the same time as:
ibmveth_close()->ibmveth_cleanup_rx_resources()
free_page((unsigned long)adapter->buffer_list_addr[i]);
adapter->buffer_list_addr[i] = NULL;
The NULL check and the pointer load are two separate plain reads of
buffer_list_addr[0], with no lock and no READ_ONCE().
Could the reader pass the check, then reload NULL and fault at offset
0xff8? Could it also read the page after free_page() has run, as the
baseline code already could?
As far as I can tell this is dealt with later in the series.
ibmveth_close() clears adapter->opened first, calls synchronize_net(), and
reads the no-buffer count under replenish_lock before freeing anything.
ibmveth_poll_controller() returns early when !opened.
[ ... ]
> @@ -752,26 +902,12 @@ static int ibmveth_open(struct net_device *netdev)
> ibmveth_free_buffer_pool(adapter,
> &adapter->rx_buff_pool[0][i]);
> }
> -out_unmap_filter_list:
> - dma_unmap_single(dev, adapter->filter_list_dma, 4096,
> - DMA_BIDIRECTIONAL);
> -
> out_free_tx_ltb:
> - while (--i >= 0) {
> + while (--i >= 0)
> ibmveth_free_tx_ltb(adapter, i);
[Severity: Medium]
This isn't a bug introduced by this patch, but the buffer pool unwind and
the TX LTB unwind share the loop variable i.
If ibmveth_alloc_buffer_pool() or request_irq() fails, out_free_buffer_pools
runs its own while (--i >= 0) loop, which leaves i == -1. Execution then
falls through to this loop.
Doesn't this loop then run zero times and leak every tx_ltb_ptr[] buffer
and its DMA mapping?
This looks like it gets fixed later in the series.
"ibmveth: Refactor buffer pool management for per-queue MQ RX" moves the
pool loop into a helper, and "ibmveth: Refactor TX resource allocation in
open/close paths" moves the TX allocation into ibmveth_alloc_tx_resources()
at the end of open(). After those patches the two unwinds no longer share
an index.
> - }
> -
> -out_unmap_buffer_list:
> - dma_unmap_single(dev, adapter->buffer_list_dma[0], 4096,
> - DMA_BIDIRECTIONAL);
> -out_free_queue_mem:
> - dma_free_coherent(dev, adapter->rx_queue[0].queue_len,
> - adapter->rx_queue[0].queue_addr,
> - adapter->rx_queue[0].queue_dma);
> + ibmveth_cleanup_rx_resources(adapter);
[Severity: High]
This is a pre-existing issue, but it is on the unwind path this patch
rewires.
The buffer pool failure branch in ibmveth_open() runs after
ibmveth_register_logical_lan() has already succeeded:
if (ibmveth_alloc_buffer_pool(&adapter->rx_buff_pool[0][i])) {
netdev_err(netdev, "unable to alloc pool\n");
adapter->rx_buff_pool[0][i].active = 0;
rc = -ENOMEM;
goto out_free_buffer_pools;
}
The request_irq() failure path calls h_free_logical_lan() before its goto.
This branch does not.
Execution then reaches ibmveth_cleanup_rx_resources() and
ibmveth_free_filter_list(). Those call dma_free_coherent() on the RX queue
and unmap and free the buffer list and filter list pages, while PHYP still
has all of them registered.
Can the hypervisor then write RX descriptors or the no-buffer counter into
freed memory?
The later patches in the series seem to address this.
"ibmveth: Refactor TX resource allocation in open/close paths" adds
h_free_logical_lan() on this path. "ibmveth: Add RX queue register helpers
for MQ" allocates the buffer pools before registration.
> out_free_filter_list:
> - free_page((unsigned long)adapter->filter_list_addr);
> -out_free_buffer_list:
> - free_page((unsigned long)adapter->buffer_list_addr[0]);
> + ibmveth_free_filter_list(adapter);
[Severity: Low]
Does this change behavior on the TX LTB failure path in a way the commit
message doesn't mention?
In the baseline, the filter list was mapped before the TX LTB loop, and a
failed ibmveth_allocate_tx_ltb() jumped to out_free_tx_ltb. That label sits
below out_unmap_filter_list, so the unmap was skipped. The page was then
freed by free_page() in out_free_filter_list while its DMA/TCE mapping was
still live.
After this patch the same failure reaches ibmveth_free_filter_list(), which
unmaps before freeing.
The commit message describes this as a helper extraction:
"Runtime behavior stays single-queue (num_rx_queues is still 1)."
Should the commit message mention this fix? Should it also be split out
with a Fixes: tag, so that stable trees pick up the filter list unmap fix?
> out:
> napi_disable(&adapter->napi[0]);
> return rc;
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790319558.git.mmc%40linux.ibm.com
