[PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[tlambert]

From: Terry Lambert <tlambert@chromium.org>

Fixed order of calls in evdev_ungrab to allow iterative use of
code which grabs and releases input event devices.

Signed-off-by: Terry Lambert <tlambert@chromium.org>
---
 drivers/input/evdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c8471a2..0bac8da 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -160,9 +160,9 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
 	if (evdev->grab != client)
 		return  -EINVAL;
 
+	input_release_device(&evdev->handle);
 	rcu_assign_pointer(evdev->grab, NULL);
 	synchronize_rcu();
-	input_release_device(&evdev->handle);
 
 	return 0;
 }
-- 
1.7.3.1

Re: [PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[Dmitry Torokhov]

On Thu, Feb 10, 2011 at 03:42:50PM -0800, tlambert@chromium.org wrote:
> From: Terry Lambert <tlambert@chromium.org>
> 
> Fixed order of calls in evdev_ungrab to allow iterative use of
> code which grabs and releases input event devices.
> 

Sorry, could you  please be more verbose?

> Signed-off-by: Terry Lambert <tlambert@chromium.org>
> ---
>  drivers/input/evdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c8471a2..0bac8da 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -160,9 +160,9 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
>  	if (evdev->grab != client)
>  		return  -EINVAL;
>  
> +	input_release_device(&evdev->handle);
>  	rcu_assign_pointer(evdev->grab, NULL);
>  	synchronize_rcu();
> -	input_release_device(&evdev->handle);
>  
>  	return 0;
>  }
> -- 
> 1.7.3.1
> 

-- 
Dmitry

Re: [PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[Terry Lambert]

Sure.  First patch to linux in 16 years and all, I didn't want to be
too noisy...

Problem:

If you use EVIOCGRAB, such as when you are using a Cr-48 with the
Synaptics trackpad driver, the grab on the input device has to be
dropped before you zero out the grab on the evdev, or you can't grab
it again without rebooting.  The X Server does this; it's usually not
an issue for Chromium OS because it tends to just reboot, but everyone
else gets a locked up pointer.

I found this when copying the evdev_grab/input_device_grab() &
input_release_device()/evdev_ungrab() design pattern into some new
code to use for automated testing (record & playback of arbitrary
event streams) and for training software for gesture recognition.
When doing this sort of thing, you tend to repeat operations to obtain
large training sets, or string together a series of test gestures with
scripts, and the problem became obvious.

This is pretty easy to reproduce with a modified evtest program that
does an EVIOCGRAB, if you don't have a Synaptics touchpad to use for
testing (but you won't get whatever you grab back unless you use the
same fd or reboot).

I have some patches to a number of bugs in the Synaptics driver, and
some additional code cleanup vs. checkpatch.pl for
drivers/input/evdev.c too, but I thought I'd start small.

-- Terry

On Thu, Feb 10, 2011 at 3:54 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Feb 10, 2011 at 03:42:50PM -0800, tlambert@chromium.org wrote:
> > From: Terry Lambert <tlambert@chromium.org>
> >
> > Fixed order of calls in evdev_ungrab to allow iterative use of
> > code which grabs and releases input event devices.
> >
>
> Sorry, could you  please be more verbose?
>
> > Signed-off-by: Terry Lambert <tlambert@chromium.org>
> > ---
> >  drivers/input/evdev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > index c8471a2..0bac8da 100644
> > --- a/drivers/input/evdev.c
> > +++ b/drivers/input/evdev.c
> > @@ -160,9 +160,9 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
> >       if (evdev->grab != client)
> >               return  -EINVAL;
> >
> > +     input_release_device(&evdev->handle);
> >       rcu_assign_pointer(evdev->grab, NULL);
> >       synchronize_rcu();
> > -     input_release_device(&evdev->handle);
> >
> >       return 0;
> >  }
> > --
> > 1.7.3.1
> >
>
> --
> Dmitry

