Date: Mon, 5 Mar 2007 18:29:14 +0300 From: Yar Tikhiy <yar@comp.chem.msu.su> To: Andre Oppermann <andre@freebsd.org> Cc: freebsd-net@freebsd.org, Bruce M Simpson <bms@incunabulum.net> Subject: Re: [PATCH] Ethernet cleanup; 802.1p input and M_PROMISC Message-ID: <20070305152914.GD57253@comp.chem.msu.su> In-Reply-To: <45EC2AA8.8060302@freebsd.org> References: <45E8B964.2090200@incunabulum.net> <20070303215359.GB40430@comp.chem.msu.su> <45EA0756.2000107@incunabulum.net> <20070304070458.GG40430@comp.chem.msu.su> <45EB750A.90105@incunabulum.net> <20070305142411.GC57253@comp.chem.msu.su> <45EC2AA8.8060302@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 05, 2007 at 03:35:20PM +0100, Andre Oppermann wrote: > Yar Tikhiy wrote: > >On Mon, Mar 05, 2007 at 01:40:26AM +0000, Bruce M Simpson wrote: > >>Yar Tikhiy wrote: > >>>Now I see your point, thanks! Well, at least in theory, the driver > >>>shouldn't call ether_input() if the interface isn't running. OTOH, > >>>the interface shouldn't be getting traffic if it's !UP. However, > >>>I suspect that not all drivers handle IFF_UP fully or even can do > >>>it at all due to hardware limitations. As I understand it, in an > >>>ideal world a !UP interface should be deaf and dumb and not interfering > >>>in any way with the network still connected to it physically. > >>>Therefore discarding inbound traffic from a !UP interface may be a > >>>necessary workaround, but it may not be enough. All that boils > >>>down to this: The IFF_UP check in ether_input() is more to a sanity > >>>check than to the way for IFF_UP to work. Therefore we can add the > >>>IFF_DRV_RUNNING sanity check there, too, for completeness. > >>> > >>Thanks for your explanation. > >> > >>I'm still not sure I understand why IFF_DRV_RUNNING should be checked > >>for in ether_input(). > >> > >>There is a pretty clear reason for checking for IFF_UP in ether_input(); > >>an interface which is configured administratively down should not be > >>bringing traffic into the stack, regardless of whether it is a hardware > >>device or a pseudo-device. IFF_UP has been in since 4.2BSD; it is more > >>or less integral to how the BSD network stack operates. There are > >>situations in which a pseudo-device or hardware device could incorrectly > >>call ether_input() with such traffic. > >> > >>Reading <net/if.h>, IFF_DRV_RUNNING is documented as meaning 'resources > >>are allocated for this device'. Surely such a check is redundant and not > >>relevant to the operation of ether_input()? As far as I can tell it is > >>similar to the old meaning of IFF_RUNNING, and there are legitimate > >>situations in which the hardware or its queues may have stopped > >>processing temporarily whilst the interface may be administratively up > >>(and thus accepting traffic). > >> > >>Please correct me if I'm wrong or point out situations where it's > >>important IFF_DRV_RUNNING state is checked outside of a driver. Sorry if > >>I seem obtuse, but I'm sure I'm missing some detail here. > > > >My concern is that, with possible callers of ether_input() being > >not really *from* but *on behalf* of the interface, e.g., in Netgraph, > >IFF_DRV_RUNNING can be a way for the interface driver to tell us: > >I'm not ready yet, so don't believe anyone who pretends he has a > >packet from me. > > > >E.g., a vlan(4) interface gets IFF_DRV_RUNNING set only if it is > >properly attached to an Ethernet interface (known as the vlan's > >parent). AFAIK this is a totally legitimate use of IFF_DRV_RUNNING. > >Now assume that a vlan interface is UP but not RUNNING because it's > >detached from the parent. If a buggy Netgraph node or another > >source of synthetic traffic decides to inject a packet as though > >it comes in from the said vlan interface, handling the packet as > >usual will be bogus. > > > >IMHO the IFF_UP check in ether_input() is mostly for a similar > >purpose: If all callers of ether_input() were in real and conformant > >interface drivers, we shouldn't bother re-checking IFF_UP in > >ether_input() either because the driver of a down interface wouldn't > >call ether_input() for it in the first place. > > > >So I view the checks in ether_input() as a way to work around broken > >drivers and simplify synthetic callers of ether_input(). In fact, > >the whole first part of ether_input() is for that: It essentially > >verifies the caller's conformance. I mean the checks for the proper > >mbuf flags and length, recvif, etc. > > > >Of course, we can omit the check for IFF_DRV_RUNNING if we think > >that synthetic traffic from an unready interface is OK. But I'm > >afraid we shouldn't. > > > >In addition, I wonder if we can move the conformance checks to a > >wrapper function so that conformant drivers don't have to pay the > >performance penalty of the "just in case" checks per each inbound > >Ethernet packet. > > Then make it a KASSERT() if it only catches buggy drivers. Hmmm, KASSERT() on a network packet handling path is dangerous: if any driver proves buggy later, evil people can get a way to DoS CURRENT systems some brave folks run in production. -- Yar
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070305152914.GD57253>