svn commit: r299590 - head/sys/dev/bxe

[John Baldwin]

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