Patch for oops in a grabbed evdev after disconnect

Patch for oops in a grabbed evdev after disconnect

[Pete Zaitcev]

If a device was grabbed through evdev and then became disconnected,
we oops on close. This happens because input_release_device uses memory
which was freed.

Fedora enabled evdev in X11 by default recently, and now anyone who
flips a KVM oopses when they log out (Fedora bug 436659).

Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 0727b0a..c0874a3 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -155,7 +155,8 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
 
 	rcu_assign_pointer(evdev->grab, NULL);
 	synchronize_rcu();
-	input_release_device(&evdev->handle);
+	if (evdev->exist)
+		input_release_device(&evdev->handle);
 
 	return 0;
 }

Re: Patch for oops in a grabbed evdev after disconnect

[Dmitry Torokhov]

Hi Pete,

On Mon, Mar 17, 2008 at 11:48:07PM -0700, Pete Zaitcev wrote:
> If a device was grabbed through evdev and then became disconnected,
> we oops on close. This happens because input_release_device uses memory
> which was freed.
> 
> Fedora enabled evdev in X11 by default recently, and now anyone who
> flips a KVM oopses when they log out (Fedora bug 436659).
> 

Could you tell me what memory is freed? As far as I understand the
the input_dev structure shold be pinned in memory by the driver
core since we have this link:

	evdev->dev.parent = &input_dev->dev;

This should guarantee that input_device is not gone until we
call evdev_free which should be done way after the ungrab.

What am I missing here?

> Signed-off-by: Pete Zaitcev <zaitcev@redhat.com>
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 0727b0a..c0874a3 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -155,7 +155,8 @@ static int evdev_ungrab(struct evdev *evdev, struct evdev_client *client)
>  
>  	rcu_assign_pointer(evdev->grab, NULL);
>  	synchronize_rcu();
> -	input_release_device(&evdev->handle);
> +	if (evdev->exist)
> +		input_release_device(&evdev->handle);
>  
>  	return 0;
>  }

-- 
Dmitry

Re: Patch for oops in a grabbed evdev after disconnect

[Pete Zaitcev]

On Tue, 18 Mar 2008 09:31:25 -0400, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Mon, Mar 17, 2008 at 11:48:07PM -0700, Pete Zaitcev wrote:

> > If a device was grabbed through evdev and then became disconnected,
> > we oops on close. This happens because input_release_device uses memory
> > which was freed.

> Could you tell me what memory is freed?

The input_dev is freed. We only have two structures involved,
really: evdev and input_dev. The rest is either immaterial like
evdev_client, or embedded into the two like input_handle and device.

It's freed by this route:
 hidinput_disconnect
   input_unregister_device
     evdev_disconnect  --- marks evdev->exists = 0
     device_unregister
     put_device -- kobject_put -- kref_put -- kobject_cleanup
       input_dev_release

> [] As far as I understand the
> the input_dev structure shold be pinned in memory by the driver
> core since we have this link:
> 
> 	evdev->dev.parent = &input_dev->dev;
> 
> This should guarantee that input_device is not gone until we
> call evdev_free which should be done way after the ungrab.

I don't think anyone checks this, unless the accompaining refcount
is set.

Honestly, I didn't consider the implications for the integrity
of sysfs. It seems like there's no extra memory leak with my
patch, that's the only thing I can promise. Maybe you want to
poke Greg about it.

It may be easier for you to reproduce it than to parse my arguments,
it's trivial. Use the evdev-x which I attached to the bug, or write
an application which:
 - opens evdev
 - grabs <------- this is important
 - sleeps for 10 seconds
 - exits
Run it, pull the plug while it sleeps. The system will oops.
If you enable lock debugging, it will traceback first like in
the bug, because slab poisoning destroys spinlock magic.

-- Pete

Re: Patch for oops in a grabbed evdev after disconnect

[Dmitry Torokhov]

