kern/136618: [pf][stf] panic on cloning interface without unit number, e.g. `stf'

Eygene Ryabinkin rea-fbsd at
Thu Jul 9 22:30:39 UTC 2009

// Sorry for a long letter ;))

In fact, stf(4) problem will be healed with the attached patch: it works
for me and should provide absolutely sane pf rules, because stf(4) is
essentially a singleton interface, so there won't be ambiguities: 'stf'
as the interface name will have the same effect as the 'stf' being
treated as the interface group name.

In fact, the problem is rather simple: when cloned interface is created,
interface group with the interface family name (e.g. "vlan" or "carp")
is created prior to the interface creation.  So, when we create (with
pure creation or rename) the interface with the name of the "family",
pf will pick up the kif (via pfi_attach_ifnet) that already exists and
correspond to the interface group name and thus the loop will be
created: one of interface's groups will be its "family" group and this
group's ifg_pf_kif will be equal to the kif.  Recursive functions
like pfi_kif_update() won't be happy with this.

With my patch it shouldn't be a problem for any interface type --
infinite recursion will be avoided.  But another problem exists for
non-singleton interfaces: when we have, for example, interfaces carp1,
carp2 and carp3 and some of them (say, carp2) is renamed to just "carp",
then the rules for "carp" will really correspond to the whole family of
carp interfaces and not just to the former "carp2".  Of course, this is
a kind of shooting yourself to your feet, but this can be left unnoticed
and will produce a big headache; moreover, interface groups are hardly
well-documented now, so a person should learn from the experience or
from sources.

There is kern/127042 (rather old one, but it essentially the same as the
current PR) that addresses this aspect of the problem: it

 - refuses to add new group whose name coincides with the name of
   existing interface (first hunk);

 - additionally, it refuses to rename interface to the name that
   coincides with one of existing group names (second and third hunks of
   the patch in the said PR, by the way, last hunk misses '{' after 'if'
   and has 'groupname' instead of 'new_name').

The first part makes singletons to be real singletons and there will be
no groups of their name (in contrast with the current behaviour) as long
as these interfaces aren't renamed.  I mean that for the 'stf' case,
there will be no group named 'stf'.  But if I'll rename 'stf' -> 'foo',
then I'll be able to create the 'stf' group: 'ifconfig lo0 group stf'
will work happily.  This is the weird example, but it shows that the
behaviour here isn't very consistent with the notion of singletons.
Perhaps, interfaces such as 'stf' shouldn't be allowed to be renamed
at all -- this will close this issue.

Thus, this second patch (slightly reworked one from kern/127042) should
eliminate the foot-shooting problem: there will be no iface renames and
group creation that will result in the described foot-shooting.  And
since interface cloning when the "family" name is used (e.g. 'ifconfig
tun create') will create tunX with X being the first available digit, we
should be safe with this as long as all cloned interfaces (but singleton
ones) will refuse to create interface with the "family" name.

If there will be general consensus that singleton interfaces shouldn't
be allowed to be renamed, then I can try to implement such
functionality.  Going the other way and allowing creation of the 'stf'
group shouldn't also be hard -- just an additional test inside
if_addgroup() and a new interface flag that will be tested inside
if_addgroup().  And may be a special function for allocation of
singleton interface units -- just for simplicity and to avoid code

Any views on this?  Perhaps I am missing something important here?
It is already deep night here, so I can produce some bad ideas :((

Thanks for your patience!

--- pf_if.c-avoid-infinite-recursion.diff begins here ---
begin 600 pf_if.c-avoid-infinite-recursion.diff
M8F5R(&]F("HO"BT):68@*&MI9BT^<&9I:U]I9G`@(3T at 3E5,3"D*+0D)5$%)
M9F=L7VYE>'0I"BT)"0EP9FE?:VEF7W5P9&%T92 at H<W1R=6-T('!F:5]K:68@
M("IG<E]K:68["BL)"51!24Q17T9/4D5!0T at H:69G;"P@)FMI9BT^<&9I:U]I
M9E]K:68["BL)"0DO*@HK"0D)("H at 079O:60@:6YF:6YI=&4@<F5C=7)S:6]N
M.B!I9B!I;G1E<F9A8V4@;F%M92!I<PHK"0D)("H@=&AE('-A;64 at 87,@=&AE
M9B at T*2!H87,@;VYL>2!O;F4@:6YS=&%N8V4 at 86YD(&ET<R!K:68@=VEL;"!B
M="!B92!P<F]H:6)I=&5D+"!S:6YC92!I="!M86ME<R!P9 at HK"0D)("H@<G5L
M"2`J(')U;&5S('=I;&P at 87!P;'D@=&\@=&AE('=H;VQE(&=R;W5P+ at HK"0D)
--- pf_if.c-avoid-infinite-recursion.diff ends here ---

--- if.c-avoid-group-and-iface-name-clashes.diff begins here ---
begin 600 if.c-avoid-group-and-iface-name-clashes.diff
M("!G<F]U<&YA;65;<W1R;&5N*&=R;W5P;F%M92D at +2`Q72`\/2`G.2<I"B`)
M/2!.54Q,*0HK"0ER971U<FX@*$5%6$E35"D["BL*(`E)1DY%5%]73$]#2R at I
M.PH@"51!24Q17T9/4D5!0T at H:69G;"P@)FEF<"T^:69?9W)O=7!S+"!I9F=L
M<F]U<"P at 9W)O=7!N86UE*2D@>PI`0"`M,3DP-RPV("LQ.3$P+#<@0$`*(`EC
M<"`J:69G(#T at 3E5,3#L*(`H@"6EF<B`]("AS=')U8W0@:69R97$@*BED871A
M.PH@"7-W:71C:"`H8VUD*2!["D!`("TR,#$T+#<@*S(P,3 at L,34 at 0$`*(`D)
M4U0I.PHK"0D)?0HK"0E)1DY%5%]254Y,3T-+*"D["BL*(`D)+RH at 06YN;W5N
M9F%N;F]U;F-E;7-G*&EF<"P at 249!3E]$15!!4E154D4I.PH@"0E%5D5.5$A!
--- if.c-avoid-group-and-iface-name-clashes.diff ends here ---
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pf_if.c-avoid-infinite-recursion.diff
Type: text/x-diff
Size: 1156 bytes
Desc: not available
Url :

More information about the freebsd-net mailing list