[PATCH] Ethernet cleanup; 802.1p input and M_PROMISC
andre at freebsd.org
Mon Mar 5 14:35:13 UTC 2007
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.
More information about the freebsd-net