Re: [PATCH net-next v4 10/14] ibmveth: Add per-queue RX and TX statistics collection

[mingming cao <mmc@linux.ibm.com>]

On 8/6/26 11:37 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> ibmveth: Add per-queue RX and TX statistics collection
>
> This patch adds per-queue RX and TX counter arrays that live for the
> adapter lifetime, exports them through ethtool -S, and adds an
> ndo_get_stats64 implementation that sums them. It also changes what
> ibmveth_get_channels() reports for max_rx and rx_count.
>
> A few questions below.
Hi Jakub,

Thanks for the review.
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 4ad7ced3c6083..dbb47a42b4cb5 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -921,6 +975,8 @@ static void ibmveth_update_rx_no_buffer(struct ibmveth_adapter *adapter)
>>   		__be64 *p = adapter->buffer_list_addr[i] + 4096 - 8;
>>   		u64 drops = be64_to_cpup(p);
>>   
>> +		if (adapter->rx_qstats)
>> +			adapter->rx_qstats[i].no_buffer_drops = drops;
>>   		adapter->rx_no_buffer += drops;
>>   	}
>>   }
> [Severity: Medium]
>
> Can concurrent polls on different queues corrupt these counters?
>
> ibmveth_update_rx_no_buffer() writes rx_qstats[i] for every queue, but
> its caller ibmveth_replenish_task(adapter, queue_index) holds only that
> one queue's lock:
>
> 	spin_lock_irqsave(&rxq->replenish_lock, flags);
> 	...
> 	ibmveth_update_rx_no_buffer(adapter);
> 	spin_unlock_irqrestore(&rxq->replenish_lock, flags);
>
> So two NAPI polls on different queues run the same all-queue loop under
> disjoint locks (replenish_lock[0] vs replenish_lock[1]):
>
> CPU A: reads queue 1 hypervisor value 100
> CPU B: reads queue 1 hypervisor value 105, stores 105
> CPU A: stores 100
>
> Does that make rxN_no_buffer_drops move backwards as seen by ethtool?
> The adapter->rx_no_buffer = 0 / += drops sequence around it is also an
> unsynchronized read-modify-write visible to a concurrent ethtool reader.
>
> This also seems to work against the ____cacheline_aligned_in_smp
> annotation added to struct ibmveth_rx_queue_stats, since every replenish
> cycle now dirties every queue's stats cache line from a foreign CPU.
>
> Would passing queue_index into the helper and touching only
> rx_qstats[queue_index] and buffer_list_addr[queue_index] work, deriving
> the adapter-level rx_no_buffer by summing on read the way the patch
> already does for rx_large_packets and rx_invalid_buffer?
>
> Related: at the end of the series the RX scale-down path in
> ibmveth_resize_rx_queues_incremental() lowers adapter->num_rx_queues and
> then frees a queue's buffer_list page while polls on surviving queues keep
> running. Can a poll that already loaded the old bound dereference
> buffer_list_addr[i] for a freed page here?

Yes. This needs to become queue-local and NULL-safe.

I’m planning to update only the current queue’s slot, keep the absolute
PHYP count in `rx_qstats[q].no_buffer_drops`, and derive the adapter
`rx_no_buffer` total by summing on read. That avoids concurrent-poll
RMW on one shared field, avoids foreign cacheline writes, and closes the
scale-down freed-page hazard.
>
> [Severity: Medium]
>
> Should no_buffer_drops be accumulated rather than assigned?
>
> The value PHYP writes into the last 8 bytes of the buffer-list page is an
> absolute count for the life of that page, and the page is re-obtained with
> get_zeroed_page(GFP_KERNEL) by ibmveth_alloc_rx_queues() on every
> ibmveth_open() and released by ibmveth_cleanup_rx_resources() on every
> ibmveth_close(), including the close/open pairs done by the reset work,
> ibmveth_set_csum_offload() and ibmveth_set_tso().
>
> Since rx_qstats[i].no_buffer_drops is assigned with "= drops", does
> rxN_no_buffer_drops (and the recomputed adapter->rx_no_buffer) jump
> backwards after an ifdown/ifup, a driver reset, or an ethtool -K tso
> change?
>
> That appears to contradict the comment this patch adds above the sum
> helpers:
>
> 	 * globals on the hot path (ibmvnic-style); with qstats allocated for the
> 	 * adapter lifetime, these sums remain meaningful across ifdown/up.
The PHYP field there is an absolute count for the life of that
buffer-list page, so assigning `=` into the per-queue slot is correct
for that page lifetime.

