> On 01 Sep 2015, at 14:31, Alexandr Nedvedicky > <alexandr.nedvedi...@oracle.com> wrote: > >>> 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. As written above: I do not really care. > 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. Yes, good choice. :) > updated patch is further below. ok jung@ > 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) {