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

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

On 10/3/26 7:12 PM, netdev-bot+sashiko@kernel.org wrote:
> 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.

Yes. v2 adds a release() for the pool kobjects and waits for it before
free_netdev() in both probe and remove():

|https://lore.kernel.org/netdev/cover.1791178212.git.mmc@linux.ibm.com/|


pw-bot: cr

Thanks,

Mingming