What needs tightening is the lifetime wording around the aggregate:
qstats live for the adapter lifetime, but a raw sum of these page-
absolute values can still go backwards across ifdown/up. I’m planning
to clarify that in the comments and ethtool wording rather than imply
monotonic lifetime `no_buffer` semantics without a baseline+delta
model.
>> @@ -1972,22 +2028,131 @@ static int ibmveth_set_features(struct net_device *dev,
>>    * globals on the hot path (ibmvnic-style); with qstats allocated for the
>>    * adapter lifetime, these sums remain meaningful across ifdown/up.
>>    */
>> +static u64 ibmveth_sum_rx_invalid_buffers(struct ibmveth_adapter *adapter)
>> +{
>> +	u64 total = 0;
>> +	int i;
>> +
>> +	if (!adapter->rx_qstats)
>> +		return adapter->rx_invalid_buffer;
>> +
>> +	for (i = 0; i < adapter->num_rx_queues; i++)
>> +		total += adapter->rx_qstats[i].invalid_buffers;
>> +
>> +	return total;
>> +}
> [ ... ]
>
>> +static u64 ibmveth_sum_tx_send_failed(struct ibmveth_adapter *adapter)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	u64 total = 0;
>> +	int i;
>> +
>> +	if (!adapter->tx_qstats)
>> +		return adapter->tx_send_failed;
>> +
>> +	for (i = 0; i < netdev->real_num_tx_queues; i++)
>> +		total += adapter->tx_qstats[i].send_failures;
>> +
>> +	return total;
>> +}
> [Severity: Medium]
>
> Do these sums go backwards when the queue count is reduced?
>
> The qstats arrays persist for the adapter lifetime, but the sums are
> bounded by the currently configured queue count. ibmveth_set_channels()
> lowers netdev->real_num_tx_queues:
>
> 	rc = netif_set_real_num_tx_queues(netdev, goal);
>
> Nothing folds or clears the counters of the queues that disappear, so
> after "ethtool -L ethX tx N" with a smaller N, do the adapter-level
> ethtool strings tx_large_packets and tx_send_failed, plus the
> rtnl_link_stats64 tx_packets/tx_bytes/tx_dropped summed in
> ibmveth_get_stats64(), all drop by the traffic accounted to the removed
> queues, and then jump forward again when the count is raised?
>
> Since this patch removes the hot-path netdev->stats accounting,
> ibmveth_get_stats64() is now the only source for the rtnl counters, so a
> decrease is directly user visible; tools computing deltas (ip -s link,
> ifstat) will read it as a counter wrap. The same applies on the RX side
> with adapter->num_rx_queues once the incremental RX resize commit later in
> this series changes it at runtime.

Yes. The aggregate sums should not be bounded by only the currently live
queue count.

I’m planning to make the aggregate stats walk the fixed MAX queue slots
so retired queues keep contributing and the adapter-visible totals do
not move backwards when the active queue count shrinks.

