Comment 1 Mark Linimon 2009-07-09 19:26:44 UTC

Responsible Changed
From-To: freebsd-bugs->freebsd-net

Over to maintainer(s).

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

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
M+2TM('-Y<R]C;VYT<FEB+W!F+VYE="]P9E]I9BYC+F]R:6<),C`P.2TP-RTQ
M,"`P,#HQ.3HT-"XP,#`P,#`P,#`@*S`T,#`**RLK('-Y<R]C;VYT<FEB+W!F
M+VYE="]P9E]I9BYC"3(P,#DM,#<M,3`@,#$Z,#`Z,38N,#`P,#`P,#`P("LP
M-#`P"D!`("TU,S`L,3`@*S4S,"PR-B!`0`H@"0EP9FE?9'EN861D<E]U<&1A
M=&4H<"D["B`*(`DO*B!A9V%I;B!F;W(@86QL(&=R;W5P<R!K:68@:7,@;65M
M8F5R(&]F("HO"BT):68@*&MI9BT^<&9I:U]I9G`@(3T@3E5,3"D*+0D)5$%)
M3%%?1D]214%#2"AI9F=L+"`F:VEF+3YP9FEK7VEF<"T^:69?9W)O=7!S+"!I
M9F=L7VYE>'0I"BT)"0EP9FE?:VEF7W5P9&%T92@H<W1R=6-T('!F:5]K:68@
M*BD*+0D)"2`@("!I9F=L+3YI9F=L7V=R;W5P+3YI9F=?<&9?:VEF*3L**PEI
M9B`H:VEF+3YP9FEK7VEF<"`A/2!.54Q,*2!["BL)"7-T<G5C="!P9FE?:VEF
M("IG<E]K:68["BL)"51!24Q17T9/4D5!0T@H:69G;"P@)FMI9BT^<&9I:U]I
M9G`M/FEF7V=R;W5P<RP@:69G;%]N97AT*2!["BL)"0EG<E]K:68@/0HK"0D)
M("`@("AS=')U8W0@<&9I7VMI9B`J*6EF9VPM/FEF9VQ?9W)O=7`M/FEF9U]P
M9E]K:68["BL)"0DO*@HK"0D)("H@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@87,@=&AE
M(&EN=&5R9F%C92!G<F]U<"!N86UE("AF;W(@97AA;7!L92P**PD)"2`J('-T
M9B@T*2!H87,@;VYL>2!O;F4@:6YS=&%N8V4@86YD(&ET<R!K:68@=VEL;"!B
M90HK"0D)("H@;F%M960@(G-T9B(L(&%S('1H92!G<F]U<"=S(")K:68B+B`@
M06YO=&AE<B!C87-E"BL)"0D@*B!I<R!T:&4@:6YT97)F86-E(')E;F%M960@
M=&\@:70G<R!F86UI;'D@;F%M90HK"0D)("H@*&4N9RXL(")V;&%N6"(@+3X@
M(G9L86XB*2X@($EN(&9A8W0L('1H92!L871T97(**PD)"2`J(&-A<V4@;75S
M="!B92!P<F]H:6)I=&5D+"!S:6YC92!I="!M86ME<R!P9@HK"0D)("H@<G5L
M97,@=&\@8F5H879E(&YO;BUI;G1U:71I=F5L>3H@:6YT97)F86-E)W,**PD)
M"2`J(')U;&5S('=I;&P@87!P;'D@=&\@=&AE('=H;VQE(&=R;W5P+@HK"0D)
M("HO"BL)"0EI9B`H:VEF("$](&=R7VMI9BD**PD)"0EP9FE?:VEF7W5P9&%T
?92AG<E]K:68I.PHK"0E]"BL)?0H@?0H@"B!V;VED"@``
`
end
--- 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+2TM('-Y<R]N970O:68N8RYO<FEG"3(P,#DM,#<M,3`@,#$Z-3DZ,3`N,#`P
M,#`P,#`P("LP-#`P"BLK*R!S>7,O;F5T+VEF+F,),C`P.2TP-RTQ,"`P,CHP
M,#HQ,RXP,#`P,#`P,#`@*S`T,#`*0$`@+3DV-2PV("LY-C4L.2!`0`H@"2`@
M("!G<F]U<&YA;65;<W1R;&5N*&=R;W5P;F%M92D@+2`Q72`\/2`G.2<I"B`)
M"7)E='5R;B`H14E.5D%,*3L*(`HK"6EF("AI9G5N:70H9W)O=7!N86UE*2`A
M/2!.54Q,*0HK"0ER971U<FX@*$5%6$E35"D["BL*(`E)1DY%5%]73$]#2R@I
M.PH@"51!24Q17T9/4D5!0T@H:69G;"P@)FEF<"T^:69?9W)O=7!S+"!I9F=L
M7VYE>'0I"B`)"6EF("@A<W1R8VUP*&EF9VPM/FEF9VQ?9W)O=7`M/FEF9U]G
M<F]U<"P@9W)O=7!N86UE*2D@>PI`0"`M,3DP-RPV("LQ.3$P+#<@0$`*(`EC
M:&%R(&YE=U]N86UE6TE&3D%-4TE:73L*(`ES=')U8W0@:69A9&1R("II9F$[
M"B`)<W1R=6-T('-O8VMA9&1R7V1L("IS9&P["BL)<W1R=6-T(&EF9U]G<F]U
M<"`J:69G(#T@3E5,3#L*(`H@"6EF<B`]("AS=')U8W0@:69R97$@*BED871A
M.PH@"7-W:71C:"`H8VUD*2!["D!`("TR,#$T+#<@*S(P,3@L,34@0$`*(`D)
M"7)E='5R;B`H14E.5D%,*3L*(`D):68@*&EF=6YI="AN97=?;F%M92D@(3T@
M3E5,3"D*(`D)"7)E='5R;B`H145825-4*3L*+0D)"BL**PD)249.151?4DQ/
M0TLH*3L**PD)5$%)3%%?1D]214%#2"AI9F<L("967VEF9U]H96%D+"!I9F=?
M;F5X="D**PD)"6EF("@A<W1R8VUP*&EF9RT^:69G7V=R;W5P+"!N97=?;F%M
M92DI('L**PD)"0E)1DY%5%]254Y,3T-+*"D["BL)"0D)<F5T=7)N("A%15A)
M4U0I.PHK"0D)?0HK"0E)1DY%5%]254Y,3T-+*"D["BL*(`D)+RH@06YN;W5N
M8V4@=&AE(&1E<&%R='5R92!O9B!T:&4@:6YT97)F86-E+B`J+PH@"0ER=%]I
M9F%N;F]U;F-E;7-G*&EF<"P@249!3E]$15!!4E154D4I.PH@"0E%5D5.5$A!
J3D1,15)?24Y63TM%*&EF;F5T7V1E<&%R='5R95]E=F5N="P@:69P*3L*
`
end
--- if.c-avoid-group-and-iface-name-clashes.diff ends here ---
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #

2009/7/10 Eygene Ryabinkin <rea-fbsd@codelabs.ru>:
> There is kern/127042 (rather old one, but it essentially the same as the
> current PR) that addresses this aspect of the problem: it
>
> =C2=A0- additionally, it refuses to rename interface to the name that
> =C2=A0 coincides with one of existing group names (second and third hunks=
 of
> =C2=A0 the patch in the said PR, by the way, last hunk misses '{' after '=
if'
> =C2=A0 and has 'groupname' instead of 'new_name').

sorry for bad patch, my bad s/copy/coffee/

thanks for working on this.




--=20
Artis Caune

    Everything should be made as simple as possible, but not simpler.

Artis, good day.

Fri, Jul 10, 2009 at 08:53:07AM +0300, Artis Caune wrote:
> 2009/7/10 Eygene Ryabinkin <rea-fbsd@codelabs.ru>:
> > There is kern/127042 (rather old one, but it essentially the same as the
> > current PR) that addresses this aspect of the problem: it
> >
> > - 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').
> 
> sorry for bad patch, my bad s/copy/coffee/

No problems.  Care to test both patches?  If yes, please, report back
any findings of any type.

> thanks for working on this.

You're welcome ;))
-- 
Eygene
 _                ___       _.--.   #
 \`.|\..----...-'`   `-._.-'_.-'`   #  Remember that it is hard
 /  ' `         ,       __.--'      #  to read the on-line manual
 )/' _/     \   `-_,   /            #  while single-stepping the kernel.
 `-'" `"\_  ,_.-;_.-\_ ',  fsc/as   #
     _.-'_./   {_.'   ; /           #    -- FreeBSD Developers handbook
    {_.-``-'         {_/            #

2009/7/10 Eygene Ryabinkin <rea-fbsd@codelabs.ru>

>  No problems.  Care to test both patches?  If yes, please, report back
>  any findings of any type.
>


Hi, patch works good on amd64, 8.0-BETA2, r195712


root@FreeBSD ~# ifconfig carp0 name carp
ifconfig: ioctl (set name): File exists





-- 
Artis Caune

   Everything should be made as simple as possible, but not simpler.

Comment 6 Eitan Adler 2017-12-31 07:58:33 UTC

For bugs matching the following criteria:

Status: In Progress Changed: (is less than) 2014-06-01

Reset to default assignee and clear in-progress tags.

Mail being skipped