Kostik Belousov wrote:
On Fri, Feb 05, 2010 at 08:51:25AM -0500, Andrew Gallatin wrote:
Kostik Belousov wrote:
On Thu, Feb 04, 2010 at 03:40:28PM -0500, Andrew Gallatin wrote:
I've got a commercial driver that uses device cloning.
At unload time, the driver calls clone_cleanup(). When I unload
the driver when the kernel is built with INVARIANTS, I'll see a
panic in devfs_populate_loop(). This happens in 6-stable,
as well as 8-stable.
>From what I can see the clone has been freed, but it
remains on the devfs cdevp_list. Then the next time
devfs_populate_loop() is called, it trips over the bad
entry (cdp->cdp_dirents points to 0xdeadc0dedeadc0de)
See appended kgdb session.
If I trace the code path, it looks like clone_cleanup()
calls destroy_devl(). And destroy_devl() will eventually
call devfs_free() if the si_refcnt is zero. But I don't
see anything which will get the cdev removed from
the cdevp_list prior to it being freed.
The only code I see which will get the cdev removed from
the cdevp_list() seems to be the "GC any lingering devices"
block in devfs_populate_loop
What am I missing?
You did not mentioned it, but my guess is that you create clones from
the dev_clone event handler. Please note that devfs_lookup() that fires
Yes, I do.
dev_clone event, consumes a device reference. Thus clone handlers shall
do dev_ref().
Due to races with cleanup, you should use MAKEDEV_REF flag for
make_dev_credv(9) KPI instead of doing make_dev()/dev_ref() pair.
I need to support FreeBSD going all the way back to 6, so that's not an
option in some versions.
But, I'm talking about device removal time. If I call clone_cleanup()
where the clones have dev->si_refcount==1, then I get the use-after-free
panic. If I hack things to elevate the reference count (such that
dev->si_refcount==2 when clone_cleanup() is called), then I don't
get the panic.
Are you saying I should have been taking the extra reference
via my dev_clone eventhandler? Won't having the extra reference
lead to a memory leak? Or am I just mis-reading the code, and
this will lead to things being freed normally?
Yes, clone handler shall do dev_ref(). Either by doing race-free
make_dev_credf(MAKEDEV_REF) call, or by using dev_ref() after make_dev().
OK, cool. The man pages are handy. When I started this
back in the FreeBSD 5 days, the man pages didn't exist :)
That said, do you really need clones at all ?
I need to support FreeBSD back to 6.x, and I need to support the
linux-like model of opening the "same" /dev/node multiple times
and getting unique handles. So I think I need clones.
Wouldn't it be cleaner to use cdevpriv for the 7/8/HEAD where it is
present ? And have special #ifdef-ed code for 6, that could be
eventually dropped.
Yes, the cdevpriv() is a much cleaner interface. I'll probably add
support for that soon.
Thanks for the help,
Drew
_______________________________________________
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
