cvs commit: src/sys/dev/re if_re.c

[Ruslan Ermilov]

On Fri, Sep 16, 2005 at 09:01:40AM -0600, M. Warner Losh wrote:
> In message: <20050916091928.GG88456 at ip.net.ua>
>             Ruslan Ermilov <ru at FreeBSD.org> writes:
> : On Thu, Sep 15, 2005 at 11:56:39PM +0300, Ruslan Ermilov wrote:
> : > The first is the BPF detach bad interaction with foo_detach(),
> : > as described in re_detach().  This panic is real with (I think)
> : > all drivers.  And testing IFF_DRV_RUNNING here doesn't seem to
> : > be able to prevent the panic.  Perhaps the fix would be to
> : > move ether_ifdetach() before foo_stop() in foo_detach(), I'm
> : > not yet sure.
> : > 
> : I tried with rl(4) PCCARD, by moving ether_ifdetach() before
> : rl_stop() in rl_detach().  It fixes the panic when you eject
> : the card, but doesn't fix it when kldunloading the module.
> : The difference is that rl_detach() is called already after
> : miibus0 and rlphy0 has been detached when kldunloading the
> : module.  When ejecting the card, rl_detach() is called first.
> : What happens when you kldunload the module with BPF listener
> : attached, is that bpfdetach() calls rl_ioctl() to reset
> : promisc, that calls rl_init_locked(), and that results in
> : 
> : 	mii = device_get_softc(sc->rl_miibus);
> : 
> : being NULL (remember the miibus has already been detached),
> : and that panics later here:
> : 
> : 	mii_mediachg(mii);
> : 
> : When we reset IFF_UP, rl_ioctl(SIOCSIFFLAGS) silently exits
> : and no harm is done.  So the question is: how do we prevent
> : this from happening without resetting IFF_UP.  One possible
> : solution would be to add sc->detaching, similar to
> : sc->suspended, abd check it in rl_ioctl().
> 
> Ugg.  In ed, we check to make sure that we still have a child before
> doing things with mii bus.  A similar fix could be made.
> 
No, ed(4) has the same problem:

        if (sc->miibus != NULL) {
                struct mii_data *mii;
                mii = device_get_softc(sc->miibus);
                mii_mediachg(mii);
        }

The device (sc->miibus) will still be there but already detached,
and its softc will already be freed, so "mii" will be NULL, and
mii_mediachg(NULL) will panic the system.

xl(4), for example, also checks for "mii" to be not NULL:

        /* XXX Downcall to miibus. */
        if (mii != NULL)
                mii_mediachg(mii);

Somehow I think it's plain incorrect to implicitly call start()
after explicitly calling stop() from detach() -- except for
possibly panicing the system, it also re-enables interrupts,
re-schedules just cancelled callouts, etc.


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-all/attachments/20050916/b06ff059/attachment.bin