>> +static u64 ibmveth_ethtool_adapter_stat(struct ibmveth_adapter *adapter,
>> +					int index)
>> +{
>> +	unsigned long offset = ibmveth_stats[index].offset;
>> +
>> +	if (offset == IBMVETH_STAT_OFF(rx_invalid_buffer))
>> +		return ibmveth_sum_rx_invalid_buffers(adapter);
>> +	if (offset == IBMVETH_STAT_OFF(rx_large_packets))
>> +		return ibmveth_sum_rx_large_packets(adapter);
>> +	if (offset == IBMVETH_STAT_OFF(tx_large_packets))
>> +		return ibmveth_sum_tx_large_packets(adapter);
>> +	if (offset == IBMVETH_STAT_OFF(tx_send_failed))
>> +		return ibmveth_sum_tx_send_failed(adapter);
>> +
>> +	return IBMVETH_GET_STAT(adapter, offset);
>> +}
>> +
>>   static void ibmveth_get_strings(struct net_device *dev, u32 stringset, u8 *data)
>>   {
>> +	struct ibmveth_adapter *adapter = netdev_priv(dev);
>> +	u8 *p = data;
>>   	int i;
>>   
>>   	if (stringset != ETH_SS_STATS)
>>   		return;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++, data += ETH_GSTRING_LEN)
>> -		memcpy(data, ibmveth_stats[i].name, ETH_GSTRING_LEN);
>> +	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++) {
>> +		memcpy(p, ibmveth_stats[i].name, ETH_GSTRING_LEN);
>> +		p += ETH_GSTRING_LEN;
>> +	}
>> +
>> +	for (i = 0; i < adapter->num_rx_queues; i++) {
>> +		ethtool_sprintf(&p, "rx%d_packets", i);
>> +		ethtool_sprintf(&p, "rx%d_bytes", i);
>> +		ethtool_sprintf(&p, "rx%d_interrupts", i);
>> +		ethtool_sprintf(&p, "rx%d_polls", i);
>> +		ethtool_sprintf(&p, "rx%d_large_packets", i);
>> +		ethtool_sprintf(&p, "rx%d_invalid_buffers", i);
>> +		ethtool_sprintf(&p, "rx%d_no_buffer_drops", i);
>> +	}
>> +
>> +	for (i = 0; i < dev->real_num_tx_queues; i++) {
>> +		ethtool_sprintf(&p, "tx%d_packets", i);
>> +		ethtool_sprintf(&p, "tx%d_bytes", i);
>> +		ethtool_sprintf(&p, "tx%d_large_packets", i);
>> +		ethtool_sprintf(&p, "tx%d_dropped_packets", i);
>> +		ethtool_sprintf(&p, "tx%d_send_failures", i);
>> +		ethtool_sprintf(&p, "tx%d_checksum_offload", i);
>> +	}
> [Severity: Medium]
>
> Should the per-queue packet and byte counters use the standard per-queue
> statistics interface instead of driver-private ethtool strings?
>
> rx%d_packets, rx%d_bytes, tx%d_packets and tx%d_bytes map one to one onto
> struct netdev_queue_stats_rx and struct netdev_queue_stats_tx, which are
> exported through netlink via struct netdev_stat_ops. No netdev_stat_ops
> is added here, so generic tooling (ynl) cannot consume these values, and
> Documentation/networking/statistics.rst asks that counters with a standard
> uAPI not be duplicated in ethtool -S.
>
> Also, the new ibmveth_get_stats64() populates only rx/tx packets, bytes
> and tx_dropped:
>
> 	stats->tx_errors = dev->stats.tx_errors;
>
> After this patch, is there any remaining writer of dev->stats in this
> driver? The only match in the file is this read, so tx_errors appears to
> be permanently 0. The loss counters this patch does collect
> (no_buffer_drops from the hypervisor, invalid_buffers) are not mapped
> into rx_dropped / rx_missed_errors / rx_errors either, so ip -s link
> reports zero RX drops while ethtool -S shows a non-zero rx_no_buffer.

Agreed in principle. The duplicated standard queue stats in `-S`, the
stale `tx_errors` read, and the incomplete RX drop mapping are all real
interface issues, but I still see them as broader uAPI cleanup rather
than something that has to be solved in the same patch as MQ stats
enablement.

