Re: [PATCH net-next 5/7] ibmveth: release the pool kobjects when probe fails

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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] On the new err_put_pools path in ibmveth_probe()
  (drivers/net/ethernet/ibm/ibmveth.c), the code calls kobject_put() on…

--

ibmveth: release the pool kobjects when probe fails

This commit adds an err_put_pools unwind label to ibmveth_probe(). The label
calls kobject_put() on each rx_buff_pool[i].kobj before free_netdev(). It is
used when either netif_set_real_num_tx_queues() or register_netdev() fails.

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

[ ... ]

> @@ -1981,13 +1980,18 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  
>  	if (rc) {
>  		netdev_dbg(netdev, "failed to register netdev rc=%d\n", rc);
> -		free_netdev(netdev);
> -		return rc;
> +		goto err_put_pools;
>  	}
>  
>  	netdev_dbg(netdev, "registered\n");
>  
>  	return 0;
> +
> +err_put_pools:
> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> +		kobject_put(&adapter->rx_buff_pool[i].kobj);
> +	free_netdev(netdev);
> +	return rc;
>  }

[Severity: Medium]
With CONFIG_DEBUG_KOBJECT_RELEASE=y, can this path still free the pool
kobjects while their cleanup is pending?

The kobjects are embedded in netdev_priv(netdev), and ktype_veth_pool has
no release() callback:

static struct kobj_type ktype_veth_pool = {
	.release        = NULL,
	.sysfs_ops      = &veth_pool_ops,
	...
};

On that config, kobject_release() defers the cleanup to a delayed_work
that is embedded in the kobject itself:

lib/kobject.c:kobject_release() {
	...
	INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);

	schedule_delayed_work(&kobj->release, delay);
	...
}

free_netdev() then frees the adapter while that work is still armed. The
work runs 1 to 4 seconds later. kobject_cleanup() then works on freed
memory: it reads kobj->state_in_sysfs and kobj->name, calls
__kobject_del(), and drops the parent reference.

Until the work runs, the pool%d active, num and size files under
/sys/devices/vio/<unit>/ still exist. A read or write goes through
veth_pool_show() or veth_pool_store(). Both use container_of() to reach
the freed pool, adapter and netdev.

A re-probe in that window could also fail to add pool0. That is the same
failure this commit is meant to fix.

The commit message mentions this and refers to ibmveth_remove(), which
uses the same put-then-free pattern. kobject_cleanup() also reports this
ktype as one that "does not have a release() function, it is broken and
must be fixed".

Calling kobject_del() before kobject_put() would remove the sysfs files
synchronously. However, the delayed_work inside the freed adapter would
still be armed.

Would it make sense to give the pool kobjects their own lifetime? One
option is to allocate them separately with a real release() callback.

This also stays the same later in the series. "ibmveth: wait for
in-flight transmits in ibmveth_close()" leaves both err_put_pools and the
NULL release in place.

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