> On 01 Sep 2015, at 14:31, Alexandr Nedvedicky
> <[email protected]> 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) {
