[PATCH] Ethernet cleanup; 802.1p input and M_PROMISC

[Julian Elischer]

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


When we added netgraph we split both the input and output parts
so that they would provide 'natural' entrypoints for a bridge.
Consider where a bridge wants to put packets.

Since the split however other code has made use of those entrypoints at different
times. I'm not sure at the moment whether other code does so now.

> 
> 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
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"