[PATCH v3 0/2] some fixes in pata_parport

[PATCH v3 0/2] some fixes in pata_parport

[Pei Xiao]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=a, Size: 1146 bytes --]

Patch 1 pins the protocol module before the device becomes visible.
Previously try_module_get() ran after device_register(), so a forced
module unload in between left pi->proto dangling from the moment the
device appeared on the bus.

Patch 2：fix parport attach idr_alloc for port numbers > 0

remove this patch:
https://lore.kernel.org/lkml/cec58ea83362df88f516ba7a5b895cf07edd7728.1788867690.git.xiaopei01@kylinos.cn/#t

I have become cautious now, for fear of causing a disaster, so I removed
this patch. The other patches sent this time have undergone simple
compilation tests and simple tests such as rmmod. Since I don't have the
relevant hardware to test this patch, out of caution, I removed it this
time.

in v3:
1.ata: pata_parport: pin the protocol module before device_register() no
  changes
2.Add ata: pata_parport: fix parport attach for port numbers > 0
 
Pei Xiao (2):
  ata: pata_parport: pin the protocol module before device_register()
  ata: pata_parport: fix parport attach for port numbers > 0

 drivers/ata/pata_parport/pata_parport.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] ata: pata_parport: pin the protocol module before device_register()

[Pei Xiao]

pi_init_one() calls device_register() before try_module_get(). Between
these two calls the device is already visible but the module is not
pinned yet, so an unload in this window leaves pi->proto dangling:

    pi_init_one()                       rmmod -f <proto>
    --------------------------------------------------------
    device_register(&pi->dev)
         device visible on the bus
                                           module memory freed
         pi->proto = pr        <- writes into freed memory / dangles
         try_module_get(...)   <- too late, module already gone

Take the module reference before registering the device, and drop it
on the device_register() failure path.

Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/ata/pata_parport/pata_parport.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index cf81a6128f55..046ab7e3adbc 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -509,6 +509,14 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 		return NULL;
 	}
 
+	pi->proto = pr;
+
+	if (!try_module_get(pi->proto->owner)) {
+		kfree(pi);
+		ida_free(&pata_parport_bus_dev_ids, id);
+		return NULL;
+	}
+
 	/* set up pi->dev before pi_probe_unit() so it can use dev_printk() */
 	pi->dev.parent = pata_parport_bus;
 	pi->dev.bus = &pata_parport_bus_type;
@@ -517,15 +525,12 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 	pi->dev.id = id;
 	dev_set_name(&pi->dev, "pata_parport.%u", pi->dev.id);
 	if (device_register(&pi->dev)) {
+		module_put(pi->proto->owner);
 		put_device(&pi->dev);
 		/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 		return NULL;
 	}
 
-	pi->proto = pr;
-
-	if (!try_module_get(pi->proto->owner))
-		goto out_unreg_dev;
 	if (pi->proto->init_proto && pi->proto->init_proto(pi) < 0)
 		goto out_module_put;
 