On Tue, Mar 18, 2008 at 11:03:42AM -0700, Pete Zaitcev wrote:
> On Tue, 18 Mar 2008 09:31:25 -0400, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Mar 17, 2008 at 11:48:07PM -0700, Pete Zaitcev wrote:
> 
> > > If a device was grabbed through evdev and then became disconnected,
> > > we oops on close. This happens because input_release_device uses memory
> > > which was freed.
> 
> > Could you tell me what memory is freed?
> 
> The input_dev is freed. We only have two structures involved,
> really: evdev and input_dev. The rest is either immaterial like
> evdev_client, or embedded into the two like input_handle and device.
> 
> It's freed by this route:
>  hidinput_disconnect
>    input_unregister_device
>      evdev_disconnect  --- marks evdev->exists = 0
>      device_unregister
>      put_device -- kobject_put -- kref_put -- kobject_cleanup
>        input_dev_release
> 
> > [] As far as I understand the
> > the input_dev structure shold be pinned in memory by the driver
> > core since we have this link:
> > 
> > 	evdev->dev.parent = &input_dev->dev;
> > 
> > This should guarantee that input_device is not gone until we
> > call evdev_free which should be done way after the ungrab.
> 
> I don't think anyone checks this, unless the accompaining refcount
> is set.
> 
> Honestly, I didn't consider the implications for the integrity
> of sysfs. It seems like there's no extra memory leak with my
> patch, that's the only thing I can promise. Maybe you want to
> poke Greg about it.
> 

I dont oppose your patch, I am just trying to understand why it
is needed because driver core should pin the parent device as
far as I understand and if this does not happen there are other
issues in input core that need to be taken care of.

>From what I see we should be automatically taking the reference
to parent kobject in

kobject_add_internal():
	parent = kobject_get(kobj->parent);

perent is set in kobject_add_varg():
	kobj->parent = parent;

.. which is called from kobject_add() which is called from
device_add().

Hmm, I wonder if obsolete sysfs links mess up proper parenting
data... I dont think I have obsolete links set up on any of my
boxes, can you see if oops goes away if you disable deprectated
sysfs? If so then instead of checking exist flag we need explicitely
take reference to the parent input_dev. Although I wonder what
other subsystems might be affected.

-- 
Dmitry

Re: Patch for oops in a grabbed evdev after disconnect

[Pete Zaitcev]

On Tue, 18 Mar 2008 14:54:17 -0400, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

I'm sorry, but I'm weak. I'll have to poke Greg (to cc:), although
I know he's pretty busy, but I don't grok sysfs and kobjects. So:

> > > > If a device was grabbed through evdev and then became disconnected,
> > > > we oops on close. This happens because input_release_device uses memory
> > > > which was freed.
> > 
> > > Could you tell me what memory is freed?
> > 
> > The input_dev is freed. [...]

> > > [] As far as I understand the
> > > the input_dev structure shold be pinned in memory by the driver
> > > core since we have this link:
> > > 
> > > 	evdev->dev.parent = &input_dev->dev;
> > > 
> > > This should guarantee that input_device is not gone until we
> > > call evdev_free which should be done way after the ungrab.
> > 
> > I don't think anyone checks this, unless the accompaining refcount
> > is set.

> I dont oppose your patch, I am just trying to understand why it
> is needed because driver core should pin the parent device as
> far as I understand and if this does not happen there are other
> issues in input core that need to be taken care of.
> 
> From what I see we should be automatically taking the reference
> to parent kobject in
> 
> kobject_add_internal():
> 	parent = kobject_get(kobj->parent);
> 
> perent is set in kobject_add_varg():
> 	kobj->parent = parent;
> 
> .. which is called from kobject_add() which is called from
> device_add().

Hmm, I see it now. OK.

> Hmm, I wonder if obsolete sysfs links mess up proper parenting
> data... I dont think I have obsolete links set up on any of my
> boxes, can you see if oops goes away if you disable deprectated
> sysfs? If so then instead of checking exist flag we need explicitely
> take reference to the parent input_dev. []

I'll check this.

-- Pete

Re: Patch for oops in a grabbed evdev after disconnect

[Pete Zaitcev]

On Tue, 18 Mar 2008 14:54:17 -0400, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> > > [] As far as I understand the
> > > the input_dev structure shold be pinned in memory by the driver
> > > core since we have this link:
> > > 
> > > 	evdev->dev.parent = &input_dev->dev;

> > I don't think anyone checks this, unless the accompaining refcount
> > is set.

> From what I see we should be automatically taking the reference
> to parent kobject in
> 
> kobject_add_internal():
> 	parent = kobject_get(kobj->parent);
> 
> perent is set in kobject_add_varg():
> 	kobj->parent = parent;

I see you fixed this in a7097ff89c3204737a07eecbc83f9ae6002cc534.
Adam Jackson reports on the X11 side that it worked.

-- Pete