Re: pf: use proper interface for route-to when it is used with sticky-address
On Wed, 10 Jul 2019 23:50:23 +0100
Stuart Henderson wrote:
> On 2019/07/10 23:27, Alexandr Nedvedicky wrote:
>> Hello Stuart,
>>
>> On Wed, Jul 10, 2019 at 08:19:13PM +0100, Stuart Henderson wrote:
>> > On 2019/07/05 17:09, YASUOKA Masahiko wrote:
>> > > Hi,
>> > >
>> > > Previous diff made src-node have a reference for the kif. My
>> > > colleague pointed out that incrementing the reference count of the kif
>> > > is required.
>> > >
>> > > ok?
>> > >
>> > > Fix previous commit which made src-node have a reference for the kif.
>> > > Src-node should use the reference counter since it might live longer
>> > > than its table entry, rule or the associated states.
>> >
>> > I'm seeing crashes soon after starting network which must be related
>> > to this.
>> >
>> > I have a few rules with standard "max-src-conn-rate" options, e.g.
>> > "keep state (max-src-conn-rate 5/8 overload flush global)"
>> > If I remove the max-src-conn-rate things are stable again.
>> >
>>
>> does patch below fix the NULL pointer dereference panic for you?
>>
>> thanks for report and
>> sorry for inconveniences
>>
>> sashan
>
> Yes, that's working OK here now, thanks for the quick response.
Thank you for find and fix.
ok yasuoka
On Wed, 10 Jul 2019 23:50:23 +0100
Stuart Henderson wrote:
> On 2019/07/10 23:27, Alexandr Nedvedicky wrote:
>> Hello Stuart,
>>
>> On Wed, Jul 10, 2019 at 08:19:13PM +0100, Stuart Henderson wrote:
>> > On 2019/07/05 17:09, YASUOKA Masahiko wrote:
>> > > Hi,
>> > >
>> > > Previous diff made src-node have a reference for the kif. My
>> > > colleague pointed out that incrementing the reference count of the kif
>> > > is required.
>> > >
>> > > ok?
>> > >
>> > > Fix previous commit which made src-node have a reference for the kif.
>> > > Src-node should use the reference counter since it might live longer
>> > > than its table entry, rule or the associated states.
>> >
>> > I'm seeing crashes soon after starting network which must be related
>> > to this.
>> >
>> > I have a few rules with standard "max-src-conn-rate" options, e.g.
>> > "keep state (max-src-conn-rate 5/8 overload flush global)"
>> > If I remove the max-src-conn-rate things are stable again.
>> >
>>
>> does patch below fix the NULL pointer dereference panic for you?
>>
>> thanks for report and
>> sorry for inconveniences
>>
>> sashan
>
> Yes, that's working OK here now, thanks for the quick response.
>
>
>> 8<---8<---8<--8<
>> diff --git a/sys/net/pf.c b/sys/net/pf.c
>> index 26c3d420254..9addec6d788 100644
>> --- a/sys/net/pf.c
>> +++ b/sys/net/pf.c
>> @@ -586,10 +586,12 @@ pf_insert_src_node(struct pf_src_node **sn, struct
>> pf_rule *rule,
>> }
>> (*sn)->creation = time_uptime;
>> (*sn)->rule.ptr->src_nodes++;
>> -(*sn)->kif = kif;
>> +if (kif != NULL) {
>> +(*sn)->kif = kif;
>> +pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
>> +}
>> pf_status.scounters[SCNT_SRC_NODE_INSERT]++;
>> pf_status.src_nodes++;
>> -pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
>> } else {
>> if (rule->max_src_states &&
>> (*sn)->states >= rule->max_src_states) {
>>
Re: pf: use proper interface for route-to when it is used with sticky-address
On 2019/07/10 23:27, Alexandr Nedvedicky wrote:
> Hello Stuart,
>
> On Wed, Jul 10, 2019 at 08:19:13PM +0100, Stuart Henderson wrote:
> > On 2019/07/05 17:09, YASUOKA Masahiko wrote:
> > > Hi,
> > >
> > > Previous diff made src-node have a reference for the kif. My
> > > colleague pointed out that incrementing the reference count of the kif
> > > is required.
> > >
> > > ok?
> > >
> > > Fix previous commit which made src-node have a reference for the kif.
> > > Src-node should use the reference counter since it might live longer
> > > than its table entry, rule or the associated states.
> >
> > I'm seeing crashes soon after starting network which must be related
> > to this.
> >
> > I have a few rules with standard "max-src-conn-rate" options, e.g.
> > "keep state (max-src-conn-rate 5/8 overload flush global)"
> > If I remove the max-src-conn-rate things are stable again.
> >
>
> does patch below fix the NULL pointer dereference panic for you?
>
> thanks for report and
> sorry for inconveniences
>
> sashan
Yes, that's working OK here now, thanks for the quick response.
> 8<---8<---8<--8<
> diff --git a/sys/net/pf.c b/sys/net/pf.c
> index 26c3d420254..9addec6d788 100644
> --- a/sys/net/pf.c
> +++ b/sys/net/pf.c
> @@ -586,10 +586,12 @@ pf_insert_src_node(struct pf_src_node **sn, struct
> pf_rule *rule,
> }
> (*sn)->creation = time_uptime;
> (*sn)->rule.ptr->src_nodes++;
> - (*sn)->kif = kif;
> + if (kif != NULL) {
> + (*sn)->kif = kif;
> + pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
> + }
> pf_status.scounters[SCNT_SRC_NODE_INSERT]++;
> pf_status.src_nodes++;
> - pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
> } else {
> if (rule->max_src_states &&
> (*sn)->states >= rule->max_src_states) {
>
Re: pf: use proper interface for route-to when it is used with sticky-address
Hello Stuart,
On Wed, Jul 10, 2019 at 08:19:13PM +0100, Stuart Henderson wrote:
> On 2019/07/05 17:09, YASUOKA Masahiko wrote:
> > Hi,
> >
> > Previous diff made src-node have a reference for the kif. My
> > colleague pointed out that incrementing the reference count of the kif
> > is required.
> >
> > ok?
> >
> > Fix previous commit which made src-node have a reference for the kif.
> > Src-node should use the reference counter since it might live longer
> > than its table entry, rule or the associated states.
>
> I'm seeing crashes soon after starting network which must be related
> to this.
>
> I have a few rules with standard "max-src-conn-rate" options, e.g.
> "keep state (max-src-conn-rate 5/8 overload flush global)"
> If I remove the max-src-conn-rate things are stable again.
>
does patch below fix the NULL pointer dereference panic for you?
thanks for report and
sorry for inconveniences
sashan
8<---8<---8<--8<
diff --git a/sys/net/pf.c b/sys/net/pf.c
index 26c3d420254..9addec6d788 100644
--- a/sys/net/pf.c
+++ b/sys/net/pf.c
@@ -586,10 +586,12 @@ pf_insert_src_node(struct pf_src_node **sn, struct
pf_rule *rule,
}
(*sn)->creation = time_uptime;
(*sn)->rule.ptr->src_nodes++;
- (*sn)->kif = kif;
+ if (kif != NULL) {
+ (*sn)->kif = kif;
+ pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
+ }
pf_status.scounters[SCNT_SRC_NODE_INSERT]++;
pf_status.src_nodes++;
- pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
} else {
if (rule->max_src_states &&
(*sn)->states >= rule->max_src_states) {
Re: pf: use proper interface for route-to when it is used with sticky-address
On 2019/07/05 17:09, YASUOKA Masahiko wrote:
> Hi,
>
> Previous diff made src-node have a reference for the kif. My
> colleague pointed out that incrementing the reference count of the kif
> is required.
>
> ok?
>
> Fix previous commit which made src-node have a reference for the kif.
> Src-node should use the reference counter since it might live longer
> than its table entry, rule or the associated states.
I'm seeing crashes soon after starting network which must be related
to this.
I have a few rules with standard "max-src-conn-rate" options, e.g.
"keep state (max-src-conn-rate 5/8 overload flush global)"
If I remove the max-src-conn-rate things are stable again.
starting early daemons: syslogd unbounduvm_fault(0x81d9cb00, 0xe4, 0,
2) -> e
^Mkernel: page fault trap, code=0
^MStopped at pfi_kif_ref+0x3f [/sys/net/pf_if.c:151]:addl
$0x1,0x
^Me4(%rdi)
^Mddb{3}> tr
^Mpfi_kif_ref(80001f806d98,8060d010,0,2,fd805f66e424,0) at
pfi_ki
^Mf_ref+0x3f [/sys/net/pf_if.c:151]
^Mpf_test_rule(80001f806eb8,80001f806fa8,80001f806fc0,80001f806f9
^M0,80001f806f80,80001f806fce) at pf_test_rule+0x812
[/sys/net/pf.c:3886]
^M
^Mpf_test(2,1,80184000,80001f8070d8,1c23766404077ddd,80184000
^M) at pf_test+0x794 [/sys/net/pf.c:0]
^Mip_input_if(80001f8070d8,80001f8070e4,4,0,80184000,80001f80
^M70d8) at ip_input_if+0x3d6 [/sys/netinet/ip_input.c:356]
^Mipv4_input(80184000,fd8060dc8f00,159191501168ecca,1f807122,fd80
^M60dc8f00,80184000) at ipv4_input+0x39 [/sys/netinet/ip_input.c:256]
^Msppp_input(80184000,fd8060dc8f00,2269ff70e5351674,30,fd8060dc8f
^M00,80022040) at sppp_input+0x2b3 [/sys/net/if_spppsubr.c:505]
^Mpppoeintr(b3d67ef4c5ba913d,0,4000,80022040,816acdb0,0) at
p
^Mppoeintr+0xa04 [/sys/net/if_pppoe.c:734]
^Mif_netisr(0,0,80001f807230,80022040,81581a7d,80001f8072
^M20) at if_netisr+0xbd [/sys/net/if.c:1028]
^Mtaskq_thread(80022040,80022040,0,0,816acdb0,0) at
taskq
^M_thread+0x4d [/sys/kern/kern_task.c:369]
^Mend trace frame: 0x0, count: -9
^Mddb{3}> ps /o
^MTIDPIDUID PRFLAGS PFLAGS CPU COMMAND
^M 359291 82145 0 0x3 00 pgrep
^M 439451 16713 0 0x3 02 unbound-checkcon
^M*169187 90314 0 0x14000 0x2003K softnet
^Mddb{3}> sh reg
^Mrdi0
^Mrsi 0x4
^Mrbp 0x80001f806d20
^Mrbx0
^Mrdx 0x1388__ALIGN_SIZE+0x388
^Mrcx 0xfd805f5d1ed8
^Mrax 0xfd805f5d1ed8
^Mr8 0
^Mr9 0
^Mr10 0x4a4f63677d822114
^Mr11 0x1c6ef5dd3e707ef4
^Mr12 0x8060d010
^Mr13 0x
^Mr14 0x80001f806d98
^Mr15 0xfd805f66e424
^Mrip 0x8157d10fpfi_kif_ref+0x3f
^Mcs 0x8
^Mrflags 0x10246__ALIGN_SIZE+0xf246
^Mrsp 0x80001f806c08
^Mss 0x10
^Mpfi_kif_ref+0x3f [/sys/net/pf_if.c:151]:addl$0x1,0xe4(%rdi)
Re: pf: use proper interface for route-to when it is used with sticky-address
Hello Yasuoka, > Previous diff made src-node have a reference for the kif. My > colleague pointed out that incrementing the reference count of the kif > is required. > > ok? > very good catch, indeed. Thanks for taking care of it. diff looks good to me. OK sashan
Re: pf: use proper interface for route-to when it is used with sticky-address
Hi,
Previous diff made src-node have a reference for the kif. My
colleague pointed out that incrementing the reference count of the kif
is required.
ok?
Fix previous commit which made src-node have a reference for the kif.
Src-node should use the reference counter since it might live longer
than its table entry, rule or the associated states.
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1083
diff -u -p -r1.1083 pf.c
--- sys/net/pf.c2 Jul 2019 09:04:53 - 1.1083
+++ sys/net/pf.c5 Jul 2019 07:57:57 -
@@ -589,6 +589,7 @@ pf_insert_src_node(struct pf_src_node **
(*sn)->kif = kif;
pf_status.scounters[SCNT_SRC_NODE_INSERT]++;
pf_status.src_nodes++;
+ pfi_kif_ref(kif, PFI_KIF_REF_SRCNODE);
} else {
if (rule->max_src_states &&
(*sn)->states >= rule->max_src_states) {
@@ -612,6 +613,7 @@ pf_remove_src_node(struct pf_src_node *s
RB_REMOVE(pf_src_tree, _src_tracking, sn);
pf_status.scounters[SCNT_SRC_NODE_REMOVALS]++;
pf_status.src_nodes--;
+ pfi_kif_unref(sn->kif, PFI_KIF_REF_SRCNODE);
pool_put(_src_tree_pl, sn);
}
Index: sys/net/pf_if.c
===
RCS file: /cvs/src/sys/net/pf_if.c,v
retrieving revision 1.96
diff -u -p -r1.96 pf_if.c
--- sys/net/pf_if.c 10 Dec 2018 16:48:15 - 1.96
+++ sys/net/pf_if.c 5 Jul 2019 07:57:57 -
@@ -147,6 +147,9 @@ pfi_kif_ref(struct pfi_kif *kif, enum pf
case PFI_KIF_REF_ROUTE:
kif->pfik_routes++;
break;
+ case PFI_KIF_REF_SRCNODE:
+ kif->pfik_srcnodes++;
+ break;
default:
panic("pfi_kif_ref with unknown type");
}
@@ -185,6 +188,14 @@ pfi_kif_unref(struct pfi_kif *kif, enum
}
kif->pfik_routes--;
break;
+ case PFI_KIF_REF_SRCNODE:
+ if (kif->pfik_srcnodes <= 0) {
+ DPFPRINTF(LOG_ERR,
+ "pfi_kif_unref: src-node refcount <= 0");
+ return;
+ }
+ kif->pfik_srcnodes--;
+ break;
default:
panic("pfi_kif_unref with unknown type");
}
@@ -192,7 +203,8 @@ pfi_kif_unref(struct pfi_kif *kif, enum
if (kif->pfik_ifp != NULL || kif->pfik_group != NULL || kif == pfi_all)
return;
- if (kif->pfik_rules || kif->pfik_states || kif->pfik_routes)
+ if (kif->pfik_rules || kif->pfik_states || kif->pfik_routes ||
+ kif->pfik_srcnodes)
return;
RB_REMOVE(pfi_ifhead, _ifs, kif);
Index: sys/net/pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.491
diff -u -p -r1.491 pfvar.h
--- sys/net/pfvar.h 2 Jul 2019 09:04:53 - 1.491
+++ sys/net/pfvar.h 5 Jul 2019 07:57:58 -
@@ -1162,6 +1162,7 @@ struct pfi_kif {
int pfik_states;
int pfik_rules;
int pfik_routes;
+ int pfik_srcnodes;
TAILQ_HEAD(, pfi_dynaddr)pfik_dynaddrs;
};
@@ -1169,7 +1170,8 @@ enum pfi_kif_refs {
PFI_KIF_REF_NONE,
PFI_KIF_REF_STATE,
PFI_KIF_REF_RULE,
- PFI_KIF_REF_ROUTE
+ PFI_KIF_REF_ROUTE,
+ PFI_KIF_REF_SRCNODE
};
#define PFI_IFLAG_SKIP 0x0100 /* skip filtering on interface */
Re: pf: use proper interface for route-to when it is used with sticky-address
Hello, On Mon, Jul 01, 2019 at 09:11:28PM +0900, YASUOKA Masahiko wrote: > Hi, > > "route-to" is used with the interface used for the next hop. But if > it is used with a source address track (sticky-address), it sometimes > output the packet to wrong interface. > > Example: > > pass in quick inet proto tcp from any to 192.168.0.10 port 80 \ > keep state (sloppy) route-to source-hash hogehoge sticky-address > > # pfctl -Tshow -tLB > 127.0.0.1@lo0 > 192.168.0.101@em0 > 192.168.0.102@em0 > # > thank you for providing example. Your change looks good. OK sashan
pf: use proper interface for route-to when it is used with sticky-address
Hi,
"route-to" is used with the interface used for the next hop. But if
it is used with a source address track (sticky-address), it sometimes
output the packet to wrong interface.
Example:
pass in quick inet proto tcp from any to 192.168.0.10 port 80 \
keep state (sloppy) route-to source-hash hogehoge sticky-address
# pfctl -Tshow -tLB
127.0.0.1@lo0
192.168.0.101@em0
192.168.0.102@em0
#
pf_test() => pf_route() uses r->route->kif as the output inteface.
If there is no active source address tracking record, r->route->kif is
properly set with the table entry at pfr_pool_get() (called from
pf_map_addr() <= pf_route() <= pf_route()). But there is an active
source tracking record, pf_map_addr() returns without updating
r->route->kif.
339 int
340 pf_map_addr(sa_family_t af, struct pf_rule *r, struct pf_addr *saddr,
341 struct pf_addr *naddr, struct pf_addr *init_addr, struct
pf_src_node **sns,
342 struct pf_pool *rpool, enum pf_sn_types type)
343 {
(snip)
355 if (sns[type] == NULL && rpool->opts & PF_POOL_STICKYADDR &&
356 (rpool->opts & PF_POOL_TYPEMASK) != PF_POOL_NONE &&
357 pf_map_addr_sticky(af, r, saddr, naddr, sns, rpool, type)
== 0)
358 return (0);
Since pf_map_sticky() doesn't update the kif, kif used previous is
used mistakenly.
ok?
When source address tracking record is used for "route-to", the next
hop interface configured with "route-to" was not used. Keep the
interface within the pf_src_node and use it when the record is used.
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1081
diff -u -p -r1.1081 pf.c
--- sys/net/pf.c20 Mar 2019 20:07:28 - 1.1081
+++ sys/net/pf.c1 Jul 2019 11:47:36 -
@@ -542,7 +542,7 @@ pf_src_connlimit(struct pf_state **state
int
pf_insert_src_node(struct pf_src_node **sn, struct pf_rule *rule,
enum pf_sn_types type, sa_family_t af, struct pf_addr *src,
-struct pf_addr *raddr)
+struct pf_addr *raddr, struct pfi_kif *kif)
{
struct pf_src_node k;
@@ -586,6 +586,7 @@ pf_insert_src_node(struct pf_src_node **
}
(*sn)->creation = time_uptime;
(*sn)->rule.ptr->src_nodes++;
+ (*sn)->kif = kif;
pf_status.scounters[SCNT_SRC_NODE_INSERT]++;
pf_status.src_nodes++;
} else {
@@ -3882,7 +3883,7 @@ pf_test_rule(struct pf_pdesc *pd, struct
if (r->rule_flag & PFRULE_SRCTRACK &&
pf_insert_src_node([PF_SN_NONE], r, PF_SN_NONE,
- pd->af, pd->src, NULL) != 0) {
+ pd->af, pd->src, NULL, NULL) != 0) {
REASON_SET(, PFRES_SRCLIMIT);
goto cleanup;
}
Index: sys/net/pf_lb.c
===
RCS file: /cvs/src/sys/net/pf_lb.c,v
retrieving revision 1.63
diff -u -p -r1.63 pf_lb.c
--- sys/net/pf_lb.c 10 Dec 2018 16:48:15 - 1.63
+++ sys/net/pf_lb.c 1 Jul 2019 11:47:36 -
@@ -329,6 +329,10 @@ pf_map_addr_sticky(sa_family_t af, struc
pf_print_host(naddr, 0, af);
addlog("\n");
}
+
+ if (sns[type]->kif != NULL)
+ rpool->kif = sns[type]->kif;
+
return (0);
}
@@ -618,7 +622,8 @@ pf_map_addr(sa_family_t af, struct pf_ru
pf_remove_src_node(sns[type]);
sns[type] = NULL;
}
- if (pf_insert_src_node([type], r, type, af, saddr, naddr))
+ if (pf_insert_src_node([type], r, type, af, saddr, naddr,
+ rpool->kif))
return (1);
}
Index: sys/net/pfvar.h
===
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.490
diff -u -p -r1.490 pfvar.h
--- sys/net/pfvar.h 18 Feb 2019 13:11:44 - 1.490
+++ sys/net/pfvar.h 1 Jul 2019 11:47:36 -
@@ -1712,7 +1712,7 @@ extern int pf_state_insert(struct
pfi
int pf_insert_src_node(struct pf_src_node **,
struct pf_rule *, enum pf_sn_types,
sa_family_t, struct pf_addr *,
- struct pf_addr *);
+ struct pf_addr *, struct pfi_kif *);
voidpf_remove_src_node(struct pf_src_node *);
struct pf_src_node *pf_get_src_node(struct pf_state *,
enum pf_sn_types);
