[PATCH] Ethernet cleanup; 802.1p input and M_PROMISC

Yar Tikhiy yar at comp.chem.msu.su
Mon Mar 5 15:48:28 UTC 2007


On Mon, Mar 05, 2007 at 02:41:59PM +0000, Bruce M Simpson wrote:
> 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.

Agreed!

> >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?

KASSERT is dangerous there, but INVARIANTS is reasonable.  I can
see a check under DIAGNOSTIC at the beginning of ether_input()
already, that for ifp and recvif being the same.  INVARIANTS is
more appropriate there, so the existing check and the check for
IFF_DRV_RUNNING can be but in the same "#ifdef INVARIANTS" block.
But I'm not urging you to do that, I can do it by myself later,
so that it won't interfere with your current work.

> 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'd rather not limit our view to Netgraph.  In today's hairy network
setups the need for synthetic traffic (i.e., that not coming directly
from a physical device) pops up here and there.

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

Come on, Bruce, I'm not trying to put additional burden on you.  I
myself like to hack our network stack when I have good ideas and a
bit of time.  What we actually trying to do here is to define some
basic properties of modern network interfaces as software entities.
Nowadays our system is large and complex, and we seem to have a bit
too much "just in case" checks in some places and not enough important
checks in other places.

My proposed check for IFF_DRV_RUNNING is by no means a priority
task.  I can add it by myself after you finish your great current
project regarding ether_input() and friends.

I'm going to have a closer look at bms_netdev in the nearest time.

-- 
Yar


More information about the freebsd-net mailing list