cvs commit: src/sys/net if_vlan.c

[Sam Leffler]

Yar Tikhiy wrote:
> On Thu, Aug 03, 2006 at 10:11:11AM -0700, Sam Leffler wrote:
>> Yar Tikhiy wrote:
>>> yar         2006-08-03 09:59:09 UTC
>>>
>>>   FreeBSD src repository
>>>
>>>   Modified files:
>>>     sys/net              if_vlan.c 
>>>   Log:
>>>   Should vlan_input() ever be called with ifp pointing to a non-Ethernet
>>>   interface, do not just assign -1 to tag because it breaks the logic of
>>>   the code to follow.  The better way is to handle this case as an unsupported
>>>   protocol and return unless INVARIANTS is in effect and we can panic.
>>>   Panic is good there because the scenario can happen only because of a
>>>   coding error elsewhere.
>>>   
>>>   We also should show the interface name in the panic message for easier
>>>   debugging of the problem, should it ever emerge.
>> Introducing a panic in a place where you can trivially recover is bad
>> regardless of why you got there.  Many people run production systems
>> with INVARIANTS turned on.  Is it now possible to send a "packet of
>> death" by exploiting this code path?
> 
> No nastygram can ever achieve this; only FreeBSD commiters possess
> the ability to :-)
> 
> The panic can never be reached unless one manages to attach a vlan
> interface to a non-Ethernet physical interface in advance, which
> is totally prohibited by the code at the beginning of vlan_config();
> and vlan_config() is the only way to attach a vlan interface to a
> physical interface.
> 
> I.e., it will take a developer breaking the logic in /sys/net to
> make the code path expoloitable.
> 
> OTOH, you are right that we can at least attempt to recover from
> the situation.  Perhaps it's time to introduce a common macro or
> function that emits a message on the console and then just calls
> kdb_backtrace() instead of dumping core and halting the system?
> So users will be able to post the stack traces to the lists and
> thus help to spot the possible bugs w/o having to go through panics.
> I'm unsure if sticking raw kdb_backtrace() calls in such places
> is a good idea, so I'm suggesting a wrapper function or macro.
> It is to be used in "can absolutely never happen" cases that are
> not fatal, like the one under discussion.
> 

It is my experience that problems like the "packet of death" come about
from (well-meaning) planting of a landmine of this sort followed,
sometime later, by another person enabling the code path.  I work by the
rule that panic should be used only in places where you have no way to
recover or recovery is so hard as to be not worthwhile (based on the
circumstances).

	Sam