| Message ID | 20231207063406.556770-3-vi@endrift.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Input: uinput - Multiple concurrency fixes in ff request handling | expand |
Hi Vicki, On Wed, Dec 06, 2023 at 10:34:06PM -0800, Vicki Pfau wrote: > Any pending requests may be holding a mutex from its own subsystem, e.g. > evdev, while waiting to be able to claim the uinput device mutex. > However, unregistering the device may try to claim that mutex, leading > to a deadlock. To prevent this from happening, we need to temporarily > give up the lock before calling input_unregister_device. I do not think we can simply give up the lock, the whole thing with UI_DEV_DESTROY allowing reusing connection to create a new input device was a huge mistake because if you try to do UI_DEV_CREATE again on the same fd you'll end up reusing whatever is in udev instance, including the state and the mutex, and will make a huge mess. I think the only reasonable way forward is change the driver so that no ioctls are accepted after UI_DEV_DESTROY and then start untangling the locking issues (possibly by dropping the lock on destroy after setting the status - I think you will not observe the lockups you mention if your application will stop using UI_DEV_DESTROY and simply closes the fd). > > Fixes: e8b95728f724 ("Input: uinput - avoid FF flush when destroying device") This is not the commit that introduced the problem, it has been there since forever. Thanks.
Hi Dmitry, On 12/8/23 11:58, Dmitry Torokhov wrote: > Hi Vicki, > > On Wed, Dec 06, 2023 at 10:34:06PM -0800, Vicki Pfau wrote: >> Any pending requests may be holding a mutex from its own subsystem, e.g. >> evdev, while waiting to be able to claim the uinput device mutex. >> However, unregistering the device may try to claim that mutex, leading >> to a deadlock. To prevent this from happening, we need to temporarily >> give up the lock before calling input_unregister_device. > > I do not think we can simply give up the lock, the whole thing with > UI_DEV_DESTROY allowing reusing connection to create a new input device > was a huge mistake because if you try to do UI_DEV_CREATE again on > the same fd you'll end up reusing whatever is in udev instance, > including the state and the mutex, and will make a huge mess. Yeah, I was curious why this was possible in the first place. It seemed overcomplicated compared to just opening a new fd. I suppose that that makes more sense, though it's a bit involved for this. > > I think the only reasonable way forward is change the driver so that no > ioctls are accepted after UI_DEV_DESTROY and then start untangling the > locking issues (possibly by dropping the lock on destroy after setting > the status - I think you will not observe the lockups you mention if > your application will stop using UI_DEV_DESTROY and simply closes the > fd). This does sound like a reasonable way forward. Unfortunately, I don't have access to the uinput-side application code, but I have been trying to work with them to flatten out bugs in it. I can pass this suggestion along, though there is still a reproducible deadlock that could theoretically happen with other programs in the meantime (though the likelihood of it being hit without actively trying for it is low). > >> >> Fixes: e8b95728f724 ("Input: uinput - avoid FF flush when destroying device") > > This is not the commit that introduced the problem, it has been there > since forever. My mistake. If I prepare a v2, which I may not, I'll drop the line. > > Thanks. > Vicki
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index 0330e72798db..ac6e5baa2093 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -296,17 +296,34 @@ static void uinput_destroy_device(struct uinput_device *udev) udev->state = UIST_NEW_DEVICE; if (dev) { + udev->dev = NULL; name = dev->name; phys = dev->phys; if (old_state == UIST_CREATED) { uinput_flush_requests(udev); + + /* + * Any pending requests may be holding a mutex from its + * own subsystem, e.g. evdev, while waiting to be able + * to claim the uinput device mutex. However, + * unregistering the device may try to claim that + * mutex, leading to a deadlock. To prevent this from + * happening, we need to temporarily give up the lock. + * + * Since this function is only called immediately + * before the caller exits the critical section without + * doing any further operations on the device, this + * is safe and we can immediately reclaim the mutex + * when done so the unlock is still balanced. + */ + mutex_unlock(&udev->mutex); input_unregister_device(dev); + mutex_lock(&udev->mutex); } else { input_free_device(dev); } kfree(name); kfree(phys); - udev->dev = NULL; } } @@ -745,7 +762,16 @@ static int uinput_release(struct inode *inode, struct file *file) { struct uinput_device *udev = file->private_data; + int retval; + + retval = mutex_lock_interruptible(&udev->mutex); + if (retval) + return retval; + uinput_destroy_device(udev); + + mutex_unlock(&udev->mutex); + kfree(udev); return 0;
Any pending requests may be holding a mutex from its own subsystem, e.g. evdev, while waiting to be able to claim the uinput device mutex. However, unregistering the device may try to claim that mutex, leading to a deadlock. To prevent this from happening, we need to temporarily give up the lock before calling input_unregister_device. Fixes: e8b95728f724 ("Input: uinput - avoid FF flush when destroying device") Signed-off-by: Vicki Pfau <vi@endrift.com> --- drivers/input/misc/uinput.c | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-)