Messages in this thread | | | From | Benjamin Tissoires <> | Date | Fri, 10 Feb 2023 15:11:26 +0100 | Subject | Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers |
| |
On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <hdanton@sina.com> wrote: > > On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <borrello@diag.uniroma1.it> > > Use spinlocks to deal with workers introducing a wrapper > > bigben_schedule_work(), and several spinlock checks. > > Otherwise, bigben_set_led() may schedule bigben->worker after the > > structure has been freed, causing a use-after-free. > > > > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal") > > Given the flag added in 4eb1b01de5b9 and the spinlock added in this > patchset, devm_led_classdev_register() looks to not work for you.
Actually, looking at the code now, it is clear that we need that lock. The current code is happily changing the struct bigben_device from multiple contexts, and pulls that without any barrier in the work struct which should produce some interesting results :)
And we can probably abuse that lock to prevent scheduling a new work as it is done in hid-playstation.c
I'll comment in the patch which parts need to be changed, because it is true that this patch is definitely not mergeable as such and will need another revision.
> > How about replacing the advanced devm_ method with the traditional plain > pair of led_classdev_un/register(), with the flag mentioned cut off but > without bothering to add another lock? >
As mentioned above, the lock is needed anyway, and will probably need to be added in a separate patch. Reverting to a non devm version of the led class would complexify the driver for the error paths, and is probably not the best move IMO.
Cheers, Benjamin
| |