@@ -568,7 +573,6 @@ static struct pi_adapter *pi_init_one(struct parport *parport,
 		pi->proto->release_proto(pi);
 out_module_put:
 	module_put(pi->proto->owner);
-out_unreg_dev:
 	device_unregister(&pi->dev);
 	/* pata_parport_dev_release will do ida_free(dev->id) and kfree(pi) */
 	return NULL;
-- 
2.25.1

Re: [PATCH v3 1/2] ata: pata_parport: pin the protocol module before device_register()

[Niklas Cassel]

Hello Pei,

On Tue, Sep 22, 2026 at 11:38:00AM +0800, Pei Xiao wrote:
> pi_init_one() calls device_register() before try_module_get(). Between
> these two calls the device is already visible but the module is not
> pinned yet, so an unload in this window leaves pi->proto dangling:
> 
>     pi_init_one()                       rmmod -f <proto>
>     --------------------------------------------------------
>     device_register(&pi->dev)
>          device visible on the bus
>                                            module memory freed
>          pi->proto = pr        <- writes into freed memory / dangles
>          try_module_get(...)   <- too late, module already gone
> 
> Take the module reference before registering the device, and drop it
> on the device_register() failure path.
> 
> Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>

Looking at the code, this race cannot happen.

All three callers hold pi_mutex throughout pi_init_one().

pata_parport_unregister_driver() also acquires the mutex
before unregistering.

Thus, unload cannot complete at the same time as pi_init_one().
So the motivation looks wrong.

Additionally, rmmod -f bypasses a non-zero module reference count,
so I don't see how taking a refcount earlier would solve a rmmod -f.

Are you building your kernel with CONFIG_MODULE_FORCE_UNLOAD ?


Kind regards,
Niklas

Re: [PATCH v3 1/2] ata: pata_parport: pin the protocol module before device_register()

[Pei Xiao]

在 2026/9/22 22:53, Niklas Cassel 写道:
> Hello Pei,
> 
> On Tue, Sep 22, 2026 at 11:38:00AM +0800, Pei Xiao wrote:
>> pi_init_one() calls device_register() before try_module_get(). Between
>> these two calls the device is already visible but the module is not
>> pinned yet, so an unload in this window leaves pi->proto dangling:
>>
>>     pi_init_one()                       rmmod -f <proto>
>>     --------------------------------------------------------
>>     device_register(&pi->dev)
>>          device visible on the bus
>>                                            module memory freed
>>          pi->proto = pr        <- writes into freed memory / dangles
>>          try_module_get(...)   <- too late, module already gone
>>
>> Take the module reference before registering the device, and drop it
>> on the device_register() failure path.
>>
>> Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> 
> Looking at the code, this race cannot happen.
> 
> All three callers hold pi_mutex throughout pi_init_one().
> 
> pata_parport_unregister_driver() also acquires the mutex
> before unregistering.
> 
> Thus, unload cannot complete at the same time as pi_init_one().
> So the motivation looks wrong.
> 
> Additionally, rmmod -f bypasses a non-zero module reference count,
> so I don't see how taking a refcount earlier would solve a rmmod -f.
Yes, you're right — force unload doesn't really solve this issue. Thanks
for pointing it out.
  Thanks for your reply, and sorry for the noise.

Pei.
Thanks.
> 
> Are you building your kernel with CONFIG_MODULE_FORCE_UNLOAD ?
> 
> 
> Kind regards,
> Niklas

[PATCH v3 2/2] ata: pata_parport: fix parport attach idr_alloc for

[Pei Xiao]

idr_alloc() expects an inclusive start and an exclusive end, so passing
port->number as both arguments produces an empty range. Because
idr_alloc() only falls back to INT_MAX when end <= 0, the allocation
succeeds solely for parport0 and returns -ENOSPC for every other port,
which pata_parport_attach() silently ignores. As a result only
parport0 gets probed and any higher-numbered parports are never
attached.

Use port->number + 1 as the end so the range [port->number,
port->number + 1) contains exactly the desired ID.

Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
---
 drivers/ata/pata_parport/pata_parport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
index 046ab7e3adbc..6f4e3f47d63e 100644
--- a/drivers/ata/pata_parport/pata_parport.c
+++ b/drivers/ata/pata_parport/pata_parport.c
@@ -727,7 +727,7 @@ static void pata_parport_attach(struct parport *port)
 	int pr_num, id;
 
 	mutex_lock(&pi_mutex);
-	id = idr_alloc(&parport_list, port, port->number, port->number,
+	id = idr_alloc(&parport_list, port, port->number, port->number + 1,
 		       GFP_KERNEL);
 	if (id < 0) {
 		mutex_unlock(&pi_mutex);
-- 
2.25.1

Re: [PATCH v3 2/2] ata: pata_parport: fix parport attach idr_alloc for

[Niklas Cassel]

On Tue, Sep 22, 2026 at 11:38:01AM +0800, Pei Xiao wrote:
> idr_alloc() expects an inclusive start and an exclusive end, so passing
> port->number as both arguments produces an empty range. Because
> idr_alloc() only falls back to INT_MAX when end <= 0, the allocation
> succeeds solely for parport0 and returns -ENOSPC for every other port,
> which pata_parport_attach() silently ignores. As a result only
> parport0 gets probed and any higher-numbered parports are never
> attached.
> 
> Use port->number + 1 as the end so the range [port->number,
> port->number + 1) contains exactly the desired ID.
> 
> Fixes: 246a1c4c6b7f ("ata: pata_parport: add driver (PARIDE replacement)")
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
>  drivers/ata/pata_parport/pata_parport.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 046ab7e3adbc..6f4e3f47d63e 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c
> @@ -727,7 +727,7 @@ static void pata_parport_attach(struct parport *port)
>  	int pr_num, id;
>  
>  	mutex_lock(&pi_mutex);
> -	id = idr_alloc(&parport_list, port, port->number, port->number,
> +	id = idr_alloc(&parport_list, port, port->number, port->number + 1,
>  		       GFP_KERNEL);

This patch looks correct to me.


Kind regards,
Niklas

Re: (subset) [PATCH v3 0/2] some fixes in pata_parport

[Niklas Cassel]

On Tue, 22 Sep 2026 11:37:59 +0800, Pei Xiao wrote:
> Patch 1 pins the protocol module before the device becomes visible.
> Previously try_module_get() ran after device_register(), so a forced
> module unload in between left pi->proto dangling from the moment the
> device appeared on the bus.
> 
> Patch 2：fix parport attach idr_alloc for port numbers > 0
> 
> [...]

Applied to libata/linux.git (for-7.4), thanks!

[2/2] ata: pata_parport: fix parport attach idr_alloc for
      https://git.kernel.org/libata/linux/c/193f5b84

Kind regards,
Niklas