Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 3 · Low: 2
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).
- [Medium] Resolved later in this series; not a finding at the end of the
series.
- [Medium] Two rollback paths bring queues back into service even when
widening real_num_rx_queues back up fails.
- [Medium] Four new resize paths call ibmveth_enable_irq(adapter, i) to
unmask the subordinate queue with H_VIOCTL H_ENABLE_VIO_INTERRUPT,…
- [Low] Three comments added or edited by this patch do not match the
code.
- [Low] A subordinate RX queue stranded above the live count can leak.
--
ibmveth: Implement incremental MQ RX queue resize
This patch adds ibmveth_resize_rx_queues_incremental() and
ibmveth_resize_rx_channels(), so ethtool -L can change the live RX queue
count without tearing down the device. Scale-up allocates, registers and
unmasks the new subordinate queues. Scale-down quiesces, drains and
deregisters the retiring ones, and both directions roll back on failure.
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c
> b/drivers/net/ethernet/ibm/ibmveth.c
> index 0ac0359bb71e6..1b1dd89dadf79 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
[ ... ]
> @@ -785,15 +837,25 @@ ibmveth_cleanup_rx_interrupts(struct ibmveth_adapter
> *adapter)
[ ... ]
> static bool ibmveth_schedule_rx_queue(struct ibmveth_adapter *adapter,
> int qindex)
> {
> struct napi_struct *napi = &adapter->napi[qindex];
>
> - if (WARN_ON(qindex < 0 || qindex >= ibmveth_get_num_rx_queues(adapter)))
> + if (WARN_ON(qindex < 0))
> + return false;
> +
> + /*
> + * A live shrink can publish a lower count while netpoll walks a
> + * snapshot of the old one, so an index past the end is expected
> + * here and must not splat. ibmveth_replenish_task() skips the
> + * same way; callers already treat false as "queue is gone".
> + */
> + if (qindex >= ibmveth_get_num_rx_queues(adapter))
> return false;
[ ... ]
> @@ -1261,8 +1323,7 @@ static void ibmveth_replenish_task(struct
> ibmveth_adapter *adapter,
> * SQ open leaves PHYP masked until the first poll. If schedule_prep fails,
> * NAPI never runs and the queue stays masked (TX OK, RX/ARP dead) until
> * reload. Replenish first so an enable_irq fallback can actually deliver.
> - * Also used after every open (SQ and MQ) and after scale-down so a
> - * queue is not left idle+masked.
> + * Also used after every open (SQ and MQ) and after scale-down rollback.
> */
> static void ibmveth_restart_rx_queue(struct ibmveth_adapter *adapter,
> int qindex)
[Severity: Low]
This isn't a bug, but three of the new comments don't match the code.
The updated ibmveth_restart_rx_queue() comment lists open and scale-down
rollback as its callers. ibmveth_scale_up_rx_queues() also calls it in
three places: after each successful queue bring-up, on the keep path after
a failed destroy, and on the cleanup_new_queues keep path.
The new ibmveth_schedule_rx_queue() comment says callers treat false as
"queue is gone". ibmveth_restart_rx_queue() does the opposite on false:
if (ibmveth_schedule_rx_queue(adapter, qindex))
return;
ibmveth_enable_irq(adapter, qindex);
A caller that follows the comment would unmask PHYP on a retired queue.
The ibmveth_cleanup_single_rx_interrupt() kdoc says "Used during
incremental scale-down". It also runs on the scale-up failure paths via
ibmveth_destroy_subordinate_rx_queue().
Could these comments be updated?
[ ... ]
> +static int
> +ibmveth_scale_up_rx_queues(struct ibmveth_adapter *adapter, int old_count,
> + int new_count, int rxq_entries)
> +{
[ ... ]
> + for (i = old_count; i < new_count; i++) {
> + if (adapter->queue_handle[i]) {
> + /* Left by a failed H_FREE; close frees it. */
> + netdev_err(netdev,
> + "RX queue %d still held by PHYP, reset
> pending\n",
> + i);
> + rc = -EBUSY;
> + goto cleanup_new_queues;
> + }
> + rc = ibmveth_alloc_single_rx_queue(adapter, i,
> + rxq_entries);
> + if (rc) {
> + netdev_err(netdev, "Failed to allocate queue %d: %d\n",
> + i, rc);
> + goto cleanup_new_queues;
> + }
> +
> + rc = ibmveth_register_single_rx_queue(adapter, i);
> + if (rc) {
> + netdev_err(netdev, "Failed to register queue %d: %d\n",
> + i, rc);
> + if (rc == -EOPNOTSUPP)
> + adapter->mq_fallback = true;
> + if (!ibmveth_deregister_single_rx_queue(adapter,
> + i))
> + ibmveth_free_single_rx_queue(adapter,
> + i);
> + else
> + schedule_work(&adapter->work);
> + goto cleanup_new_queues;
> + }
[Severity: Low]
If ibmveth_deregister_single_rx_queue() fails here, queue i keeps its
buffer list page, the dma_map_single() of that page, the
dma_alloc_coherent() RX ring and its pool arrays. queue_handle[i] also
stays set above the live count. The IRQ setup failure branch below does
the same.
Only ibmveth_free_stranded_rx_queues() frees such a queue, and it runs
only from ibmveth_close() when H_FREE_LOGICAL_LAN succeeds:
if (!ibmveth_free_all_queues(adapter))
ibmveth_free_stranded_rx_queues(adapter);
If H_FREE_LOGICAL_LAN fails in close, does this leak the stranded queue's
page, RX ring and DMA mapping? The out_free_all_queues unwind in
ibmveth_open() and ibmveth_remove() also don't seem to release queues
above the live count.
After a failed close, queue_handle[i] also stays set. Every later scale-up
that reaches index i then returns -EBUSY with the "reset pending" message,
but that path doesn't schedule a reset.
In the same failed-close case, the live queues' memory is freed anyway.
Keeping only the stranded queue's memory looks inconsistent.
[ ... ]
> + ibmveth_publish_num_rx_queues(adapter, i + 1);
> + ibmveth_replenish_task(adapter, i);
> + napi_enable(&adapter->napi[i]);
> +
> + rc = ibmveth_enable_irq(adapter, i);
> + if (rc) {
[ ... ]
> + if (rc) {
> + /* enable_irq errno; keep -EIO. */
> + rc = -EIO;
> + goto cleanup_new_queues;
> + }
> + ibmveth_restart_rx_queue(adapter, i);
> + }
[Severity: Medium]
Can this unmask PHYP behind a live poll?
Once ibmveth_enable_irq() unmasks queue i, an interrupt on another CPU can
run ibmveth_interrupt(), win napi_schedule_prep(), mask PHYP and schedule
NAPI. ibmveth_restart_rx_queue() then sees its own prep fail and falls
back to:
ibmveth_enable_irq(adapter, qindex);
That unmasks PHYP while the poll owns the queue. When the poll completes,
it calls ibmveth_enable_irq() again on a source that is already enabled.
ibmveth_toggle_irq() folds H_PARAMETER to success only on disable:
if (h_rc == H_PARAMETER && !enable) {
So on enable it returns -EIO, and ibmveth_poll() schedules a reset.
The v6 changelog gives this case as the reason the survivor restart loop
was dropped: "enable_irq() on prep-fail unmasks behind a live poll
(subordinate H_PARAMETER on enable is -EIO and reset)".
The same enable-then-restart sequence appears in four places:
- here
- the keep branch after a failed destroy
- the cleanup_new_queues keep loop
- both rollback loops in ibmveth_scale_down_rx_queues()
On a busy link, would an ethtool -L that otherwise succeeded end in an
adapter reset?
[ ... ]
> + /* Drop the live count before freeing the half-added queues. */
> + ibmveth_publish_num_rx_queues(adapter, old_count);
> + if (netif_set_real_num_rx_queues(netdev, old_count))
> + schedule_work(&adapter->work);
> + synchronize_net();
> +
> + for (i = failed_queue - 1; i >= old_count; i--) {
> + int keep;
> +
> + if (!ibmveth_destroy_subordinate_rx_queue(adapter, i))
> + continue;
> +
> + keep = i + 1;
> + ibmveth_publish_num_rx_queues(adapter, keep);
> + if (netif_set_real_num_rx_queues(netdev, keep))
> + schedule_work(&adapter->work);
> + for (i = old_count; i < keep; i++) {
> + int irq_rc;
> +
> + ibmveth_replenish_task(adapter, i);
> + /* START: NAPI before PHYP unmask. */
> + napi_enable(&adapter->napi[i]);
> + irq_rc = ibmveth_enable_irq(adapter, i);
[Severity: Medium]
At this point real_num_rx_queues has already been lowered to old_count,
and keep is above old_count. Growing the count goes through
net_rx_queue_update_kobjects(), which can fail (for example with -ENOMEM).
netif_set_real_num_rx_queues() then leaves dev->real_num_rx_queues
unchanged.
If that happens, is it safe to still replenish, napi_enable() and unmask
queues old_count..keep-1? Packets on those queues would carry
skb_record_rx_queue() indexes at or above real_num_rx_queues. That hits
the WARN_ONCE in get_rps_cpu() and the bypass in netif_get_rxqueue(),
which is what widening real_num up front in this function is meant to
avoid.
The destroy failure path in ibmveth_scale_down_rx_queues() has the same
pattern, after real_num has been lowered to new_count:
if (netif_set_real_num_rx_queues(netdev, keep))
schedule_work(&adapter->work);
for (i = new_count; i < keep; i++) {
[ ... ]
> @@ -2567,18 +3486,15 @@ static int ibmveth_set_channels(struct net_device
> *netdev,
> goal = channels->tx_count;
> int rc, i;
>
> - /*
> - * RX channel resize is implemented in a later patch; reject any
> - * request that changes rx_count. Read-modify-write TX adjustments
> - * submit the current rx_count and proceed.
> + /* Validate RX (and resize when opened) before the down-path
> + * early return so MQ/range errors are reported here. Publishing
> + * the desired RX count and CMO while down is the next patch.
> */
> - if (channels->rx_count != ibmveth_get_num_rx_queues(adapter))
> - return -EOPNOTSUPP;
> + rc = ibmveth_resize_rx_channels(adapter, channels->rx_count);
> + if (rc)
> + return rc;
[Severity: Medium]
The RX resize is committed here, before any TX work. If a TX LTB
allocation later fails in ibmveth_set_channels(), the code does:
goal = old;
old = i;
break;
}
rc = netif_set_real_num_tx_queues(netdev, goal);
This overwrites the allocation error with the result of reverting to the
old TX count, which is normally 0.
For a combined ethtool -L rx N tx M on an opened MQ adapter, would this
change RX, leave TX unchanged and still report success? Nothing at this
commit rolls RX back.
The last patch in the series, "ibmveth: Complete set_channels down-path
and mq_fallback max_rx cap", adds rx_changed tracking, a rollback through
ibmveth_resize_rx_queues_incremental(adapter, old_rx, ...), and a return
of rc ? rc : -ENOMEM when real_num_tx_queues doesn't match the request.
That fixes it by the end of the series, but the behavior above is still
present at this commit.
> - /* If ndo_open has not been called yet then don't allocate, just set
> - * desired netdev_queue's and return
> - */
> - if (!(netdev->flags & IFF_UP))
> + if (!adapter->opened)
> return netif_set_real_num_tx_queues(netdev, goal);
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1790319558.git.mmc%40linux.ibm.com
