Date: Fri, 13 May 2016 11:38:05 -0700 From: John Baldwin <jhb@freebsd.org> To: Scott Long <scottl@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r299590 - head/sys/dev/bxe Message-ID: <4407587.3MdJrGvffs@ralph.baldwin.cx> In-Reply-To: <201605130557.u4D5vMBZ015636@repo.freebsd.org> References: <201605130557.u4D5vMBZ015636@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, May 13, 2016 05:57:22 AM Scott Long wrote: > Author: scottl > Date: Fri May 13 05:57:21 2016 > New Revision: 299590 > URL: https://svnweb.freebsd.org/changeset/base/299590 > > Log: > Don't jam the softc in the device_probe routine. The softc isn't owned by > the driver here, so it shouldn't be accessed, let alone written to. Remove > the nearby debug line, it's the only thing that depended on the softc, and > it depended on it in a way that couldn't work in this part of the code. > > This fixes some reports of use-after-free and system instability with > DEBUG_MEMGUARD enabled. Actually, softcs are perfectly valid for use in probe routines. They are just destroyed on exit. The softc is allocated by device_set_driver() (and the previous softc is freed when device_set_driver() is called). You can see the calls to 'device_set_driver' (which also set the device name so that device_printf() works in probe routines) in device_probe_child() (the thing that calls DEVICE_PROBE()): int device_probe_child(device_t dev, device_t child) { ... for (; dc; dc = dc->parent) { for (dl = first_matching_driver(dc, child); dl; dl = next_matching_driver(dc, child, dl)) { ... PDEBUG(("Trying %s", DRIVERNAME(dl->driver))); result = device_set_driver(child, dl->driver); ... result = DEVICE_PROBE(child); ... if (result > 0) { (void)device_set_driver(child, NULL); continue; } ... } ... } ... if (best) { ... result = device_set_driver(child, best->driver); } ... } In the main loop, the 'device_set_driver()' before DEVICE_PROBE() frees the softc from the previous probe attempt. If at least one working driver is found, then best will be non-null and the last 'device_set_driver()' will free the softc from the last probe attempt and allocate a fresh one to be used when DEVICE_ATTACH() is invoked. If no driver was valid, then we would have called the 'device_set_driver(child, NULL)' case after every probe. Looking at the code, it would probably be clearer to just move that case into an 'else' clause at the end for the 'best == NULL' case. However, just touching the softc in a probe routine cannot possibly cause a memory leak. device_get_softc() does not allocate the softc on demand, it just returns whatever is already allocated. While using the softc in probe routines is often dubious, the commit is wrong in that this cannot cause memory leaks or use after free. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4407587.3MdJrGvffs>