Re: [PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[Dmitry Torokhov]

On Thu, Feb 10, 2011 at 04:41:52PM -0800, Terry Lambert wrote:
> Sure.  First patch to linux in 16 years and all, I didn't want to be
> too noisy...
> 
> Problem:
> 
> If you use EVIOCGRAB, such as when you are using a Cr-48 with the
> Synaptics trackpad driver, the grab on the input device has to be
> dropped before you zero out the grab on the evdev, or you can't grab
> it again without rebooting.  The X Server does this; it's usually not
> an issue for Chromium OS because it tends to just reboot, but everyone
> else gets a locked up pointer.

Older versions of X evdev and X synaptics drivers used to grab event
device and release the grab when switching to another VT, then reqacuire
the grab when switching back to X's VT. This works (-ed?) just fine with
the current code.

Could you please explain how exactly current order of operations
prevents subsequent grabs and why changing it resolves the issue?

FWIW the current pattern is old and true "release resources in the order
opposite to which they were acquired".

> This is pretty easy to reproduce with a modified evtest program that
> does an EVIOCGRAB, if you don't have a Synaptics touchpad to use for
> testing (but you won't get whatever you grab back unless you use the
> same fd or reboot).

Could you probably share this modified version?

> 
> I have some patches to a number of bugs in the Synaptics driver, and
> some additional code cleanup vs. checkpatch.pl for
> drivers/input/evdev.c too, but I thought I'd start small.
> 

Send them on, please.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[Henrik Rydberg]

Hi Terry,

> Fixed order of calls in evdev_ungrab to allow iterative use of
> code which grabs and releases input event devices.
> 
> Signed-off-by: Terry Lambert <tlambert@chromium.org>
> ---
>  drivers/input/evdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c8471a2..0bac8da 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -160,9 +160,9 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
>  	if (evdev->grab != client)
>  		return  -EINVAL;
>  
> +	input_release_device(&evdev->handle);
>  	rcu_assign_pointer(evdev->grab, NULL);
>  	synchronize_rcu();
> -	input_release_device(&evdev->handle);

I imagine the current code could lead to a race situation if there
were no other locks involved. However, evdev_ungrab() is always called
under evdev->mutex. As Dmitry hinted, grabbing "usually works", so
perhaps you could device a tiny program which reproduces the problem?

Thanks,
Henrik

Re: [PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[Terry Lambert]

Hi Henrik,

I'm in the middle of cutting down my failure case right now, so I
should be able to provide a stand-alone test case shortly.  This is my
top priority at work right now.

And you are correct about the race.  This is about back-to-back
operations of a tool in a script, on a relatively slow machine with
obstinate hardware, and a grabby Xorg driver (it's the closed source
one, so I can't make it non-grabby).  So technically a stress test.

You noted the evdev->mutex; the idempotence of the call for the two
pointer clears will be preserved by the mutex, so no one is going to
get in under it while it's grabbed in the evdev_* layer, and not
grabbed in the input_*_device layer, and get unhappy.

So at worst it's a no-op change that scratches my particularly weird itch here.

PS: I supposed this use of the mutex is worthy of a comment in the
code?  Any guidance?

-- Terry

On Fri, Feb 11, 2011 at 2:04 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Terry,
>
>> Fixed order of calls in evdev_ungrab to allow iterative use of
>> code which grabs and releases input event devices.
>>
>> Signed-off-by: Terry Lambert <tlambert@chromium.org>
>> ---
>>  drivers/input/evdev.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index c8471a2..0bac8da 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -160,9 +160,9 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
>>       if (evdev->grab != client)
>>               return  -EINVAL;
>>
>> +     input_release_device(&evdev->handle);
>>       rcu_assign_pointer(evdev->grab, NULL);
>>       synchronize_rcu();
>> -     input_release_device(&evdev->handle);
>
> I imagine the current code could lead to a race situation if there
> were no other locks involved. However, evdev_ungrab() is always called
> under evdev->mutex. As Dmitry hinted, grabbing "usually works", so
> perhaps you could device a tiny program which reproduces the problem?
>
> Thanks,
> Henrik
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: fixed EVIOCGRAB iterative grab/release.

[Dmitry Torokhov]

Hi Terry,

On Fri, Feb 11, 2011 at 09:55:53AM -0800, Terry Lambert wrote:
> Hi Henrik,
> 

First of all, please do not top-post.

> I'm in the middle of cutting down my failure case right now, so I
> should be able to provide a stand-alone test case shortly.  This is my
> top priority at work right now.
> 
> And you are correct about the race.  This is about back-to-back
> operations of a tool in a script, on a relatively slow machine with
> obstinate hardware, and a grabby Xorg driver (it's the closed source
> one, so I can't make it non-grabby).  So technically a stress test.
> 
> You noted the evdev->mutex; the idempotence of the call for the two
> pointer clears will be preserved by the mutex, so no one is going to
> get in under it while it's grabbed in the evdev_* layer, and not
> grabbed in the input_*_device layer, and get unhappy.
> 
> So at worst it's a no-op change that scratches my particularly weird itch here.

No, it is either a real problem that can happen with any hardware and
userspace or it is not, and it does not matter if userspace portion is
open or close source, it should work the same.

Before I can apply the patch I need to understand exactly:

1. How the problem is triggered (on the level "CPU1 enters particular
section of the code and then event A happens that causes CPU2 to do
something and then we are in trouble")

2. How the proposed change helps to avoid the sequence of events in 1.

> 
> PS: I supposed this use of the mutex is worthy of a comment in the
> code?  Any guidance?

If it helps people to understand the code - by all means. However you
need to realize that almost every operation within the kernel is
protected by some lock or mutex...

Thanks.

-- 
Dmitry