Hello,
> > As a side effect the patch breaks block rules with dup-to action. dup-to
> > action as a part of block rule might make some sense... So if there is
> > someone, who really needs block ... dup-to he should opt for equivalent
> > rule using pass ... route-to
> >
> > Also there is one more question:
> >
> > shall we implement similar sanity checks for nat-to/rdr-to/... actions?
>
> IMHO, yes that would make sense.
>
I'll try to keep it on my todo list...
> Some bike shedding inline below, apart from that, ok jung@
I love bike shedding, so let's go an pick up some colors ;-)
>
> > - if (r->action == PF_MATCH) {
> > + /*
> > + * Basic rule sanity check.
> > + */
>
> A single line comment is enough here, isn’t it?
>
O.K. new patch has single line comment.
> > + switch (r->action) {
> > + case PF_MATCH:
> > if (r->divert.port) {
> > yyerror("divert is not supported on match rules");
> > problems++;
> > @@ -4016,6 +4019,15 @@ rule_consistent(struct pf_rule *r, int a
> > yyerror("af-to is not supported on match rules");
> > problems++;
> > }
> > + break;
> > + case PF_DROP:
> > + if (r->rt) {
> > + yyerror("route-to, reply-to and dup-to "
> > + "must not be used on block rules”);
>
> The other error messages say “is not supported” instead of “must no be used”.
> I do not care which wording you chose, but maybe take the chance and unify
> it,
> to be more consistent here?
>
the question is which consistency do you want. The patch does not show them,
but there are two options, actually three. Let me show the current code without
patch applied:
4000 /* match rules rules */
4001 if (r->action == PF_MATCH) {
4002 if (r->divert.port) {
4003 yyerror("divert is not supported on match
rules");
4004 problems++;
4005 }
4006 if (r->divert_packet.port) {
4007 yyerror("divert is not supported on match
rules");
4008 problems++;
4009 }
4010 if (r->rt) {
4011 yyerror("route-to, reply-to and dup-to "
4012 "must not be used on match rules");
4013 problems++;
4014 }
4015 if (r->rule_flag & PFRULE_AFTO) {
4016 yyerror("af-to is not supported on match
rules");
4017 problems++;
4018 }
4019 }
as you can see there are two colors to chose from:
color A:
... is not supported on ... rules (used at 4003, 4007, 4016
color B:
... must not be used on match rules (used at line 4911)
we have three options:
1) leave it as it is (both colors will be used)
2) use color A
3) use color B
IMO consistency is good here. I prefer color A as it sounds more polite.
updated patch is further below.
regards
sasha
--------8<---------------8<---------------8<------------------8<--------
Index: sbin/pfctl/parse.y
===================================================================
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.648
diff -u -p -r1.648 parse.y
--- sbin/pfctl/parse.y 21 Apr 2015 16:34:59 -0000 1.648
+++ sbin/pfctl/parse.y 1 Sep 2015 12:29:41 -0000
@@ -3997,8 +3997,9 @@ rule_consistent(struct pf_rule *r, int a
problems++;
}
- /* match rules rules */
- if (r->action == PF_MATCH) {
+ /* Basic rule sanity check. */
+ switch (r->action) {
+ case PF_MATCH:
if (r->divert.port) {
yyerror("divert is not supported on match rules");
problems++;
@@ -4009,13 +4010,22 @@ rule_consistent(struct pf_rule *r, int a
}
if (r->rt) {
yyerror("route-to, reply-to and dup-to "
- "must not be used on match rules");
+ "are not supported on match rules");
problems++;
}
if (r->rule_flag & PFRULE_AFTO) {
yyerror("af-to is not supported on match rules");
problems++;
}
+ break;
+ case PF_DROP:
+ if (r->rt) {
+ yyerror("route-to, reply-to and dup-to "
+ "are not supported on block rules");
+ problems++;
+ }
+ break;
+ default:;
}
return (-problems);
}
Index: sys/net/pf.c
===================================================================
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.936
diff -u -p -r1.936 pf.c
--- sys/net/pf.c 19 Aug 2015 21:22:41 -0000 1.936
+++ sys/net/pf.c 1 Sep 2015 12:29:41 -0000
@@ -6622,6 +6622,10 @@ done:
action = PF_PASS;
break;
#endif /* INET6 */
+ case PF_DROP:
+ m_freem(*m0);
+ *m0 = NULL;
+ break;
default:
/* pf_route can free the mbuf causing *m0 to become NULL */
if (r->rt) {