I’m planning to keep the MQ correctness fixes in this series and leave
the `netdev_stat_ops` / rtnl mapping cleanup as a follow-up, since that
is really an ABI/tools migration rather than a small MQ fix.

>> +
>> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++) {
>> +		ethtool_sprintf(&p, "pool%d_size", i);
>> +		ethtool_sprintf(&p, "pool%d_active", i);
>> +		ethtool_sprintf(&p, "pool%d_available", i);
>> +	}
>>   }
>>   
>>   static int ibmveth_get_sset_count(struct net_device *dev, int sset)
>>   {
>> +	struct ibmveth_adapter *adapter = netdev_priv(dev);
>> +
>>   	switch (sset) {
>>   	case ETH_SS_STATS:
>> -		return ARRAY_SIZE(ibmveth_stats);
>> +		return ARRAY_SIZE(ibmveth_stats) +
>> +		       adapter->num_rx_queues * IBMVETH_NUM_RX_QSTATS +
>> +		       dev->real_num_tx_queues * IBMVETH_NUM_TX_QSTATS +
>> +		       IBMVETH_NUM_BUFF_POOLS * 3;
>>   	default:
>>   		return -EOPNOTSUPP;
>>   	}
> [ ... ]
>
>> @@ -1996,21 +2161,59 @@ static int ibmveth_get_sset_count(struct net_device *dev, int sset)
>>   static void ibmveth_get_ethtool_stats(struct net_device *dev,
>>   				      struct ethtool_stats *stats, u64 *data)
>>   {
>> -	int i;
>>   	struct ibmveth_adapter *adapter = netdev_priv(dev);
>> +	int i, j;
>>   
>>   	for (i = 0; i < ARRAY_SIZE(ibmveth_stats); i++)
>> -		data[i] = IBMVETH_GET_STAT(adapter, ibmveth_stats[i].offset);
>> +		data[i] = ibmveth_ethtool_adapter_stat(adapter, i);
>> +
>> +	for (j = 0; j < adapter->num_rx_queues; j++) {
>> +		if (adapter->rx_qstats) {
>> +			data[i++] = adapter->rx_qstats[j].packets;
>> +			data[i++] = adapter->rx_qstats[j].bytes;
>> +			data[i++] = adapter->rx_qstats[j].interrupts;
>> +			data[i++] = adapter->rx_qstats[j].polls;
>> +			data[i++] = adapter->rx_qstats[j].large_packets;
>> +			data[i++] = adapter->rx_qstats[j].invalid_buffers;
>> +			data[i++] = adapter->rx_qstats[j].no_buffer_drops;
>> +		} else {
>> +			i += IBMVETH_NUM_RX_QSTATS;
>> +		}
>> +	}
> [Severity: Low]
>
> Can adapter->rx_qstats or adapter->tx_qstats ever be NULL here?
>
> The allocations happen in ibmveth_probe() before register_netdev(), and a
> failure aborts probe with ibmveth_probe_cleanup() + return -ENOMEM.
> ibmveth_remove() frees them only after unregister_netdev(). So on a
> registered netdev the NULL case seems unreachable, which would make these
> else arms, the else arms in ibmveth_poll(), and the
> "if (!adapter->*_qstats) return adapter-><legacy>;" early returns in the
> four sum helpers dead code.
>
> The two styles also disagree: ibmveth_is_packet_unsupported() guards with
> "if (adapter->tx_qstats)" while ibmveth_start_xmit() dereferences
> adapter->tx_qstats[queue_num] unconditionally at six new sites on the same
> call chain. Which of the two is intended?
>
> A side effect is that adapter->rx_invalid_buffer, rx_large_packets,
> tx_large_packets and tx_send_failed are never incremented again while
> still being listed in ibmveth_stats[].
>
> Separately, ibmveth_ethtool_adapter_stat() dispatches by comparing
> ibmveth_stats[index].offset against IBMVETH_STAT_OFF() values. Would
> adding or reordering a field in struct ibmveth_adapter silently redirect a
> statistic to the wrong source here?

Yes. After successful probe those NULL arms are effectively dead, and
the mixed guarded/unguarded style should be simplified.

I’m planning to collapse that to one defensive style at fold rather than
leave the current inconsistent mix in place.

Separately, I agree the offset-based adapter-stat dispatch is brittle,
even if it is not the main correctness blocker here.

>> +
>> +	for (j = 0; j < dev->real_num_tx_queues; j++) {
>> +		if (adapter->tx_qstats) {
>> +			data[i++] = adapter->tx_qstats[j].packets;
>> +			data[i++] = adapter->tx_qstats[j].bytes;
>> +			data[i++] = adapter->tx_qstats[j].large_packets;
>> +			data[i++] = adapter->tx_qstats[j].dropped_packets;
>> +			data[i++] = adapter->tx_qstats[j].send_failures;
>> +			data[i++] = adapter->tx_qstats[j].checksum_offload;
>> +		} else {
>> +			i += IBMVETH_NUM_TX_QSTATS;
>> +		}
>> +	}
>> +
>> +	for (j = 0; j < IBMVETH_NUM_BUFF_POOLS; j++) {
>> +		data[i++] = adapter->rx_buff_pool[0][j].size;
>> +		data[i++] = adapter->rx_buff_pool[0][j].active;
>> +		data[i++] = atomic_read(&adapter->rx_buff_pool[0][j].available);
>> +	}
>>   }
> [Severity: Medium]
>
> Should the pool%d_* strings be queue-indexed?
>
> The driver now keeps an independent pool set per RX queue
> (rx_buff_pool[queue][pool], populated for all q < adapter->num_rx_queues
> by ibmveth_alloc_buffer_pools() and updated per queue by
> ibmveth_replenish_task()), but these three entries read only
> rx_buff_pool[0][j] under queue-agnostic names, so the state of queues
> 1..N-1 is not visible. Is that intended for someone debugging drops on a
> non-zero queue?
>
> Also, size and active are configuration values already exposed through the
> per-pool sysfs kobjects rather than statistics, and this new permanent
> ethtool string set is not mentioned in the commit message.

Yes, that is intentional here: those `pool%d_*` entries are the shared
queue-0 template view, not live per-queue runtime state.

I’m planning to make that clearer in the wording. Live per-queue state
belongs in the later debugfs visibility, and I’m fine with dropping
`size` / `active` from ethtool -S later if sysfs-only is preferred.

>>   static void ibmveth_get_channels(struct net_device *netdev,
>>   				 struct ethtool_channels *channels)
>>   {
>> +	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> +
>>   	channels->max_tx = ibmveth_real_max_tx_queues();
>>   	channels->tx_count = netdev->real_num_tx_queues;
>>   
>> -	channels->max_rx = netdev->real_num_rx_queues;
>> -	channels->rx_count = netdev->real_num_rx_queues;
>> +	if (adapter->multi_queue)
>> +		channels->max_rx = IBMVETH_MAX_RX_QUEUES;
>> +	else
>> +		channels->max_rx = 1;
>> +	channels->rx_count = adapter->num_rx_queues;
>>   }
> [Severity: Low]
>
> This isn't a bug, but would this ABI-visible get_channels() reporting
> change be easier to review as its own patch? It is independent of the
> statistics work, and if it is a fix it would want its own Fixes: tag.
>
> There is also a mismatch it introduces: the new RX loops in
> ibmveth_get_strings() / ibmveth_get_sset_count() / ibmveth_get_stats64()
> use adapter->num_rx_queues while the TX loops use
> dev->real_num_tx_queues. netdev->real_num_rx_queues is only synced to
> adapter->num_rx_queues in ibmveth_open(), so on a never-opened interface
> ethtool -l reports a count the stack does not have yet.
>
> [ ... ]

Agreed. That `get_channels()` reporting change is separate enough that
it should be split out so this patch stays stats-focused and the
reporting change can be reviewed on its own.

>> @@ -2150,6 +2355,7 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff *skb,
>>   	    skb_checksum_help(skb)) {
>>   
>>   		netdev_err(netdev, "tx: failed to checksum packet\n");
>> +		adapter->tx_qstats[queue_num].dropped_packets++;
>>   		goto out;
>>   	}
> [ ... ]
>
>> @@ -2211,7 +2423,11 @@ static netdev_tx_t ibmveth_start_xmit(struct sk_buff *skb,
>>   	dma_wmb();
>>   
>>   	if (ibmveth_send(adapter, desc.desc, mss)) {
>> +		adapter->tx_qstats[queue_num].send_failures++;
>> +		adapter->tx_qstats[queue_num].dropped_packets++;
>>   	} else {
>> +		adapter->tx_qstats[queue_num].packets++;
>> +		adapter->tx_qstats[queue_num].bytes += skb->len;
>>   	}
> [Severity: Medium]
>
> These empty if/else bodies in the pre-image show that the previous commit
> in the series ("ibmveth: Enable multi-queue RX receive path") deleted the
> hot-path netdev->stats accounting, and the replacement only arrives here.
> ibmveth_poll() likewise increments no packet or byte counter at that
> commit, and no ndo_get_stats64 exists yet.
>
> Does that leave the tree at the parent commit reporting zero
> rx_packets/rx_bytes/tx_packets/tx_bytes/tx_dropped through ip -s link,
> where the pre-series baseline reported them? Would squashing the removal
> into this patch, or deferring the removal until this replacement lands,
> keep every commit in the series bisectable for counter-related issues?
>
> [ ... ]

Fair point. If the series keeps the current split, I’m planning to note
that intermediate stats hole in the cover and consider a minimal bridge
when restacking so the series stays bisectable for counter behavior.

>> @@ -2698,6 +2933,40 @@ static netdev_features_t ibmveth_features_check(struct sk_buff *skb,
>>   	return vlan_features_check(skb, features);
>>   }
>>   
> [ ... ]
>
>>   static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter,
>> -				   int pools_ready)
>> +				  int pools_ready)
>>   {
>>   	int i;
> [Severity: Low]
>
> This isn't a bug, but this whitespace-only reflow breaks the previously
> correct open-parenthesis alignment of the second parameter (checkpatch:
> "Alignment should match open parenthesis") in a function this patch does
> not otherwise change. Could it be dropped?

Yes. That whitespace-only reflow should just be dropped.

>> @@ -2724,6 +2994,19 @@ static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter,
>>   		kobject_put(&adapter->rx_buff_pool[0][i].kobj);
>>   }
>>   
>> +static void ibmveth_probe_cleanup(struct ibmveth_adapter *adapter,
>> +				  int pools_ready)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +
>> +	cancel_work_sync(&adapter->work);
>> +	ibmveth_put_pool_kobjs(adapter, pools_ready);
>> +
>> +	ibmveth_free_tx_qstats(adapter);
>> +	ibmveth_free_rx_qstats(adapter);
>> +	free_netdev(netdev);
>> +}
>> +
> [Severity: High]
>
> This isn't a problem introduced by this patch, since the old open-coded
> error paths had the same omission, but the new helper is now the single
> place four probe failure paths funnel through, so it looks like the
> natural spot to fix it.
>
> ibmveth_probe() stores the netdev in the VIO drvdata right after
> alloc_etherdev_mqs():
>
> 	dev_set_drvdata(&dev->dev, netdev);
>
> ibmveth_probe_cleanup() ends with free_netdev(netdev) but never clears it,
> and ibmveth_remove() (the only caller of dev_set_drvdata(&dev->dev, NULL))
> does not run when probe fails. So after any probe failure the vio_dev
> keeps a pointer to freed memory.
>
> On FW_FEATURE_CMO systems the stale pointer is consumed on the next bind
> attempt, before the driver probe re-initializes it:
>
> vio_bus_probe()
>    vio_cmo_bus_probe()
>      viodev->cmo.desired = IOMMU_PAGE_ALIGN(viodrv->get_desired_dma(viodev), tbl);
>        ibmveth_get_desired_dma()
>
> ibmveth_get_desired_dma() only checks for NULL:
>
> 	netdev = dev_get_drvdata(&vdev->dev);
> 	if (netdev == NULL)
> 		return ...;
>
> and then dereferences netdev->mtu and adapter->num_rx_queues, using the
> latter as the loop bound over adapter->rx_buff_pool[q][i]. Since that
> bound is read from freed memory, can this read past the end of the freed
> allocation as well? Would adding dev_set_drvdata(&dev->dev, NULL) to
> ibmveth_probe_cleanup() close this?

