some fixes in pata_parport

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

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.

---
 drivers/ata/pata_parport/pata_parport.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

index cf81a6128f55..046ab7e3adbc 100644
@@ -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

Hello Pei,

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

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.

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.

---
 drivers/ata/pata_parport/pata_parport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 046ab7e3adbc..6f4e3f47d63e 100644
@@ -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

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