Re: [PATCH net-next v3 12/15] ibmveth: Expose per-queue buffer pool details via sysfs

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

On 7/14/26 6:01 AM, Simon Horman 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.
> Full review at: https://sashiko.dev
> ---
> ibmveth: Expose per-queue buffer pool details via sysfs
>
> This commit adds a read-only sysfs attribute buffer_pools to the VIO device
> to expose per-queue buffer pool details dynamically. It outputs runtime per-queue
> buffer pressure metrics like size, active, and available buffers.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> index 4e3f49b6346fd..ecc472ee8f71f 100644
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
>> @@ -2896,6 +2896,52 @@ static const struct net_device_ops ibmveth_netdev_ops = {
> [ ... ]
>> +static ssize_t buffer_pools_show(struct device *dev,
>> +				 struct device_attribute *attr,
>> +				 char *buf)
>> +{
>> +	struct net_device *netdev = dev_get_drvdata(dev);
>> +	struct ibmveth_adapter *adapter = netdev_priv(netdev);
>> +	int len = 0;
>> +	int i, j;
>> +
>> +	len += scnprintf(buf + len, PAGE_SIZE - len,
>> +			 "Queue  Pool  Size  BuffSize  Active  Available\n");
>> +	len += scnprintf(buf + len, PAGE_SIZE - len,
>> +			 "-----  ----  ----  --------  ------  ---------\n");
> [Severity: Medium]
> Does this buffer_pools_show() implementation violate the sysfs "one value
> per file" design rule?
>
> The attribute appears to dump a complex, multi-line ASCII table with headers
> and columns rather than exposing a single value per sysfs file.
*

  Agreed this is more of a diagnostic dump than a sysfs one-value

  attribute. In v4 I’ll move it to debugfs ?

*
>
> [ ... ]
>> @@ -3056,6 +3102,14 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>>   
>>   	netdev_dbg(netdev, "registered\n");
>>   
>> +	rc = sysfs_create_group(&dev->dev.kobj, &ibmveth_attr_group);
>> +	if (rc) {
>> +		netdev_err(netdev, "failed to create sysfs attributes rc=%d\n", rc);
>> +		unregister_netdev(netdev);
>> +		free_netdev(netdev);
>> +		return rc;
>> +	}
> [Severity: High]
> If userspace brings up the interface immediately after register_netdev()
> succeeds, could adapter->work be scheduled before sysfs_create_group() fails?
>
> If this race occurs, unregister_netdev() does not synchronously cancel the
> work.
>
> Could the global workqueue execute the pending adapter->work item on freed
> memory after free_netdev() is called in this ibmveth_probe() error path?
*

  Good catch. remove() cancels the work, but this probe error path

  did not. Will cancel_work_sync() (and clean pool kobjects) before

  free_netdev() in v4.

*
> [Severity: High]
> This is a pre-existing issue in older error paths of ibmveth_probe(), but it
> is duplicated here: does this error path leak the adapter->rx_buff_pool
> kobjects?
>
> Since kobject_put() is not called on the pool kobjects before free_netdev()
> is executed, the kobjects remain attached to the VIO device with dangling
> pointers to the freed adapter memory.
>
> Could subsequent sysfs accesses to those pool kobjects trigger a
> use-after-free?
*

   Yes — that path (and some older probe failure paths) skip

   kobject_put() on the pool objects. I’ll add a common probe

   cleanup helper in v4 so pool kobjects and the reset work are

   always torn down before free_netdev().

*

Thanks,
Mingming