Re: [PATCH 4/7] kunit: platform: catch duplicate (name, id) in kunit_platform_device_add()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Asymmetric error handling in `kunit_platform_device_add()` creates a double-free or memory leak trap for unmanaged devices.
- [Medium] The duplicate device check falsely rejects explicit device IDs that happen to match the internal ID of an auto-allocated device.
--

commit eaa71e70d7a40c957322770b8d98b24b4def8073
Author: Jia He <justin.he@arm.com>
kunit: platform: catch duplicate (name, id) in kunit_platform_device_add()

This commit adds an early check in kunit_platform_device_add() to detect
duplicate platform devices before they trigger a sysfs warning stack trace.
It walks the platform bus to find matching names and IDs and returns
-EEXIST if a collision is found.

> diff --git a/lib/kunit/platform.c b/lib/kunit/platform.c
> index 0b518de26065d..515f7c859014a 100644
> --- a/lib/kunit/platform.c
> +++ b/lib/kunit/platform.c

[ ... ]

> @@ -80,6 +81,20 @@ kunit_platform_device_alloc_match(struct kunit *test,
>  
>  KUNIT_DEFINE_ACTION_WRAPPER(platform_device_unregister_wrapper,
>  			    platform_device_unregister, struct platform_device *);
> +
> +struct kunit_pdev_dup_match {
> +	const char *name;
> +	int id;
> +};
> +
> +static int kunit_pdev_dup_match_fn(struct device *dev, void *data)
> +{
> +	struct kunit_pdev_dup_match *m = data;
> +	struct platform_device *p = to_platform_device(dev);
> +
> +	return p->id == m->id && p->name && !strcmp(p->name, m->name);
> +}
> +

Could this match function falsely reject explicit device IDs that happen to
match the internal ID of an auto-allocated device?

When platform_device_add() registers a device with PLATFORM_DEVID_AUTO, it
allocates an ID and sets pdev->id_auto to true, appending an ".auto" suffix
to its sysfs name.

If a KUnit test registers an explicit device that happens to share a name
and ID with an existing auto-allocated device, this match function will flag
a collision and return -EEXIST, even though their sysfs names are distinct
and platform_device_add() would have succeeded.

>  /**
>   * kunit_platform_device_add() - Register a KUnit test managed platform device
>   * @test: test context
> @@ -95,6 +110,22 @@ int kunit_platform_device_add(struct kunit *test, struct platform_device *pdev)
>  	struct kunit_resource *res;
>  	int ret;
>  
> +	/*
> +	 * Detect duplicate (name, id) registrations early, before
> +	 * platform_device_add() reaches sysfs_create_dir_ns() and
> +	 * unconditionally dumps a stack trace via sysfs_warn_dup(). This keeps
> +	 * tests that intentionally exercise the duplicate-add failure path
> +	 * (e.g. kunit_platform_device_add_twice_fails_test) quiet without
> +	 * losing the negative return value they assert on.
> +	 */
> +	if (pdev->name && pdev->id != PLATFORM_DEVID_AUTO) {
> +		struct kunit_pdev_dup_match m = { pdev->name, pdev->id };
> +
> +		if (bus_for_each_dev(&platform_bus_type, NULL, &m,
> +				     kunit_pdev_dup_match_fn))
> +			return -EEXIST;
> +	}
> +

Does this new error path create an ambiguous state for unmanaged devices on
failure?

If kunit_platform_device_add() is called with an unmanaged device and
succeeds here, but later fails during kunit_add_action_or_reset(), the
action wrapper is automatically executed to unregister and free the device.

However, if it fails early in this new duplicate check, the device is not
freed. The caller has no way to distinguish which step failed, meaning
unconditionally calling platform_device_put() on error causes a double-free
if it failed later, while not calling it leaks the platform device if it
failed early.

>  	ret = platform_device_add(pdev);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514050455.2954509-1-justin.he@arm.com?part=4