Re: [PATCH v2 1/4] driver core: generalize driver_override in struct device

[Frank Li]

From: Frank Li (AI-BOT) <frank.li@xxxxxxx>

AI bot review and may be useless.

> +static ssize_t driver_override_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t count)
> +{
> +	int ret;
> +
> +	ret = __device_set_driver_override(dev, buf, count);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}

Potential issue: if __device_set_driver_override() succeeds but
modifies state, returning 'count' may not reflect actual bytes
consumed if buf contains embedded nulls. Consider documenting
expected behavior or validating input length upfront.

> +static ssize_t driver_override_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	guard(spinlock)(&dev->driver_override.lock);
> +	return sysfs_emit(buf, "%s\n", dev->driver_override.name);
> +}

Potential NULL dereference: if dev->driver_override.name is NULL,
sysfs_emit() will print "(null)". Add explicit check or document
that NULL is acceptable output.

> +	if (sp->bus->driver_override) {
> +		error = device_add_group(dev, &driver_override_dev_group);
> +		if (error)
> +			goto out_groups;
> +	}

Error path bug: on device_add_group() failure, code jumps to
out_groups but should jump to out_override (which doesn't exist
yet). This leaks the sysfs link created below. Reorder or add
intermediate label.

> +	error = sysfs_create_link(&sp->devices_kset->kobj, &dev->kobj, dev_name(dev));
> +	if (error)
> -		goto out_groups;
> +		goto out_override;

Good: label renamed to match new cleanup order.

> +out_override:
> +	if (dev->bus->driver_override)
> +		device_remove_group(dev, &driver_override_dev_group);

Potential issue: dev->bus may be NULL or different from sp->bus at
this point. Use sp->bus for consistency with the add path above.

> +int __device_set_driver_override(struct device *dev, const char *s, size_t len)
> +{
> +	const char *new, *old;
> +	char *cp;
> +
> +	if (!s)
> +		return -EINVAL;

Inconsistency: len parameter is passed but then recalculated via
strlen(s) below. If len is meant to be trusted, don't recalculate.
If not, remove the parameter or document why both exist.

> +	len = strlen(s);

This overwrites the len parameter, making the initial bounds check
at PAGE_SIZE potentially useless. Clarify intent: is len from
userspace or always recomputed?

> +	new = kstrndup(s, len, GFP_KERNEL);
> +	if (!new)
> +		return -ENOMEM;
> +
> +	spin_lock(&dev->driver_override.lock);
> +	old = dev->driver_override.name;
> +	if (cp != s) {
> +		dev->driver_override.name = new;
> +		spin_unlock(&dev->driver_override.lock);
> +	} else {
> +		/* "\n" passed - clear override */
> +		dev->driver_override.name = NULL;
> +		spin_unlock(&dev->driver_override.lock);
> +
> +		kfree(new);
> +	}
> +	kfree(old);

Logic is correct but confusing: cp is set only if '\n' is found, so
the condition `if (cp != s)` is checking "was newline NOT at start".
Add a comment explaining the two paths (newline-at-start clears,
otherwise