Yes. `probe_cleanup()` needs to clear the VIO drvdata before
`free_netdev()`, so a later CMO entitlement path cannot see a freed
netdev.
>>   static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   {
>>   	int rc, i, mac_len, pools_ready = 0;
>> @@ -2779,6 +3062,11 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   		netif_napi_add_weight(netdev, &adapter->napi[i],
>>   				      ibmveth_poll, 16);
>>   
>> +	if (ibmveth_alloc_rx_qstats(adapter) ||
>> +	    ibmveth_alloc_tx_qstats(adapter)) {
>> +		ibmveth_probe_cleanup(adapter, 0);
>> +		return -ENOMEM;
>> +	}
>>   
>>   	netdev->irq = dev->irq;
>>   	netdev->netdev_ops = &ibmveth_netdev_ops;
> [ ... ]
>
>> @@ -2913,6 +3198,9 @@ static void ibmveth_remove(struct vio_dev *dev)
>>   
>>   	unregister_netdev(netdev);
>>   
>> +	ibmveth_free_tx_qstats(adapter);
>> +	ibmveth_free_rx_qstats(adapter);
>> +
>>   	free_netdev(netdev);
>>   	dev_set_drvdata(&dev->dev, NULL);
>>   }
> [Severity: High]
>
> This is a pre-existing issue, since the cancel_work_sync() placement in
> ibmveth_remove() predates this patch, but the patch adds two new frees
> into exactly that window.
>
> ibmveth_remove() calls cancel_work_sync(&adapter->work) at the top,
> before unregister_netdev(), so the device is still open when the cancel
> returns and softirq context can re-arm the work:
>
> ibmveth_poll()                     /* on ibmveth_enable_irq() failure */
>    schedule_work(&adapter->work);
>
> ibmveth_replenish_buffer_pool()    /* on an invalid free_map index */
>    schedule_work(&adapter->work);
>
> Timeline:
>
> CPU A: cancel_work_sync() returns, queue empty
> CPU B: ibmveth_poll() -> schedule_work(&adapter->work)
> CPU A: unregister_netdev()
> CPU A: ibmveth_free_tx_qstats() / ibmveth_free_rx_qstats()
> CPU A: free_netdev(netdev)
> CPU B: ibmveth_reset() runs container_of(w, struct ibmveth_adapter, work)
>
> Can the worker then dereference the freed adapter and netdev in
> rtnl_lock(); dev_close(adapter->netdev); dev_open(...)?
>
> If it reaches dev_open() -> netif_tx_start_all_queues(), TX restarts with
> tx_qstats == NULL, and the unguarded adapter->tx_qstats[queue_num]
> dereferences added to ibmveth_start_xmit() by this patch would then be a
> NULL dereference. The new "if (adapter->rx_qstats)" hot-path guards do not
> help, since the adapter holding the pointer is itself inside the freed
> netdev.
>
> Would moving the cancel (or disable_work_sync()) to after
> unregister_netdev() address this?

Yes. `remove()` needs to unregister the netdev first, then cancel the
work, then free the qstats and netdev, so nothing can re-arm the reset
work after the cancel returns.

Thanks,
Mingming