Re: [PATCH] ata: pata_parport: Fix missing module refcount for pata_parport itself

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Circular dependency permanently preventing module unloading.
- [High] Race condition leading to Use-After-Free during device release.
--

--- Patch [1]: [PATCH] ata: pata_parport: Fix missing module refcount for pata_parport itself ---
commit 35d67ec0c667d20f10f4b6d490918bd71ccafa32
Author: Pei Xiao <xiaopei01@kylinos.cn>

ata: pata_parport: Fix missing module refcount for pata_parport itself

This commit adds a module reference count for the pata_parport core module
during device initialization and drops it during device release. This is
intended to prevent the module from being unloaded while devices are still
attached.

> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 40baeac594a9f..6de237602a13e 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -455,6 +455,12 @@ static void pata_parport_dev_release(struct device *dev)
>  {
>  	struct pi_adapter *pi = container_of(dev, struct pi_adapter, dev);
>  
> +	/*
> +	 * pi->proto is NULL when device_register() fails early,
> +	 * before __module_get() was called -- skip module_put().
> +	 */
> +	if (pi->proto)
> +		module_put(THIS_MODULE);

[Severity: High]
Does this create a race condition where the module could be unloaded while
pata_parport_dev_release() is still executing?

If module_put() drops the final reference to the module, a concurrent module
unload could unmap the module's memory. The remaining instructions in this
function, including the call to ida_free() which accesses the static
pata_parport_bus_dev_ids structure, would then be executing in freed memory.

>  	ida_free(&pata_parport_bus_dev_ids, dev->id);
>  	kfree(pi);
>  }

[ ... ]

> @@ -524,6 +530,8 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
>  
>  	pi->proto = pr;
>  
> +	__module_get(THIS_MODULE);
> +

[Severity: High]
Will this prevent the module from ever being unloaded?

Normally, the driver's exit function (pata_parport_exit) would unregister the
driver, which in turn detaches devices and drops their references. By taking a
module reference for every attached device, the module reference count will
never reach zero while devices exist, making pata_parport_exit() unreachable
via standard unload operations.

>  	if (!try_module_get(pi->proto->owner))
>  		goto out_unreg_dev;
>  	if (pi->proto->init_proto && pi->proto->init_proto(pi) < 0)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/a8d9807618ab786313654099c904d613075e1b63.1785398816.git.xiaopei01@kylinos.cn?part=1