Date: Fri, 10 Jul 2009 02:30:34 +0400 From: Eygene Ryabinkin <rea-fbsd@codelabs.ru> To: linimon@FreeBSD.org Cc: freebsd-net@FreeBSD.org, mlaier@freebsd.org, artis.caune@gmail.com, bug-followup@FreeBSD.org Subject: Re: kern/136618: [pf][stf] panic on cloning interface without unit number, e.g. `stf' Message-ID: <hCsIYFcvX6asZcp4d%2Bhhi1QeoOA@sMocRdeSobDWD5aEcWfflbBNqYM> In-Reply-To: <200907091827.n69IR0Sl084730@freefall.freebsd.org> References: <200907091827.n69IR0Sl084730@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--W/nzBZO5zC0uMSeA
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
// 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
=66rom 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=3D"]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=3D"]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=3D&4H<"D["B`*(`DO*B!A9V%I;B!F;W(@86QL(&=3DR;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=3DL+"`F:VEF+3YP9FEK7VEF<"T^:69?9W)O=3D7!S+"!I
M9F=3DL7VYE>'0I"BT)"0EP9FE?:VEF7W5P9&%T92@H<W1R=3D6-T('!F:5]K:68@
M*BD*+0D)"2`@("!I9F=3DL+3YI9F=3DL7V=3DR;W5P+3YI9F=3D?<&9?:VEF*3L**PEI
M9B`H:VEF+3YP9FEK7VEF<"`A/2!.54Q,*2!["BL)"7-T<G5C=3D"!P9FE?:VEF
M("IG<E]K:68["BL)"51!24Q17T9/4D5!0T@H:69G;"P@)FMI9BT^<&9I:U]I
M9G`M/FEF7V=3DR;W5P<RP@:69G;%]N97AT*2!["BL)"0EG<E]K:68@/0HK"0D)
M("`@("AS=3D')U8W0@<&9I7VMI9B`J*6EF9VPM/FEF9VQ?9W)O=3D7`M/FEF9U]P
M9E]K:68["BL)"0DO*@HK"0D)("H@079O:60@:6YF:6YI=3D&4@<F5C=3D7)S:6]N
M.B!I9B!I;G1E<F9A8V4@;F%M92!I<PHK"0D)("H@=3D&AE('-A;64@87,@=3D&AE
M(&EN=3D&5R9F%C92!G<F]U<"!N86UE("AF;W(@97AA;7!L92P**PD)"2`J('-T
M9B@T*2!H87,@;VYL>2!O;F4@:6YS=3D&%N8V4@86YD(&ET<R!K:68@=3DVEL;"!B
M90HK"0D)("H@;F%M960@(G-T9B(L(&%S('1H92!G<F]U<"=3DS(")K:68B+B`@
M06YO=3D&AE<B!C87-E"BL)"0D@*B!I<R!T:&4@:6YT97)F86-E(')E;F%M960@
M=3D&\@: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=3D"!B92!P<F]H:6)I=3D&5D+"!S:6YC92!I=3D"!M86ME<R!P9@HK"0D)("H@<G5L
M97,@=3D&\@8F5H879E(&YO;BUI;G1U:71I=3DF5L>3H@:6YT97)F86-E)W,**PD)
M"2`J(')U;&5S('=3DI;&P@87!P;'D@=3D&\@=3D&AE('=3DH;VQE(&=3DR;W5P+@HK"0D)
M("HO"BL)"0EI9B`H:VEF("$](&=3DR7VMI9BD**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*&=3DR;W5P;F%M92D@+2`Q72`\/2`G.2<I"B`)
M"7)E=3D'5R;B`H14E.5D%,*3L*(`HK"6EF("AI9G5N:70H9W)O=3D7!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=3D7!S+"!I9F=3DL
M7VYE>'0I"B`)"6EF("@A<W1R8VUP*&EF9VPM/FEF9VQ?9W)O=3D7`M/FEF9U]G
M<F]U<"P@9W)O=3D7!N86UE*2D@>PI`0"`M,3DP-RPV("LQ.3$P+#<@0$`*(`EC
M:&%R(&YE=3DU]N86UE6TE&3D%-4TE:73L*(`ES=3D')U8W0@:69A9&1R("II9F$[
M"B`)<W1R=3D6-T('-O8VMA9&1R7V1L("IS9&P["BL)<W1R=3D6-T(&EF9U]G<F]U
M<"`J:69G(#T@3E5,3#L*(`H@"6EF<B`]("AS=3D')U8W0@:69R97$@*BED871A
M.PH@"7-W:71C:"`H8VUD*2!["D!`("TR,#$T+#<@*S(P,3@L,34@0$`*(`D)
M"7)E=3D'5R;B`H14E.5D%,*3L*(`D):68@*&EF=3D6YI=3D"AN97=3D?;F%M92D@(3T@
M3E5,3"D*(`D)"7)E=3D'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=3D?
M;F5X=3D"D**PD)"6EF("@A<W1R8VUP*&EF9RT^:69G7V=3DR;W5P+"!N97=3D?;F%M
M92DI('L**PD)"0E)1DY%5%]254Y,3T-+*"D["BL)"0D)<F5T=3D7)N("A%15A)
M4U0I.PHK"0D)?0HK"0E)1DY%5%]254Y,3T-+*"D["BL*(`D)+RH@06YN;W5N
M8V4@=3D&AE(&1E<&%R=3D'5R92!O9B!T:&4@:6YT97)F86-E+B`J+PH@"0ER=3D%]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=3D'5R95]E=3DF5N=3D"P@:69P*3L*
`
end
--- if.c-avoid-group-and-iface-name-clashes.diff ends here ---
--=20
Eygene
_ ___ _.--. #
\`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard
/ ' ` , __.--' # to read the on-line manual
)/' _/ \ `-_, / # while single-stepping the kernel.
`-'" `"\_ ,_.-;_.-\_ ', fsc/as #
_.-'_./ {_.' ; / # -- FreeBSD Developers handbook
{_.-``-' {_/ #
--W/nzBZO5zC0uMSeA
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="pf_if.c-avoid-infinite-recursion.diff"
--- sys/contrib/pf/net/pf_if.c.orig 2009-07-10 00:19:44.000000000 +0400
+++ sys/contrib/pf/net/pf_if.c 2009-07-10 01:00:16.000000000 +0400
@@ -530,10 +530,26 @@
pfi_dynaddr_update(p);
/* again for all groups kif is member of */
- if (kif->pfik_ifp != NULL)
- TAILQ_FOREACH(ifgl, &kif->pfik_ifp->if_groups, ifgl_next)
- pfi_kif_update((struct pfi_kif *)
- ifgl->ifgl_group->ifg_pf_kif);
+ if (kif->pfik_ifp != NULL) {
+ struct pfi_kif *gr_kif;
+ TAILQ_FOREACH(ifgl, &kif->pfik_ifp->if_groups, ifgl_next) {
+ gr_kif =
+ (struct pfi_kif *)ifgl->ifgl_group->ifg_pf_kif;
+ /*
+ * Avoid infinite recursion: if interface name is
+ * the same as the interface group name (for example,
+ * stf(4) has only one instance and its kif will be
+ * named "stf", as the group's "kif". Another case
+ * is the interface renamed to it's family name
+ * (e.g., "vlanX" -> "vlan"). In fact, the latter
+ * case must be prohibited, since it makes pf
+ * rules to behave non-intuitively: interface's
+ * rules will apply to the whole group.
+ */
+ if (kif != gr_kif)
+ pfi_kif_update(gr_kif);
+ }
+ }
}
void
--W/nzBZO5zC0uMSeA--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?hCsIYFcvX6asZcp4d%2Bhhi1QeoOA>