Re: weird use-after-free bug in module_put

[Dmitry Torokhov]

On Fri, Oct 19, 2012 at 06:50:46PM +0100, Al Viro wrote:
> On Fri, Oct 19, 2012 at 10:36:39AM -0700, Dmitry Torokhov wrote:
> > On Fri, Oct 19, 2012 at 06:09:51PM +0100, Al Viro wrote:
> > > On Fri, Oct 19, 2012 at 09:33:18AM -0700, Dmitry Torokhov wrote:
> > > 
> > > > We are now removing instance of character device corresponding to input
> > > > device when input device disappears.
> > > > 
> > > > Ah, I know... cdev is embedded in evdev, but lives longer.. I do want to
> > > > keep cdev embedded as it allows me to easily get to evdev in
> > > > evdev_open(), but I need to be able to add and then drop reference to
> > > > evdev from cdev's ->release() method. This means I need to override it.
> > > > 
> > > > Or I could have cdev separately allocated, but then I'd like to have a
> > > > void pointer in "struct cdev" so I could get from it back to
> > > > corresponding evdev.
> > > 
> > > Your real problem is that you have two kobjects embedded into the same
> > > thing.  It can work, but you need to make the secondary (one that does
> > > *not* free in its ->release()) pin the primary.  Sigh...  Device model
> > > sucks, film at 11...
> > 
> > Right, but "cdev" is currently "sealed": it does not allow specifying a
> > custom release function from which I could unpin primary (evdev). You
> > are the author/owner of cdev code, so that is why I was asking for
> > your opinion as to what is the best way to proceed:
> > 
> > 1. Allocate cdev separately and add void * to struct cdev so that it is
> >    easy to get to corresponding structure on evdev_open.
> > 
> > 2. Keep cdev embedded in evdev but export cdev's cleanup method and
> >    have evdev override ->release with its own version that calls
> >    cdev_default_release() and then unpins evdev stucture.
> > 
> > 3. Add struct device *parent to struct cdev and have it pin and unpin it
> >    for us (if it is set up).
> 
> The last one would be my preference, TBH, but I'm not sure how to do it
> cleanly.  The thing is, kobject_del() would have to be done at some point.
> _Before_ we have dropped the last references.  And once we have, we are
> back to the original race, AFAICS.

If we unpin the parent as the very last thing in cdev_default_release()
and do not touch any memory [formerly] occupied by cdev afterwards I
think it should be OK.

> 
> What's pinning that cdev reference, BTW?

Userspace threads that have that character device opened. They are
dropping off and releasing evdev's references, but cdev is unpinned in
vfs core, which is too late as it happens after evdev_release()
competed.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/