Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 28, 2015 at 23:46 +0200, Alexandr Nedvedicky wrote:
/snip
But we'll drop this reference in pf_src_tree_remove_state,
then how will sns[PF_SN_NAT] and sns[PF_SN_ROUTE] be different?
I think I should take PF class again ;-) I've just realized there
is a test in pf_remove_src_node():
572 if (sn-states 0 || sn-expire time_uptime)
573 return;
so it will do the right thing. This is the piece I was missing.
sns[] array was prepared for this state so if we can't insert
the state, sns entries must be cleaned up. pf_remove_src_node
checks the number of associated states and if source node should
expire some time later.
yes, it seems more clear to me now.
Good! At least I wasn't blind this time! (:
Speaking of PF_SN_ROUTE, pf_set_rt_ifp should be probably called
before we insert the state for the very same reason, plus it
should check the pf_map_addr return value and do the cleanup.
I don't feel entirely qualified now, to discuss the matter ;-)
Hey, don't worry about it. Half of the people reading this have
zero clue as to what the hell are we talking about.
however
pf_set_rt_ifp() should indeed test return value of pf_map_addr(), In case
of
failure the error should be thrown further up, so pf_create_state() can
handle
it. Probably jumping to csfailed: should be sufficient.
You can't just jump to csfailed unless you do a pf_set_rt_ifp
before the pf_state_insert, but then it needs an attached key
to only get it's address family.
true, because once pf_state_insert() succeeds, it's no longer job for
csfailed:
branch to clean up a state. I currently have no better suggestion than taking
the easiest move:
add af argument to pf_set_rt_ifp() and fetch it from skw,
in pf_create_state().
I thought about it and I believe this is correct to assume that the
skw-af is correct even if we won't use this particular state key
object.
Actually there is an option:
remove KASSERT() at 655 in pf_state_key_attach().
659 pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int
idx)
660 {
661 struct pf_state_item*si;
662 struct pf_state_key *cur;
663 struct pf_state *olds = NULL;
664
665 KASSERT(s-key[idx] == NULL);
Then we can simply do:
s-key[PF_SK_WIRE] = skw;
in pf_create_state(), so pf_set_rt_ifp() will get what it expects.
This would be a hack and an overkill really.
adding af argument to pf_set_rt_if() seems more clean approach to me.
Yep.
below is your patch with reshuffled pf_create_state(), so pf_set_rt_ifp()
gets called before, pf_state_insert(). I've introduced a new reason:
PFRES_NOROUTE
Awesome.
however I'm not sure if it is descriptive enough...
Nothing significantly better springs to mind.
I'd also mark pf_set_rt_ifp __inline and remove the marker from
the pf_create_state, but that can be done later.
OK?
regards
sasha
? create_state.diff
? pf.c.diff
? pf.c.patch
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.916
diff -u -r1.916 pf.c
--- pf.c 26 May 2015 16:17:51 - 1.916
+++ pf.c 28 May 2015 21:38:09 -
@@ -204,8 +204,8 @@
u_int16_t pf_get_mss(struct pf_pdesc *);
u_int16_t pf_calc_mss(struct pf_addr *, sa_family_t, int,
u_int16_t);
-void pf_set_rt_ifp(struct pf_state *,
- struct pf_addr *);
+int pf_set_rt_ifp(struct pf_state *,
+ struct pf_addr *, sa_family_t);
struct pf_divert *pf_get_divert(struct mbuf *);
int pf_walk_option6(struct pf_pdesc *, struct ip6_hdr *,
int, int, u_short *);
@@ -2958,32 +2958,39 @@
return (mss);
}
-void
-pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr)
+int
+pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af)
{
struct pf_rule *r = s-rule.ptr;
struct pf_src_node *sns[PF_SN_MAX];
+ int rv;
s-rt_kif = NULL;
if (!r-rt)
- return;
+ return (0);
+
bzero(sns, sizeof(sns));
- switch (s-key[PF_SK_WIRE]-af) {
+ switch (af) {
case AF_INET:
- pf_map_addr(AF_INET, r, saddr, s-rt_addr, NULL, sns,
+ rv = pf_map_addr(AF_INET, r, saddr, s-rt_addr, NULL, sns,
r-route, PF_SN_ROUTE);
- s-rt_kif = r-route.kif;
- s-natrule.ptr = r;
break;
#ifdef INET6
case AF_INET6:
- pf_map_addr(AF_INET6, r, saddr, s-rt_addr, NULL, sns,
+ rv = pf_map_addr(AF_INET6, r, saddr, s-rt_addr, NULL, sns,
Re: pf_create_state() is sometimes better to use pf_unlink_state()
/snip
But we'll drop this reference in pf_src_tree_remove_state,
then how will sns[PF_SN_NAT] and sns[PF_SN_ROUTE] be different?
I think I should take PF class again ;-) I've just realized there
is a test in pf_remove_src_node():
572 if (sn-states 0 || sn-expire time_uptime)
573 return;
so it will do the right thing. This is the piece I was missing.
sns[] array was prepared for this state so if we can't insert
the state, sns entries must be cleaned up. pf_remove_src_node
checks the number of associated states and if source node should
expire some time later.
yes, it seems more clear to me now.
Good! At least I wasn't blind this time! (:
Speaking of PF_SN_ROUTE, pf_set_rt_ifp should be probably called
before we insert the state for the very same reason, plus it
should check the pf_map_addr return value and do the cleanup.
I don't feel entirely qualified now, to discuss the matter ;-)
Hey, don't worry about it. Half of the people reading this have
zero clue as to what the hell are we talking about.
however
pf_set_rt_ifp() should indeed test return value of pf_map_addr(), In case of
failure the error should be thrown further up, so pf_create_state() can
handle
it. Probably jumping to csfailed: should be sufficient.
You can't just jump to csfailed unless you do a pf_set_rt_ifp
before the pf_state_insert, but then it needs an attached key
to only get it's address family.
true, because once pf_state_insert() succeeds, it's no longer job for csfailed:
branch to clean up a state. I currently have no better suggestion than taking
the easiest move:
add af argument to pf_set_rt_ifp() and fetch it from skw,
in pf_create_state().
Actually there is an option:
remove KASSERT() at 655 in pf_state_key_attach().
659 pf_state_key_attach(struct pf_state_key *sk, struct pf_state *s, int
idx)
660 {
661 struct pf_state_item*si;
662 struct pf_state_key *cur;
663 struct pf_state *olds = NULL;
664
665 KASSERT(s-key[idx] == NULL);
Then we can simply do:
s-key[PF_SK_WIRE] = skw;
in pf_create_state(), so pf_set_rt_ifp() will get what it expects.
adding af argument to pf_set_rt_if() seems more clean approach to me.
below is your patch with reshuffled pf_create_state(), so pf_set_rt_ifp()
gets called before, pf_state_insert(). I've introduced a new reason:
PFRES_NOROUTE
however I'm not sure if it is descriptive enough...
regards
sasha
? create_state.diff
? pf.c.diff
? pf.c.patch
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.916
diff -u -r1.916 pf.c
--- pf.c26 May 2015 16:17:51 - 1.916
+++ pf.c28 May 2015 21:38:09 -
@@ -204,8 +204,8 @@
u_int16_t pf_get_mss(struct pf_pdesc *);
u_int16_t pf_calc_mss(struct pf_addr *, sa_family_t, int,
u_int16_t);
-voidpf_set_rt_ifp(struct pf_state *,
- struct pf_addr *);
+int pf_set_rt_ifp(struct pf_state *,
+ struct pf_addr *, sa_family_t);
struct pf_divert *pf_get_divert(struct mbuf *);
int pf_walk_option6(struct pf_pdesc *, struct ip6_hdr *,
int, int, u_short *);
@@ -2958,32 +2958,39 @@
return (mss);
}
-void
-pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr)
+int
+pf_set_rt_ifp(struct pf_state *s, struct pf_addr *saddr, sa_family_t af)
{
struct pf_rule *r = s-rule.ptr;
struct pf_src_node *sns[PF_SN_MAX];
+ int rv;
s-rt_kif = NULL;
if (!r-rt)
- return;
+ return (0);
+
bzero(sns, sizeof(sns));
- switch (s-key[PF_SK_WIRE]-af) {
+ switch (af) {
case AF_INET:
- pf_map_addr(AF_INET, r, saddr, s-rt_addr, NULL, sns,
+ rv = pf_map_addr(AF_INET, r, saddr, s-rt_addr, NULL, sns,
r-route, PF_SN_ROUTE);
- s-rt_kif = r-route.kif;
- s-natrule.ptr = r;
break;
#ifdef INET6
case AF_INET6:
- pf_map_addr(AF_INET6, r, saddr, s-rt_addr, NULL, sns,
+ rv = pf_map_addr(AF_INET6, r, saddr, s-rt_addr, NULL, sns,
r-route, PF_SN_ROUTE);
- s-rt_kif = r-route.kif;
- s-natrule.ptr = r;
break;
#endif /* INET6 */
+ default:
+ rv = 1;
}
+
+ if (rv == 0) {
+ s-rt_kif = r-route.kif;
+ s-natrule.ptr = r;
+ }
+
+ return (rv);
}
u_int32_t
@@ -3557,16 +3564,6 @@
goto csfailed;
}
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
-
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 28, 2015 at 01:17 +0200, Alexandr Nedvedicky wrote:
Hello,
On Wed, May 27, 2015 at 07:44:15PM +0200, Mike Belopuhov wrote:
On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
Hello,
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
This bug is not yours, but doing two pf_state_key_detach is wrong
and results in all kinds of protection fault fireworks. The right
thing is to do pf_detach_state that would handle the case where
PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
good catch.
In csfailed case we do pf_remove_src_node and then call
pf_src_tree_remove_state. With your diff this means that
pf_src_tree_remove_state will be dereferencing sni-sn that
might be pool_put by the pf_remove_src_node. Therefore their
order needs to be reversed.
I see. Another option to fix it would be to do:
sni-sn = sns[i];
sns[i] = NULL;
I'm not sure I follow. Where do you want to do it?
If it's when we pool_get sni's, then this would mean
we won't run pf_remove_src_node and cleanup source
nodes that might have been created.
sorry I was too brief. The snippet below comes from
pf_create_state() with your patch applied:
3560 for (i = 0; i PF_SN_MAX; i++)
3561 if (sns[i] != NULL) {
3562 struct pf_sn_item *sni;
3563
3564 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3565 if (sni == NULL) {
3566 REASON_SET(reason, PFRES_MEMORY);
3567 goto csfailed;
3568 }
3569 sni-sn = sns[i];
3570 sns[i] = NULL;
3571 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3572 sni-sn-states++;
3573 }
3574
the point of my suggestion is to transfer ownership of source node from
local variable sns[] to state. so the check performed later in csfailed
at line 3617, will find NULL pointer in sns[i] field and won't attempt
to call pf_remove_src_node().
3610 csfailed:
3611 if (s) {
3612 pf_normalize_tcp_cleanup(s);/* safe even w/o init
*/
3613 pf_src_tree_remove_state(s);
3614 }
3615
3616 for (i = 0; i PF_SN_MAX; i++)
3617 if (sns[i] != NULL)
3618 pf_remove_src_node(sns[i]);
3619
your patch updated by my suggestion is further below.
But that introduces a potential leak, as I said in my mail:
we won't run pf_remove_src_node and cleanup source nodes that
might have been created.
sns[] is an array of pointers to independently tracked objects
(inserted into the pf_src_tree).
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 28, 2015 at 11:43:02AM +0200, Mike Belopuhov wrote:
On Thu, May 28, 2015 at 01:17 +0200, Alexandr Nedvedicky wrote:
Hello,
On Wed, May 27, 2015 at 07:44:15PM +0200, Mike Belopuhov wrote:
On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
Hello,
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
This bug is not yours, but doing two pf_state_key_detach is wrong
and results in all kinds of protection fault fireworks. The right
thing is to do pf_detach_state that would handle the case where
PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
good catch.
In csfailed case we do pf_remove_src_node and then call
pf_src_tree_remove_state. With your diff this means that
pf_src_tree_remove_state will be dereferencing sni-sn that
might be pool_put by the pf_remove_src_node. Therefore their
order needs to be reversed.
I see. Another option to fix it would be to do:
sni-sn = sns[i];
sns[i] = NULL;
I'm not sure I follow. Where do you want to do it?
If it's when we pool_get sni's, then this would mean
we won't run pf_remove_src_node and cleanup source
nodes that might have been created.
sorry I was too brief. The snippet below comes from
pf_create_state() with your patch applied:
3560 for (i = 0; i PF_SN_MAX; i++)
3561 if (sns[i] != NULL) {
3562 struct pf_sn_item *sni;
3563
3564 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3565 if (sni == NULL) {
3566 REASON_SET(reason, PFRES_MEMORY);
3567 goto csfailed;
3568 }
3569 sni-sn = sns[i];
3570 sns[i] = NULL;
3571 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3572 sni-sn-states++;
3573 }
3574
the point of my suggestion is to transfer ownership of source node from
local variable sns[] to state. so the check performed later in csfailed
at line 3617, will find NULL pointer in sns[i] field and won't attempt
to call pf_remove_src_node().
3610 csfailed:
3611 if (s) {
3612 pf_normalize_tcp_cleanup(s);/* safe even w/o
init */
3613 pf_src_tree_remove_state(s);
3614 }
3615
3616 for (i = 0; i PF_SN_MAX; i++)
3617 if (sns[i] != NULL)
3618 pf_remove_src_node(sns[i]);
3619
your patch updated by my suggestion is further below.
But that introduces a potential leak, as I said in my mail:
we won't run pf_remove_src_node and cleanup source nodes that
might have been created.
sns[] is an array of pointers to independently tracked objects
(inserted into the pf_src_tree).
yes you are right, I was completely wrong. Still my only concern is what
happens in case we fail to allocate source node item (sni) for let's say
PF_SN_NAT type, while there is PF_SN_ROUTE type in sns[] to be processed.
I'm referring to line numbers above. So we fail to allocate PF_SN_NAT
source node and taking goto at 3567, arriving to 3610.
We do the right thing for state clean up (lines 3612, 3613). And now
arriving to for loop 3616, which will remove all source nodes from
sns[], including PF_SN_ROUTE. I completely agree we should do
pf_remove_src_node(sns[PF_SN_NAT]) as we took reference at 3572 for it.
I'm not sure if we should be calling pf_remove_src_node(sns[PF_SN_ROUTE])
here since line 3572 was not executed for it, for loop at 3560 terminated
prematurely. Probably introducing new for-loop counter 'j' in csfailed:
will fix it:
3616 for (j = 0; j i; j++)
regards
sasha
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 28, 2015 at 13:34 +0200, Alexandr Nedvedicky wrote:
On Thu, May 28, 2015 at 11:43:02AM +0200, Mike Belopuhov wrote:
On Thu, May 28, 2015 at 01:17 +0200, Alexandr Nedvedicky wrote:
Hello,
On Wed, May 27, 2015 at 07:44:15PM +0200, Mike Belopuhov wrote:
On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
Hello,
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks,
s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
This bug is not yours, but doing two pf_state_key_detach is wrong
and results in all kinds of protection fault fireworks. The right
thing is to do pf_detach_state that would handle the case where
PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
good catch.
In csfailed case we do pf_remove_src_node and then call
pf_src_tree_remove_state. With your diff this means that
pf_src_tree_remove_state will be dereferencing sni-sn that
might be pool_put by the pf_remove_src_node. Therefore their
order needs to be reversed.
I see. Another option to fix it would be to do:
sni-sn = sns[i];
sns[i] = NULL;
I'm not sure I follow. Where do you want to do it?
If it's when we pool_get sni's, then this would mean
we won't run pf_remove_src_node and cleanup source
nodes that might have been created.
sorry I was too brief. The snippet below comes from
pf_create_state() with your patch applied:
3560 for (i = 0; i PF_SN_MAX; i++)
3561 if (sns[i] != NULL) {
3562 struct pf_sn_item *sni;
3563
3564 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3565 if (sni == NULL) {
3566 REASON_SET(reason, PFRES_MEMORY);
3567 goto csfailed;
3568 }
3569 sni-sn = sns[i];
3570 sns[i] = NULL;
3571 SLIST_INSERT_HEAD(s-src_nodes, sni,
next);
3572 sni-sn-states++;
3573 }
3574
the point of my suggestion is to transfer ownership of source node from
local variable sns[] to state. so the check performed later in csfailed
at line 3617, will find NULL pointer in sns[i] field and won't attempt
to call pf_remove_src_node().
3610 csfailed:
3611 if (s) {
3612 pf_normalize_tcp_cleanup(s);/* safe even w/o
init */
3613 pf_src_tree_remove_state(s);
3614 }
3615
3616 for (i = 0; i PF_SN_MAX; i++)
3617 if (sns[i] != NULL)
3618 pf_remove_src_node(sns[i]);
3619
your patch updated by my suggestion is further below.
But that introduces a potential leak, as I said in my mail:
we won't run pf_remove_src_node and cleanup source nodes that
might have been created.
sns[] is an array of pointers to independently tracked objects
(inserted into the pf_src_tree).
yes you are right, I was completely wrong. Still my only concern is what
happens in case we fail to allocate source node item (sni) for let's say
PF_SN_NAT type, while there is PF_SN_ROUTE type in sns[] to be processed.
I'm referring to line numbers above. So we fail to allocate PF_SN_NAT
source node and taking goto at 3567, arriving to 3610.
We do the right thing for state clean up (lines 3612, 3613). And now
arriving to for loop 3616, which will remove all source nodes from
sns[], including PF_SN_ROUTE. I completely agree we should do
pf_remove_src_node(sns[PF_SN_NAT]) as we took reference at 3572 for it.
But we'll drop this reference in pf_src_tree_remove_state,
then how will sns[PF_SN_NAT] and sns[PF_SN_ROUTE] be different?
I'm not sure if we should be calling pf_remove_src_node(sns[PF_SN_ROUTE])
here since line 3572 was not executed for it, for loop at 3560 terminated
prematurely. Probably introducing new for-loop counter 'j' in csfailed:
will fix it:
3616 for (j = 0; j i; j++)
sns[] array was prepared for this state so if we can't insert
the state, sns entries must be cleaned up. pf_remove_src_node
checks the number of associated states and if source node should
expire some time later.
Speaking of PF_SN_ROUTE, pf_set_rt_ifp should be probably called
before we insert the state for the very same reason, plus it
should check the pf_map_addr return value and do the cleanup.
regards
sasha
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
Hello,
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
This bug is not yours, but doing two pf_state_key_detach is wrong
and results in all kinds of protection fault fireworks. The right
thing is to do pf_detach_state that would handle the case where
PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
good catch.
In csfailed case we do pf_remove_src_node and then call
pf_src_tree_remove_state. With your diff this means that
pf_src_tree_remove_state will be dereferencing sni-sn that
might be pool_put by the pf_remove_src_node. Therefore their
order needs to be reversed.
I see. Another option to fix it would be to do:
sni-sn = sns[i];
sns[i] = NULL;
I'm not sure I follow. Where do you want to do it?
If it's when we pool_get sni's, then this would mean
we won't run pf_remove_src_node and cleanup source
nodes that might have been created.
since your fix works my comment is kind of bikeshedding.
regards
sasha
Re: pf_create_state() is sometimes better to use pf_unlink_state()
Hello,
On Wed, May 27, 2015 at 07:44:15PM +0200, Mike Belopuhov wrote:
On Wed, May 27, 2015 at 10:39 +0200, Alexandr Nedvedicky wrote:
Hello,
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
This bug is not yours, but doing two pf_state_key_detach is wrong
and results in all kinds of protection fault fireworks. The right
thing is to do pf_detach_state that would handle the case where
PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
good catch.
In csfailed case we do pf_remove_src_node and then call
pf_src_tree_remove_state. With your diff this means that
pf_src_tree_remove_state will be dereferencing sni-sn that
might be pool_put by the pf_remove_src_node. Therefore their
order needs to be reversed.
I see. Another option to fix it would be to do:
sni-sn = sns[i];
sns[i] = NULL;
I'm not sure I follow. Where do you want to do it?
If it's when we pool_get sni's, then this would mean
we won't run pf_remove_src_node and cleanup source
nodes that might have been created.
sorry I was too brief. The snippet below comes from
pf_create_state() with your patch applied:
3560 for (i = 0; i PF_SN_MAX; i++)
3561 if (sns[i] != NULL) {
3562 struct pf_sn_item *sni;
3563
3564 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3565 if (sni == NULL) {
3566 REASON_SET(reason, PFRES_MEMORY);
3567 goto csfailed;
3568 }
3569 sni-sn = sns[i];
3570 sns[i] = NULL;
3571 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3572 sni-sn-states++;
3573 }
3574
the point of my suggestion is to transfer ownership of source node from
local variable sns[] to state. so the check performed later in csfailed
at line 3617, will find NULL pointer in sns[i] field and won't attempt
to call pf_remove_src_node().
3610 csfailed:
3611 if (s) {
3612 pf_normalize_tcp_cleanup(s);/* safe even w/o init */
3613 pf_src_tree_remove_state(s);
3614 }
3615
3616 for (i = 0; i PF_SN_MAX; i++)
3617 if (sns[i] != NULL)
3618 pf_remove_src_node(sns[i]);
3619
your patch updated by my suggestion is further below.
regards
sasha
? pf.c.diff
? pf.c.patch
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.916
diff -u -r1.916 pf.c
--- pf.c26 May 2015 16:17:51 - 1.916
+++ pf.c27 May 2015 22:59:32 -
@@ -3557,16 +3557,6 @@
goto csfailed;
}
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
- *sks = *skw = NULL;
- REASON_SET(reason, PFRES_STATEINS);
- goto csfailed;
- } else
- *sm = s;
-
- /* attach src nodes late, otherwise cleanup on error nontrivial */
for (i = 0; i PF_SN_MAX; i++)
if (sns[i] != NULL) {
struct pf_sn_item *sni;
@@ -3574,16 +3564,22 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
- return (PF_DROP);
+ goto csfailed;
}
sni-sn = sns[i];
+ sns[i] = NULL;
SLIST_INSERT_HEAD(s-src_nodes, sni, next);
sni-sn-states++;
}
+ if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
+ pf_detach_state(s);
+ *sks = *skw = NULL;
+ REASON_SET(reason, PFRES_STATEINS);
+ goto csfailed;
+ } else
+ *sm = s;
+
pf_set_rt_ifp(s, pd-src); /* needs s-state_key set */
if (tag 0) {
pf_tag_ref(tag);
@@ -3612,12 +3608,16 @@
return (PF_PASS);
csfailed:
+ if (s) {
+ pf_normalize_tcp_cleanup(s);/* safe even w/o init */
+ pf_src_tree_remove_state(s);
+ }
+
for (i = 0; i PF_SN_MAX; i++)
if (sns[i] != NULL)
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 21, 2015 at 21:28 +0200, Alexandr Nedvedicky wrote:
Hello,
On Thu, May 21, 2015 at 07:43:51PM +0200, Mike Belopuhov wrote:
On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
Hello,
Hi,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks,
s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error
nontrivial */
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni,
next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the cleanup on error nontrivial. Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
perhaps swapping the for loop block with pf_state_insert() will work.
We can then bail out using goto csfailed then (see patch below...)
Yup.
we should do a pf_state_insert afterwards? This might simplify
locking later on.
speaking of locking... everything is more complicated, however
in this particular case it makes things easier for sure. basically in
our current SMP model, once we insert state to table, the only way
to remove it is to leave the job on garbage collector thread...
Right.
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
We would definitely be interested in such a diff, but at the moment
there's no use for it as pf_test is called directly from the IP stack
and hence IP stack needs to support parallel execution first. This
doesn't mean that we can't start integrating bits and pieces, esp.
if they're presented as separate patches.
we use a compile time switch to enable SMP code (-D_PF_SMP_). this is
something I'd like to keep around anyway. I'll start on syncing up our changes
with CURRENT. It will take couple of weeks (ENOCYCLES as usually). Once
I'll have something to show I'll share it and you'll see what can be done
with it.
Cool. Looking forward to!
Thanks for your quality diffs btw, help is always much appreciated.
I'm just trying to be useful. It's pleasure to work with PF sources.
thanks and
regards
sasha
Unfortunately this diff has two nasty bugs (one is not even yours).
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 19:18:04 -
@@ -3556,15 +3556,6 @@
goto csfailed;
}
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
This bug is not yours, but doing two pf_state_key_detach is wrong
and results in all kinds of protection fault fireworks. The right
thing is to do pf_detach_state that would handle the case where
PF_SK_STACK == PF_SK_WIRE (i.e. we need only one invocation).
- *sks = *skw = NULL;
- REASON_SET(reason, PFRES_STATEINS);
- goto csfailed;
- } else
- *sm = s;
-
/* attach src nodes late, otherwise cleanup on error nontrivial */
for (i = 0; i PF_SN_MAX; i++)
if (sns[i] != NULL) {
@@ -3572,16
Re: pf_create_state() is sometimes better to use pf_unlink_state()
* Alexandr Nedvedicky alexandr.nedvedi...@oracle.com [2015-05-21 21:31]:
On Thu, May 21, 2015 at 07:43:51PM +0200, Mike Belopuhov wrote:
On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks,
s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error
nontrivial */
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni,
next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the cleanup on error nontrivial. Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
perhaps swapping the for loop block with pf_state_insert() will work.
We can then bail out using goto csfailed then (see patch below...)
makes sense, I like it.
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
I'd certainly like to see it.
As Mike points out, there's more to do before it can be useful for us
tho :/
Thanks for your quality diffs btw, help is always much appreciated.
absolutely!
--
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS. Virtual Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/
Re: pf_create_state() is sometimes better to use pf_unlink_state()
On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
Hello,
Hi,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error
nontrivial */
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the cleanup on error nontrivial. Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
We would definitely be interested in such a diff, but at the moment
there's no use for it as pf_test is called directly from the IP stack
and hence IP stack needs to support parallel execution first. This
doesn't mean that we can't start integrating bits and pieces, esp.
if they're presented as separate patches.
Thanks for your quality diffs btw, help is always much appreciated.
Cheers,
Mike
regards
sasha
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 15:20:01 -
@@ -3573,9 +3573,7 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
+ pf_unlink_state(s);
return (PF_DROP);
}
sni-sn = sns[i];
Re: pf_create_state() is sometimes better to use pf_unlink_state()
Hello,
On Thu, May 21, 2015 at 07:43:51PM +0200, Mike Belopuhov wrote:
On Thu, May 21, 2015 at 17:34 +0200, Alexandr Nedvedicky wrote:
Hello,
Hi,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error
nontrivial */
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
Indeed. But I don't like the comment stating that we're attaching
src nodes late because the cleanup on error nontrivial. Perhaps
we should do a pf_state_insert afterwards? This might simplify
locking later on.
perhaps swapping the for loop block with pf_state_insert() will work.
We can then bail out using goto csfailed then (see patch below...)
we should do a pf_state_insert afterwards? This might simplify
locking later on.
speaking of locking... everything is more complicated, however
in this particular case it makes things easier for sure. basically in
our current SMP model, once we insert state to table, the only way
to remove it is to leave the job on garbage collector thread...
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
We would definitely be interested in such a diff, but at the moment
there's no use for it as pf_test is called directly from the IP stack
and hence IP stack needs to support parallel execution first. This
doesn't mean that we can't start integrating bits and pieces, esp.
if they're presented as separate patches.
we use a compile time switch to enable SMP code (-D_PF_SMP_). this is
something I'd like to keep around anyway. I'll start on syncing up our changes
with CURRENT. It will take couple of weeks (ENOCYCLES as usually). Once
I'll have something to show I'll share it and you'll see what can be done
with it.
Thanks for your quality diffs btw, help is always much appreciated.
I'm just trying to be useful. It's pleasure to work with PF sources.
thanks and
regards
sasha
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 19:18:04 -
@@ -3556,15 +3556,6 @@
goto csfailed;
}
- if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
- pf_state_key_detach(s, PF_SK_STACK);
- pf_state_key_detach(s, PF_SK_WIRE);
- *sks = *skw = NULL;
- REASON_SET(reason, PFRES_STATEINS);
- goto csfailed;
- } else
- *sm = s;
-
/* attach src nodes late, otherwise cleanup on error nontrivial */
for (i = 0; i PF_SN_MAX; i++)
if (sns[i] != NULL) {
@@ -3572,16 +3563,21 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
- REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
- return (PF_DROP);
+ goto csfailed;
}
sni-sn = sns[i];
pf_create_state() is sometimes better to use pf_unlink_state()
Hello,
snippet below comes from pf_create_state():
3559 if (pf_state_insert(BOUND_IFACE(r, pd-kif), skw, sks, s)) {
3560 pf_state_key_detach(s, PF_SK_STACK);
3561 pf_state_key_detach(s, PF_SK_WIRE);
3562 *sks = *skw = NULL;
3563 REASON_SET(reason, PFRES_STATEINS);
3564 goto csfailed;
3565 } else
3566 *sm = s;
3567
3568 /* attach src nodes late, otherwise cleanup on error nontrivial
*/
3569 for (i = 0; i PF_SN_MAX; i++)
3570 if (sns[i] != NULL) {
3571 struct pf_sn_item *sni;
3572
3573 sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
3574 if (sni == NULL) {
3575 REASON_SET(reason, PFRES_MEMORY);
3576 pf_src_tree_remove_state(s);
3577 STATE_DEC_COUNTERS(s);
3578 pool_put(pf_state_pl, s);
3579 return (PF_DROP);
3580 }
3581 sni-sn = sns[i];
3582 SLIST_INSERT_HEAD(s-src_nodes, sni, next);
3583 sni-sn-states++;
3584 }
at line 3559 PF inserts state to table. If insert operation succeeds, then
state can no longer be killed using simple pool_put() as it currently
happens at line 3578. I think PF should go for pf_unlink_state() instead.
patch below should kill the bug.
also one more off-topic question:
would you be interested in SMP patch for PF?
it basically introduces fine locking and reference counting
on PF data objects, so firewall can handle more packets at
single instance of time.
regards
sasha
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.913
diff -u -r1.913 pf.c
--- pf.c11 May 2015 12:22:14 - 1.913
+++ pf.c21 May 2015 15:20:01 -
@@ -3573,9 +3573,7 @@
sni = pool_get(pf_sn_item_pl, PR_NOWAIT);
if (sni == NULL) {
REASON_SET(reason, PFRES_MEMORY);
- pf_src_tree_remove_state(s);
- STATE_DEC_COUNTERS(s);
- pool_put(pf_state_pl, s);
+ pf_unlink_state(s);
return (PF_DROP);
}
sni-sn = sns[i];
