Re: [net-next,v6,06/15] ibmveth: Refactor TX resource allocation in open/close paths

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

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

- [Medium] The patch silently repairs a live mainline bug while
  presenting itself as a TX refactor.

Pre-existing issues:
- [Low] Concern (assessed as a FALSE POSITIVE): the new `if
  (unlikely(!adapter->tx_ltb_ptr[queue_num]))` guard in…
- [Low] Concern (assessed as a FALSE POSITIVE for the final series
  state): ibmveth_alloc_tx_resources() calls ibmveth_allocate_tx_ltb()…

--

ibmveth: Refactor TX resource allocation in open/close paths

This moves the per-queue TX LTB allocation and free out of ibmveth_open()
and ibmveth_close() into ibmveth_alloc_tx_resources() and
ibmveth_free_tx_resources(), moves the TX allocation to the end of open()
after RX interrupt setup, and adds a tx_ltb_ptr[] check to
ibmveth_start_xmit(). It also reworks the open() failure unwind labels.

A few questions below.

[Severity: Low]
This is a pre-existing style point rather than something this patch breaks,
but the new kdoc on ibmveth_allocate_tx_ltb() states an invariant that the
helper itself never checks:

  /* Caller must ensure tx_ltb_ptr[idx] is NULL. ...

and ibmveth_alloc_tx_resources() below calls it unconditionally for every
index in [0, real_num_tx_queues). If a slot were ever populated on entry,
the kzalloc() would overwrite the old pointer and leak both the 64 KiB
buffer and its streaming DMA mapping.

Tracing the callers, the only way to reach open() with a populated slot was
ibmveth_set_channels() gating on IFF_UP instead of adapter->opened, and a
later patch in this series makes set_channels() return early on !opened
before it touches the LTBs, so no reachable caller can present a populated
index. Would a WARN_ON_ONCE(adapter->tx_ltb_ptr[idx]) (or just an early
return) be worth adding so the documented precondition is enforced rather
than only described?

[ ... ]

[Severity: Medium]
Should the h_free_logical_lan() part of this hunk be split out as its own
patch with a Fixes: tag and Cc: stable?

Rerouting the ibmveth_alloc_buffer_pools() failure to out_unregister_lan
looks like a real bug fix, not just a label rename. At the series merge base
(1b78070aaef6) that failure path in ibmveth_open() runs:

  rc = ibmveth_alloc_buffer_pools(adapter);
  if (rc)
          goto out_free_buffer_pools;
  ...
  out_free_buffer_pools:
          while (--i >= 0) { ... }
  out_unmap_filter_list:
          dma_unmap_single(dev, adapter->filter_list_dma, 4096,
                           DMA_BIDIRECTIONAL);

so the filter list, the buffer list and the RX completion ring are unmapped
and freed while the logical LAN registered by ibmveth_register_logical_lan()
is still live. Only the request_irq() failure path issued the hcall. This
patch is the first place that path deregisters, which matches the commit
message note:

  "After LAN registration, open-fail teardown issues h_free_logical_lan()
  before RX pool DMA teardown on the pool-fail path that previously never
  issued that hcall (missing deregistration, not a preference reorder)."

The concern is packaging rather than correctness of the code here. As
written, the fix for an ifup-under-memory-pressure path that leaves PHYP
registered against freed DMA memory is folded into a TX helper refactor with
no Fixes: tag, no Cc: stable and no statement of the failure mode.

The next patch in the series ("ibmveth: Add RX queue register helpers for
MQ") then moves pool allocation ahead of registration, so this arrangement
is transient inside the series and no single commit is backportable for the
mainline/stable bug. Could the deregistration fix be lifted to a standalone
patch at the front of the series?

[ ... ]

[Severity: Low]
This isn't a bug introduced by the patch, and after checking it appears to
be safe, but the shape of the new guard invites the question. The check
reads adapter->tx_ltb_ptr[queue_num] once, then the body re-reads the same
slots later:

  memcpy(adapter->tx_ltb_ptr[queue_num], skb->data, skb_headlen(skb));
  ...
  desc.fields.address = adapter->tx_ltb_dma[queue_num];

Since ibmveth_free_tx_ltb() now NULLs the slot before the unmap, is the
guard intended as a race barrier, or purely as a check for a slot that is
already known to be quiesced?

Every free site does look quiesced independently of this check:
ibmveth_close() calls netif_tx_disable() plus synchronize_net() before
ibmveth_free_tx_resources(); the ibmveth_set_channels() shrink frees only
after netif_set_real_num_tx_queues(), which does synchronize_net() on the
disabling path; and the open() unwind runs with all TX queues still XOFF.

The commit message already says as much:

  "NULL-first in free_tx_ltb() only closes the check-then-use window, it is
  not itself a UAF barrier."

Would it be clearer to state next to the guard that TX is quiesced at all
free sites, so the single read is not load-bearing?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1788102125.git.mmc%40linux.ibm.com