Re: [PATCH v2 2/2] Add TAINT_DRIVER_OVERRIDE for usage of driver_override

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Sep 28, 2026 at 10:50 AM CEST, Uwe Kleine-König wrote:
> On Sun, Sep 27, 2026 at 12:03:54PM +0200, Danilo Krummrich wrote:
>> On Sun Sep 27, 2026 at 11:55 AM CEST, Danilo Krummrich wrote:
>> > On Sun Sep 27, 2026 at 10:03 AM CEST, Uwe Kleine-König wrote:
>> >> Commit fcbfaffee51a ("driver core: add TAINT_FORCED_BIND for when
>> >> userspace manually messes with devices and drivers") introduced a taint
>> >> for usage of bind/unbind sysfs files that manually trigger driver probe
>> >> and remove respectively.
>> >>
>> >> For drivers that do their resource management correctly (which is also
>> >> needed for module unloading) bind and unbind for matching devices are
>> >> not critical operations. The thing that makes bind and unbind unsafe is
>> >> that drivers can be forced on devices that originally don't match using
>> >> driver_override. The result is that e.g. of_device_get_match_data()
>> >> returns NULL despite all .of_match_table entries having a non-NULL
>> >> .driver_data member which yields a NULL pointer exception for several
>> >> drivers. And given that after setting a driver_override a manual bind is
>> >> only one way a driver can be bound to an unexpected device, a separate
>> >> taint for such an override is justified.
>> >>
>> >> Reviewed-by: Bradley Morgan <brads@mainlining.org>
>> >> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
>> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>> >
>> > Suggested-by: Danilo Krummrich <dakr@kernel.org>
>> > Link: https://lore.kernel.org/driver-core/DLIL9H50MALI.3JROXYEEUM3KU@kernel.org/
>
> I came up with the idea on my own, but ok, will add that reference.

Sure, I didn't mean to suggest you couldn't have arrived at it independently. I
offered the tag because I'd proposed it in the earlier discussion and you agreed
with it there.

>> >> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
>> >> index c51ad96d4de4..7d5dc016a457 100644
>> >> --- a/drivers/base/bus.c
>> >> +++ b/drivers/base/bus.c
>> >> @@ -513,6 +513,7 @@ static ssize_t driver_override_store(struct device *dev,
>> >>  {
>> >>  	int ret;
>> >>  
>> >> +	add_taint_module(NULL, TAINT_DRIVER_OVERRIDE, LOCKDEP_STILL_OK);
>> >>  	ret = __device_set_driver_override(dev, buf, count);
>> >
>> > There are buses (such as SPI) which unfortunately have to call
>> > __device_set_driver_override() directly.
>> >
>> > I think it would be better to move the taint into __device_set_driver_override()
>> > and properly document the purpose of __device_set_driver_override().
>> 
>> Of course I meant to say to create a new forwarding function for this purpose,
>> such that we do not taint for device_set_driver_override().
>
> What is the rationale to exclude device_set_driver_override()?

It's used / abused within subsystems to hardwire stuff, which should indeed be
fixed, but also doesn't justify the taint, as it is not userspace controlling
it.

> For the dynamic spi device creation I like it to trigger the taint.

Oh, there's a new usage in new_device_store() as of v7.3-rc1. I wasn't aware of
this. I'd just make it call the device_set_driver_override_store() function
then, as it is exactly the same class of issue.

> For
> sound/soc/samsung/i2s.c it looks as if device_set_driver_override() is
> just the lazy way to make the created device bind and there is no reason
> to stick to normal binding. And why does it call device_attach()?
> Shouldn't that trigger automatically after platform_device_add()?
> Also in drivers/slimbus/qcom-ngd-ctrl.c the call to
> device_set_driver_override() seems redundant.
>
>> Maybe device_store_driver_override() or device_set_driver_override_store()?
>> 
>> > It only exists as SPI and AP are a bit special; both print "\n" when
>> > driver_override is not set, whereas all other buses (and thus the driver-core)
>> > produce "(null)\n" in this case. I.e. it should never get any new users.
>
> I guess it's API and thus hardly changable, but I like "\n" better, and
> if it's only because "(null)" might be a driver name and there is no way
> to distinguish the situation after
>
> 	echo '(null)' > driver_override
>
> from the normal state.

I would have preferred "\n" too, but I don't want to mess with this uAPI. When I
implemented the driver_override generalization I looked it up (e.g. in [1]) and
tools seem to care about this. driverctl handles both cases, as it is bus
agnostic, but I don't know what else exists.

[1] https://gitlab.com/driverctl/driverctl/-/blob/0.121/driverctl?ref_type=tags#L99