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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 0 (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] The patch is titled 'Refactor', but it also carries the only
  fix for a NULL-pointer dereference that already exists in the base…
- [Medium] ibmveth_open() now allocates the TX LTBs (line 1432) after
  h_register_logical_lan, ibmveth_alloc_buffer_pools(),…
- [Medium] The patch makes a non-NULL tx_ltb_ptr[idx] mean 'this LTB is
  usable'.

Pre-existing issues:
- [Low] Not a real bug for the scenario described (baseline decision:
  false).
- [Low] Resolved later in the series (baseline decision: false).

--

ibmveth: Refactor TX resource allocation in open/close paths

This patch moves TX LTB allocation and freeing in open() and close() into
the new ibmveth_alloc_tx_resources() and ibmveth_free_tx_resources()
helpers. It also moves the TX allocation to the end of open(), after RX
is live. It adds netif_tx_disable() at the start of open(), and adds a
check in ibmveth_start_xmit() that drops packets when tx_ltb_ptr[] is
NULL.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 62e6c9a9f5e91..cab895cc970c5 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> +/* Caller must ensure tx_ltb_ptr[idx] is NULL. open() runs on
> + * probe-zeroed slots; set_channels() skips populated indices.
> + */
>  static int ibmveth_allocate_tx_ltb(struct ibmveth_adapter *adapter, int idx)
>  {
>  	adapter->tx_ltb_ptr[idx] = kzalloc(adapter->tx_ltb_size,
> @@ -1277,12 +1287,54 @@ static int ibmveth_allocate_tx_ltb(struct ibmveth_adapter *adapter, int idx)
>  			   "unable to DMA map tx long term buffer\n");
>  		kfree(adapter->tx_ltb_ptr[idx]);
>  		adapter->tx_ltb_ptr[idx] = NULL;
> +		adapter->tx_ltb_dma[idx] = 0;
>  		return -ENOMEM;
>  	}

[Severity: Medium]
ibmveth_free_tx_ltb() now takes a non-NULL tx_ltb_ptr[idx] to mean the
LTB is usable, and the new check in ibmveth_start_xmit() depends on that.
Does ibmveth_allocate_tx_ltb() follow the same rule?

It stores the kzalloc() result in tx_ltb_ptr[idx] before tx_ltb_dma[idx]
is mapped. On the map failure path it calls kfree() before it clears the
slot.

