Re: [PATCH] test: ata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The commit message is completely devoid of technical details, failing to describe the changes, their purpose, or the problem being solved.

Pre-existing issues:
- [High] pata_parport_unregister_driver() does not unregister the devices associated with the unloaded protocol, leaving dangling devices on the bus that hold freed pointers.
--

--- Patch [1]: [PATCH] test: ata ---
commit 787e1d65e64282ddc25d6c43593b6cd3674b9345
Author: Pei Xiao <xiaopei01@kylinos.cn>

test: ata

This commit modifies locking in pata_parport by adopting guard(mutex) for
pi_mutex in new_device_store() and extending the mutex_unlock call in
pata_parport_unregister_driver() to also cover driver_unregister().

> test: ata
> 
> hi maintainer and shuangpeng,
> 
> I tested this patch passed.

[Severity: Medium]
Could this commit message be expanded with more technical details?

The message only mentions that the patch was tested, but it does not describe
the functional changes being introduced, such as expanding the lock scope to
protect driver_find() and changing the lock ordering in the unregister path.

Describing the problem and the rationale for the changes would greatly assist
with review and future maintenance.

> diff --git a/drivers/ata/pata_parport/pata_parport.c b/drivers/ata/pata_parport/pata_parport.c
> index 40baeac594a9f..34f09192a4190 100644
> --- a/drivers/ata/pata_parport/pata_parport.c
> +++ b/drivers/ata/pata_parport/pata_parport.c

[ ... ]

> @@ -618,8 +619,9 @@ void pata_parport_unregister_driver(struct pi_protocol *pr)
>  			break;
>  	}
>  	idr_remove(&protocols, id);
> -	mutex_unlock(&pi_mutex);
>  	driver_unregister(&pr->driver);
> +	mutex_unlock(&pi_mutex);
> +
>  }

[Severity: High]
This is a pre-existing issue, but does pata_parport_unregister_driver() leak
devices?

When a protocol module's init function registers multiple protocols (like
kbic_init registering k951 and k971) and a subsequent registration fails, it
will call pata_parport_unregister_driver() on the already-registered protocol.

While the protocol is removed from the IDR and the driver is unregistered, the
dynamically created pi_adapter devices are not cleaned up. Since the module
init returns an error, the module loader frees the module memory, bypassing
the reference held by the devices.

If these dangling devices are later removed (for example, via sysfs
delete_device), pi_remove_one() calls pi_disconnect(pi), which dereferences
the freed pi->proto->disconnect pointer, leading to a kernel crash.

Should the associated devices be unregistered here as well?

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