Re: [PATCH net-next v4 11/14] ibmveth: Expose per-queue buffer pool details via debugfs

[Jakub Kicinski <kuba@kernel.org>]

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: Expose per-queue buffer pool details via debugfs

This adds a read-only buffer_pools debugfs file that dumps size,
buff_size, active and available for every RX queue and buffer pool, and
wires creation into ibmveth_probe() and teardown into ibmveth_remove().
The existing queue-0 poolN sysfs files are left unchanged.

A few questions about the new code below.

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index dbb47a42b4cb5..db13747466025 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c

[ ... ]

> @@ -2985,6 +2986,44 @@ static const struct net_device_ops ibmveth_netdev_ops = {
>  #endif
>  };
>  
> +static int ibmveth_buffer_pools_show(struct seq_file *m, void *v)
> +{
> +	struct ibmveth_adapter *adapter = m->private;
> +	int i, j;
> +
> +	seq_puts(m, "Queue  Pool  Size  BuffSize  Active  Available\n");
> +	seq_puts(m, "-----  ----  ----  --------  ------  ---------\n");
> +
> +	for (i = 0; i < adapter->num_rx_queues; i++) {
> +		for (j = 0; j < IBMVETH_NUM_BUFF_POOLS; j++) {
> +			struct ibmveth_buff_pool *pool =
> +				&adapter->rx_buff_pool[i][j];

[Severity: Low]
Should this dump be taken under rtnl_lock()?

ibmveth_buffer_pools_show() reads adapter->num_rx_queues plus
pool->size, pool->buff_size, pool->active and pool->available with no
lock held, but every writer of those fields runs under RTNL:

veth_pool_store() {
	...
	rtnl_lock();
	...
	pool->active = newactive;
	pool->buff_size = newbuff_size;
	pool->size = newsize;
	...
	/* on ibmveth_open() failure, the same three fields are
	 * written back with the old values */
			pool->active = oldactive;
			pool->buff_size = oldbuff_size;
			pool->size = oldsize;
	...
}

ibmveth_alloc_buffer_pools() likewise copies the queue-0 geometry into
queues 1..N-1 field by field under RTNL from ibmveth_open(), and the
reset path goes through dev_close()/dev_open() under rtnl_lock().

Can a concurrent read of buffer_pools therefore print a torn snapshot,
for example active=1 paired with the previous size/buff_size, or a
half-copied per-queue template, or the transient rollback state that was
never an actual configuration?

There does not appear to be a memory-safety consequence here, since
rx_buff_pool[] is a fixed-size array embedded in struct ibmveth_adapter
and only scalar members are read, so the concern is limited to the
values printed.

> +
> +			seq_printf(m, "%5d  %4d  %4u  %8u  %6d  %9d\n",
> +				   i, j, pool->size, pool->buff_size,
> +				   pool->active,
> +				   atomic_read(&pool->available));

[Severity: Low]
Are Active and Available meaningful while the interface is down?

ibmveth_free_buffer_pool() frees free_map, unmaps and frees all
skbuffs, and frees dma_addr/skbuff, but leaves pool->active set and
never decrements pool->available:

	kfree(pool->free_map);
	pool->free_map = NULL;
	...
	if (pool->skbuff) {
		kfree(pool->skbuff);
		pool->skbuff = NULL;
	}
	/* pool->active and pool->available untouched */

available is only cleared later, in ibmveth_alloc_buffer_pool() via
atomic_set(&pool->available, 0) on the next open.

So a read after ibmveth_close() -> ibmveth_free_buffer_pools() ->
ibmveth_free_queue_buffer_pools() -> ibmveth_free_buffer_pool() reports
every configured pool as Active=1 with the last non-zero Available,
even though the skbuffs and DMA mappings are gone.

Related: for queues 1..N-1 the table prints all-zero rows before the
first open, because the queue-0 template is only copied into those
queues inside ibmveth_alloc_buffer_pools() during open.

The commit message says "Per-queue runtime pressure is what debugfs
shows" - could the down/uninitialized cases be either reflected in the
output or documented?

> +		}
> +	}
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(ibmveth_buffer_pools);
> +
> +static void ibmveth_debugfs_init(struct ibmveth_adapter *adapter)
> +{
> +	adapter->debugfs_dir = debugfs_create_dir(adapter->netdev->name,
> +						  NULL);
> +	debugfs_create_file("buffer_pools", 0400, adapter->debugfs_dir,
> +			    adapter, &ibmveth_buffer_pools_fops);
> +}

[Severity: Medium]
Is the mutable interface name a safe key for a directory created
directly in the debugfs root?

Three separate things follow from passing NULL as the parent and
netdev->name as the name.

First, the name goes stale. The directory is created once, as the last
statement of ibmveth_probe(), and the driver installs no netdev
notifier and never calls debugfs_rename(). After the udev/systemd
predictable-name rename that happens right after register_netdev(), or
after any later ip link set dev ethN name foo, does the debugfs
directory keep the obsolete name for the lifetime of the device?

Second, with the debugfs root as parent the name has to be globally
unique there. Consider the sequence: first adapter probes as eth0, udev
renames it to enP1p1s0, a second ibmveth adapter probes and
alloc_etherdev_mqs() names it eth0 again because eth0 is free in the
netdev namespace. debugfs_create_dir("eth0", NULL) then collides with
the first adapter's stale directory:

fs/debugfs/inode.c:debugfs_start_creating() {
	...
	dentry = simple_start_creating(parent, name);
	if (IS_ERR(dentry)) {
		if (dentry == ERR_PTR(-EEXIST))
			pr_err("'%s' already exists in '%pd'\n", name, parent);
	...
}

The ERR_PTR is stored in adapter->debugfs_dir and handed to
debugfs_create_file() as the parent, which returns immediately:

fs/debugfs/inode.c:debugfs_start_creating() {
	...
	if (IS_ERR(parent))
		return parent;
	...
}

Does the second and every subsequent adapter then end up with no
buffer_pools file at all, with only that debugfs pr_err as an
indication?

Third, netdev->name is read here after register_netdev() has published
the device, without rtnl_lock(), while dev_change_name() rewrites
net_device->name under RTNL. Can the string handed to
debugfs_create_dir() be a partially updated name?

Would a driver-owned parent directory plus a stable identifier such as
dev_name(&dev->dev) avoid all three?

> +
> +static void ibmveth_debugfs_exit(struct ibmveth_adapter *adapter)
> +{
> +	debugfs_remove_recursive(adapter->debugfs_dir);
> +	adapter->debugfs_dir = NULL;
> +}
> +
>  static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter,
>  				  int pools_ready)
>  {
> @@ -3182,6 +3221,8 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  
>  	netdev_dbg(netdev, "registered\n");
>  
> +	ibmveth_debugfs_init(adapter);
> +
>  	return 0;
>  }

[ ... ]

Cross-instance finding from sashiko-gemini (b6bfc50872782421f7178a42615ea39fe861918e607d995ed830a5a2db8a4460):
[Severity: Medium]
Addition of driver-private ethtool -S strings for standard queue statistics.