[PATCH] Ethernet cleanup; 802.1p input and M_PROMISC

[Bruce M Simpson]

Hi,

Thanks for your reply.

Yar Tikhiy wrote:
> 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.
>   
I agree with the point you make here about non-conforming drivers; 
however there are cogent performance arguments for checking IFF_UP 
immediately. If an interface is configured administratively down, it 
shouldn't be pumping traffic into the network stack.  I do however 
realize there are situations where this can happen.

Suppose, for example, the thread which calls ether_input() is scheduled 
on another CPU. Dropping such frames immediately on entry into 
ether_input() saves tying up a thread for any longer than is absolutely 
necessary.

Perhaps Kip, who is working on 10GbE performance just now, can advise 
further.
>
> 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.
>   
Thanks for explaining this further. Perhaps I should put the check for 
IFF_DRV_RUNNING under INVARIANTS or make it a KASSERT?

The code in bms_netdev as it stands bends the rules a little. The IFF_UP 
check was in ether_demux() before. The original reason for the 
ether_input()/ether_demux() split was to accomodate Netgraph. I must 
admit that I hadn't fully mapped out the possible re-entry scenarios 
with Netgraph because they may be arbitrarily complicated by its very 
nature.

Whilst Netgraph is a cool feature, and one I am very grateful that 
FreeBSD has, I wonder if it is OK that we should have checks which  
potentially pessimize performance for the main use cases to protect the 
stack against Netgraph frames which are bogons, or bugs in Netgraph nodes.

I'm open to hearing more about this, but my own resources (time, money) 
are a limiting factor as to what I can do.

Regards,
BMS