Re: Patch for oops in a grabbed evdev after disconnect

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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