The stale sender that the new guard is meant to catch could hit these
windows on a later grow. After a shrink, netif_tx_wake_all_queues()
leaves the freed index j without DRV_XOFF. If ethtool -L then grows the
queues, ibmveth_set_channels() does this:

	netif_tx_stop_all_queues(netdev);

	/* Allocate any queue that we need */
	for (i = old; i < goal; i++) {
		...
		rc = ibmveth_allocate_tx_ltb(adapter, i);

The grow path has no synchronize_net(). A stale sender that already
passed the DRV_XOFF check could see a non-NULL tx_ltb_ptr[j] and
memcpy() into it. It could then put tx_ltb_dma[j] == 0 into the
H_SEND_LOGICAL_LAN descriptor, or write into memory that the map failure
path is about to kfree().

The grow-failure free loop in ibmveth_set_channels() also frees the slots
it just allocated without waiting for a grace period.

Would it be safer to allocate and map into local variables, and publish
tx_ltb_ptr[idx] last with WRITE_ONCE() or a release store? On failure,
the slot could be cleared before kfree(), and ibmveth_start_xmit() could
read the pointer and the DMA address once each.

[ ... ]

> @@ -1373,26 +1424,36 @@ static int ibmveth_open(struct net_device *netdev)
>  	 */
>  	rc = ibmveth_setup_rx_interrupts(adapter);
>  	if (rc)
> -		goto out_free_buffer_pools;
> +		goto out_unregister_lan;
>  
>  	netdev_dbg(netdev, "initial replenish cycle\n");
>  	ibmveth_schedule_rx_queue(adapter, 0);
>  
> +	rc = ibmveth_alloc_tx_resources(adapter);
> +	if (rc)
> +		goto out_cleanup_rx_interrupts;

[Severity: Medium]
Can a TX LTB allocation failure here now free RX buffers that PHYP still
owns?

By this point ibmveth_schedule_rx_queue(adapter, 0) has posted RX skb DMA
addresses to the hypervisor. If kzalloc() or dma_map_single() fails in
ibmveth_allocate_tx_ltb(), open() jumps to out_cleanup_rx_interrupts and
falls through into:

out_unregister_lan:
	do {
		lpar_rc = h_free_logical_lan(adapter->vdev->unit_address);
	} while (H_IS_LONG_BUSY(lpar_rc) || (lpar_rc == H_BUSY));
	ibmveth_free_buffer_pools(adapter);
out_free_queue_mem:
	ibmveth_cleanup_rx_resources(adapter);

Any H_FREE_LOGICAL_LAN result other than busy is ignored. The posted RX
skbs are then unmapped and freed, and so are the RX ring and the buffer
list pages. If the H_FREE failed, could PHYP still DMA inbound frames, or
the buffer list drop counter, into pages already returned to the
allocator?

Before this patch, TX allocation ran before LAN registration, so only
close() had this window.

The commit message says a later patch keeps this memory mapped on a hard
failure. At the end of the series, though, ibmveth_free_all_queues() only
returns -EIO in multi-queue mode:

	if (lpar_rc != H_PARAMETER && adapter->multi_queue)
		return -EIO;

so single-queue mode would still free the posted buffers on this path.

Does TX LTB allocation need to move after the replenish kick? Since
ibmveth_alloc_tx_resources() unwinds its own partial failure, it could
probably stay before h_register_logical_lan(), or at least before
ibmveth_schedule_rx_queue().

[ ... ]

> @@ -1864,6 +1923,14 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff *skb,
>  	int i, queue_num = skb_get_queue_mapping(skb);
>  	unsigned long mss = 0;
>  
> +	/* TX LTBs are freed on shrink; a sender that picked its queue
> +	 * before the shrink can get here after the free.
> +	 */
> +	if (unlikely(!adapter->tx_ltb_ptr[queue_num])) {

[Severity: Medium]
Does this check fix a NULL pointer dereference that is already in the
base tree?

On a TX shrink, ibmveth_set_channels() frees the dropped LTBs and then
wakes every queue, including the freed indices:

	for (i = old; i > goal; i--) {
		if (adapter->tx_ltb_ptr[i - 1])
			ibmveth_free_tx_ltb(adapter, i - 1);
	}

	netif_tx_wake_all_queues(netdev);

Some senders can still have a queue_mapping that points at a freed
index. One is AF_PACKET with PACKET_QDISC_BYPASS, preempted between
packet_pick_tx_queue() and dev_direct_xmit(). Another is pktgen reusing
a cloned skb. In the old ibmveth_start_xmit(), such a sender reaches the
memcpy() into adapter->tx_ltb_ptr[queue_num] with a NULL pointer.

If so, could this check be sent as a separate minimal patch for net,
with a tag like the following?

Fixes: 10c2aba89cc0 ("ibmveth: Ethtool set queue support")

Right now it sits inside a refactor in the middle of a net-next series,
along with the open() reorder and the RX helper dependencies, so it
can't be backported on its own.

Also, the commit message says:

  a sender already in xmit during the free is not covered, as before
  this series.

Isn't that sender already drained on the shrink path? When queues are
being disabled, netif_set_real_num_tx_queues() calls synchronize_net(),
and ibmveth_set_channels() calls it before the free loop. What remains is
a stale sender that arrives after netif_tx_wake_all_queues(), and that
is the case this check catches.

[Severity: Low]
This is a pre-existing issue and wasn't introduced by this patch. The
lockless read of tx_ltb_ptr[queue_num] here pairs with plain stores in
ibmveth_free_tx_ltb() and ibmveth_allocate_tx_ltb().

A sender already in flight can't overlap a shrink-time free, because
netif_set_real_num_tx_queues() calls synchronize_net() before
ibmveth_set_channels() frees the LTBs. Should these accesses still use
READ_ONCE() and WRITE_ONCE() to mark the data race?

> +		netdev->stats.tx_dropped++;

[Severity: Low]
This isn't a bug introduced by this patch, since the other drop paths in
ibmveth_start_xmit() do the same thing. Still, netdev->stats.tx_dropped++
is a non-atomic update of a device-wide counter. ndo_start_xmit can run
on several TX queues at once, so increments can be lost.

The later patch in this series, "ibmveth: Add per-queue RX and TX
statistics collection", replaces this with
IBMVETH_STAT_INC(txs->dropped_packets) on the per-queue counter, which
the queue's xmit lock serializes.

> +		goto out;
> +	}
> +
>  	if (ibmveth_is_packet_unsupported(skb, netdev))
>  		goto out;

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