</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.c 26 May 2015 16:17:51 -0000 1.916
+++ pf.c 28 May 2015 21:38:09 -0000
@@ -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,
&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)) {
- 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,17 +3571,26 @@
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];
SLIST_INSERT_HEAD(&s->src_nodes, sni, next);
sni->sn->states++;
}
- pf_set_rt_ifp(s, pd->src); /* needs s->state_key set */
+ if (pf_set_rt_ifp(s, pd->src, (*skw)->af) != 0) {
+ REASON_SET(&reason, PFRES_NOROUTE);
+ goto csfailed;
+ }
+
+ 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;
+
if (tag > 0) {
pf_tag_ref(tag);
s->tag = tag;
@@ -3612,12 +3618,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)
pf_remove_src_node(sns[i]);
+
if (s) {
- pf_normalize_tcp_cleanup(s); /* safe even w/o init */
- pf_src_tree_remove_state(s);
STATE_DEC_COUNTERS(s);
pool_put(&pf_state_pl, s);
}
Index: pfvar.h
===================================================================
RCS file: /cvs/src/sys/net/pfvar.h,v
retrieving revision 1.414
diff -u -r1.414 pfvar.h
--- pfvar.h 11 Apr 2015 13:00:12 -0000 1.414
+++ pfvar.h 28 May 2015 21:38:09 -0000
@@ -1316,7 +1316,8 @@
#define PFRES_SRCLIMIT 13 /* Source node/conn limit */
#define PFRES_SYNPROXY 14 /* SYN proxy */
#define PFRES_TRANSLATE 15 /* No translation address
available */
-#define PFRES_MAX 16 /* total+1 */
+#define PFRES_NOROUTE 16 /* No route found for PBR
action */
+#define PFRES_MAX 17 /* total+1 */
#define PFRES_NAMES { \
"match", \
@@ -1335,6 +1336,7 @@
"src-limit", \
"synproxy", \
"translate", \
+ "no-route", \
NULL \
}
