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) {