On Tue, 22 Aug, 2023 08:57:41 -0700 Rahul Rameshbabu <rrameshbabu@xxxxxxxxxx> wrote: > Hi Maxime, > > 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). After thinking about it a bit further, I believe hid-nvidia-shield is not impacted by this issue in its current state. hid_hw_stop will trigger devres_release_all, which will free the input name allocated. This is problematic in the case that the input_dev is devres managed, since device resource management will hook the uevent callback on device_del triggered by input_unregister_device invoked by hid_hw_stop. However, hid-nvidia-shield does not use devm to allocate the input device and explicitly calls input_unregister_device before the call to hid_hw_stop. I believe this leads to the uevent being fired before name is freed by devres after the hid_hw_stop call. Let me know if this analysis seems off. Hoping this can be helpful in general if this is the case. Freed by task 4508: kasan_save_stack+0x33/0x50 mm/kasan/common.c:45 kasan_set_track+0x25/0x30 mm/kasan/common.c:52 kasan_save_free_info+0x2b/0x40 mm/kasan/generic.c:522 ____kasan_slab_free mm/kasan/common.c:236 [inline] ____kasan_slab_free+0x15b/0x1b0 mm/kasan/common.c:200 kasan_slab_free include/linux/kasan.h:164 [inline] slab_free_hook mm/slub.c:1800 [inline] slab_free_freelist_hook+0x114/0x1e0 mm/slub.c:1826 slab_free mm/slub.c:3809 [inline] __kmem_cache_free+0xb8/0x2f0 mm/slub.c:3822 release_nodes drivers/base/devres.c:506 [inline] devres_release_all+0x192/0x240 drivers/base/devres.c:535 device_del+0x628/0xa50 drivers/base/core.c:3827 input_unregister_device+0xb9/0x100 drivers/input/input.c:2440 hidinput_disconnect+0x160/0x3e0 drivers/hid/hid-input.c:2386 hid_disconnect+0x143/0x1b0 drivers/hid/hid-core.c:2273 hid_hw_stop+0x16/0x70 drivers/hid/hid-core.c:2322 uclogic_remove+0x47/0x90 drivers/hid/hid-uclogic-core.c:485 > >> >> We discussed a couple of solutions with Benjamin, such as creating a >> helper devm action to free and clear the input_dev->name field, droping >> the name, phys and uniq fields from the uevent, or converting name, phys >> and uniq to char arrays so drivers don't have to allocate them. >> >> We couldn't find a perfect one though, so... yeah. >> >> Maxime > -- Thanks, Rahul Rameshbabu
