On Wed, Aug 23, 2023 at 09:44:22AM +0200, Maxime Ripard wrote: > Hi Rahul, > > On Tue, Aug 22, 2023 at 08:57:41AM -0700, Rahul Rameshbabu wrote: > > On Tue, 22 Aug, 2023 11:12:28 +0200 Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > > Hi, > > > > > > So, we discussed it this morning with Benjamin, and I think the culprit > > > is that the uclogic driver will allocate a char array with devm_kzalloc > > > in uclogic_input_configured() > > > (https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-uclogic-core.c#L149), > > > and will assign input_dev->name to that pointer. > > > > > > When the device is removed, the devm-allocated array is freed, and the > > > input framework will send a uevent in input_dev_uevent() using the > > > input_dev->name field: > > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1688 > > > > > > So it's a classic dangling pointer situation. > > > > > > And even though it was revealed by that patch, I think the issue is > > > unrelated. The fundamental issue seems to be that the usage of devm in > > > that situation is wrong. > > > > > > input_dev->name is accessed by input_dev_uevent, which for KOBJ_UNBIND > > > and KOBJ_REMOVE will be called after remove. > > > > > > For example, in __device_release_driver() (with the driver remove hook > > > being called in device_remove() and devres_release_all() being called in > > > device_unbind_cleanup()): > > > https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L1278 > > > > > > So, it looks to me that, with or without the patch we merged recently, > > > the core has always sent uevent after device-managed resources were > > > freed. Thus, the uclogic (and any other input driver) was wrong in > > > allocating its input_dev name with devm_kzalloc (or the phys and uniq > > > fields in that struct). > > > > > > Note that freeing input_dev->name in remove would have been just as bad. > > > > > > Looking at the code quickly, at least hid-playstation, > > > hid-nvidia-shield, hid-logitech-hidpp, mms114 and tsc200x seem to be > > > affected by the same issue. > > > > I agree with this analysis overall. At least in hid-nvidia-shield, I can > > not use devm for allocating the input name string and explicitly free it > > after calling input_unregister_device. In this scenario, the name string > > would have been freed explicitly after input_put_device was called > > (since the input device is not devres managed). input_put_device would > > drop the reference count to zero and the device would be cleaned up at > > that point triggering KOBJ_REMOVE and firing off that final > > input_dev_uevent. > > > > I think this can be done for a number of the drivers as a workaround > > till this issue is properly resolved. If this seems appropriate, I can > > send out a series later in the day. This is just a workaround till the > > discussion below converges (which I am interested in). > > I'm sorry, I don't know the input framework well enough to understand > what you had in mind exactly. Could you send a patch with your > suggestion for the hid-nvidia-shield so we can discuss this further? > > That being said, I think that the current design around name, phys and > uniq is fairly treacherous to drivers and we should aim for a solution > that prevents that issue from being possible at all. > > I was inclined to go for a char array for each to get rid of the pointer > entirely, but Benjamin raised some concerns over the structure size so > it's probably not a great solution. I think everything is much simpler, with uclogic driver being in the wrong here: devm resource needs to be attached to the right device (instance of HID) rather than to the input device itself (which should never have any driver resources attached since it never has a driver). Something like this: diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c index f67835f9ed4c..f234a7c97360 100644 --- a/drivers/hid/hid-uclogic-core.c +++ b/drivers/hid/hid-uclogic-core.c @@ -148,7 +148,7 @@ static int uclogic_input_configured(struct hid_device *hdev, if (suffix) { len = strlen(hdev->name) + 2 + strlen(suffix); - name = devm_kzalloc(&hi->input->dev, len, GFP_KERNEL); + name = devm_kzalloc(&hdev->dev, len, GFP_KERNEL); if (name) { snprintf(name, len, "%s %s", hdev->name, suffix); hi->input->name = name; In general, drivers should attach devm resources they allocate to the instance of device they are binding to, and nothing else. Thanks. -- Dmitry
