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

[Bill Paul]

> In message: <20050916202207.GA22151 at ip.net.ua>
>             Ruslan Ermilov <ru at FreeBSD.org> writes:
> : Hmm, I'm not very fluent in device(9) API, but I wonder what's then
> : the analog of device_delete_child(sc->miibus) that the majority of
> : foo_detach() methods do.  I.e., will the miibus device really be
> : removed?
> 
> Yes.  I believe so.
> 
> : > How again can this happen?
> : > 
> : tcpdump -n -i ed0 &
> : kldunload if_ed
> : 
> : Still, ed_init_locked() will instantiate many things inappropriate
> : for ed_detach() context.
> 
> How does ed_init_locked get called in ed_detach()?  It looks to me
> that ed_stop is called, but ed_init() isn't.  Where would it be
> called?

BPF calls it.

If someone's got a BPF listener on an interface and you detach the
interface, BPF goes "oh shit, this interface is going away and
I've still got my grimy paws on it -- I better clean up" and one of the
things it does in the process is turn off promiscuous mode. To do that,
it calls (*ifp->if_ioctl)(SIOCSIFFLAGS) to turn off IFF_PROMISC.

Now, _some_ drivers will shortcut the handling of SIOCSIFFLAGS by
simply calling their (*ifp->if_init)() function, since they know this
routine configures the RX filter settings anyway. At this point,
even though you're in the middle of detaching the interface,
IFF_UP is still set, so (*ifp->if_init)() goes through the motions
of initializing the chip and all the other driver machinery, which
brings the interface back up again even though foo_detach() just
stopped it.

The main way you get in trouble with this is if your foo_init()
routine launches any timers or taskqueue processing. These may
be triggered after foo_detach() returns, by which time either
the resources allocated for the driver have been deleted or the
text pages have been invalidated, or both. You could also get
in trouble if you did:

foo_detach()
{
	foo_stop(sc);
	free(sc->something_really_important, M_WHATEVER);
	ether_ifdetach(ifp);
}

If foo_init() depends on something_really_important, then you're
hosed if foo_init() runs again.

My solution in re_detach() was basically just convenient. I had no
idea a bikeshed would errupt over the fact that I was clearing IFF_UP.
I know IFF_UP is supposed to be an administratively changed
flag, but unloading the driver _is_ an administrative action.
And for crying out loud, the interface is about to be destroyed: what
difference does it make?

If you insist on sticking to the "no twiddling IFF_UP from inside
the driver" rule, then I think the problem can be avoided by simply not
being lazy in foo_ioctl() and actually having explicit code in SIOCSIFFLAGS
case that turns promisc mode on and off on an IFF_PROMISC transition,
and is careful not to do it unless IFF_RUNNING is set. It also
needs to handle IFF_UP separately from IFF_PROMISC. I think the
correct logic would look something like:

	if (IFF_UP was toggled up)
		foo_init(sc);
	else if (IFF_UP was toggled down)
		foo_stop(sc);

	if (IFF_PROMISC was toggled up && ifp->if_flags & IFF_RUNNING)
		foo_set_promisc(sc);
	else if (IFF_PROMISC was toggled down && ifp->if_flags & IFF_RUNNING)
		foo_clear_promisc(sc);

Note that the ether_ifdetach->bpf->foo_ioctl interaction can occur
regardless of the bus the NIC is attached to. It doesn't matter if
it's PCI, pccard or usb, or whether or not it uses mii. The bus
attachments can influence the thing ends up crashing though.

-Bill

--
=============================================================================
-Bill Paul            (510) 749-2329 | Senior Engineer, Master of Unix-Fu
                 wpaul at windriver.com | Wind River Systems
=============================================================================
         "Ignorance may be bliss, but delusion is ecstasy!" -Perki
=============================================================================