Re: [PATCH nft] parser: support reject unreach|reset

[Florian Westphal <fw@strlen.de>]

Patrick McHardy <kaber@trash.net> wrote:
> On Mon, Mar 03, 2014 at 10:12:26PM +0100, Florian Westphal wrote:
> > reject did not allow to use tcp reset instead of icmp unreach.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  After this patch its possibe to do something like
> > 
> >  rule filter output reject reset
> 
> Actually this looks a lot like "iptables thinking" :)

*ahem* :)

> Why not simply use "reset" as keyword?

Yes, that would work as well.

> >  Which makes kernel generate bogus tcp resets in repsonse
> >  to non-tcp packets.
> > 
> >  In iptables this is avoided by making checkentry fail if -p tcp is not
> >  specified when tcp-reset is requested.
> > 
> >  How should this be handled in nft?
> 
> We could either add some infrastructure to the kernel to mimic the iptables
> check, add runtime fallback to ICMP for non-TCP packets or simply ignore them.
> 
> It shouldn't be difficult to check within the rule for the required matches,
> however I'm not so sure this is really the way to go since it precludes
> optimizations that spawn over multiple rules, f.i. checking for TCP before
> a chain jump and have only TCP-specific rules in that chain.

Good point.

> I'm tending towards simply ignoring non-TCP packets at runtime.

Eric, please let me know if you want to address all of this in your icmp
code patch series, or if you would like me to handle this.

If you want me to handle it I will resubmit this patch with the issues
pointed out by Patrick resolved, plus a small kernel patch to not xmit
reset for non-tcp packets.

Otherwise I'll just mark this patch as "rejected" in patchwork.

> >  %token _REJECT			"reject"
> > +%token REJECT_RESET		"reset"
> > +%token REJECT_UNREACH		"unreach"
> 
> Please don't use expression/statement-specific token names. We're simply
> dealing with tokens, how they're used depends on the remaining grammar.

Makes sense.

Thanks for your help Patrick.