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.
[ ... ]
> @@ -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?
[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?
