Re: [PATCH net-next v5 5/9] 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: 1 · Medium: 0 · 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).

- [High] kobject: module unload race causing execution-after-free

--

> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1890,6 +1890,40 @@ static const struct net_device_ops ibmveth_netdev_ops = {
>  	.ndo_features_check	= ibmveth_features_check,
>  };
>  
> +/**
> + * ibmveth_pool_kobj_release - Mark a pool kobject finished
> + * @kobj: kobject embedded in the pool
> + *
> + * The pool kobjects live in netdev_priv(), so the last put must wait
> + * for this before free_netdev().
> + */
> +static void ibmveth_pool_kobj_release(struct kobject *kobj)
> +{
> +	struct ibmveth_buff_pool *pool = container_of(kobj,
> +						      struct ibmveth_buff_pool,
> +						      kobj);
> +
> +	complete(&pool->released);
> +}
> +
> +/**
> + * ibmveth_put_pool_kobjs - Drop the pool kobjects and wait for release
> + * @adapter: ibmveth adapter
> + *
> + * With CONFIG_DEBUG_KOBJECT_RELEASE the cleanup, including removing the
> + * sysfs files, runs later from a work item in the kobject; wait for it
> + * so free_netdev() cannot free the pools first.
> + */
> +static void ibmveth_put_pool_kobjs(struct ibmveth_adapter *adapter)
> +{
> +	int i;
> +
> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> +		kobject_put(&adapter->rx_buff_pool[i].kobj);
> +	for (i = 0; i < IBMVETH_NUM_BUFF_POOLS; i++)
> +		wait_for_completion(&adapter->rx_buff_pool[i].released);
> +}
> +
>  static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
>  {
>  	int rc, i, mac_len;

[Severity: High]
With CONFIG_DEBUG_KOBJECT_RELEASE, ibmveth_pool_kobj_release() runs
from the kobject's delayed release work, not from kobject_put(). The
complete(&pool->released) here wakes ibmveth_put_pool_kobjs() in
ibmveth_remove(). Remove can then return and let
vio_unregister_driver() and the module unload finish while the worker
is still inside ibmveth_pool_kobj_release(), on its way back from
complete(). That CPU then executes freed module text. So rmmod can
oops in exactly the configuration this v2 was meant to make safe.

Signalling a completion from module code does not by itself prove the
signaller has left the module; that is why kthread_complete_and_exit()
exists.

Waiting in remove() for the final put is also not how kobject lifetime
is meant to work. release() should free the containing object rather
than hand it back to someone blocked on it.

Would it be simpler to drop the separate pool kobjects and create the
pool%d directories as named attribute groups on dev->dev.kobj? Then
nothing with its own refcount lives in netdev_priv(). The trade-off is
that veth_pool_show() and veth_pool_store() would need another way to
find the pool than container_of() on the kobject. How do you want to
handle this?

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