On Wed, Jun 20, 2018 at 01:29:51PM +0200, Florian Westphal wrote:
> Máté Eckl <ecklm94@xxxxxxxxx> wrote:
> > This patch is built on the commit not applied yet with the title:
> > evaluate: Detect address family in inet context
>
> You can add this ...
>
> > Example ruleset:
> > table inet x {
> > chain y {
> > type filter hook prerouting priority -150; policy accept;
> > socket transparent 1 mark set 0x00000001 accept
> > tproxy mark set 0x00000001 counter packets 611 bytes 46181
> > meta l4proto tcp tproxy to :50080 mark set 0x00000001 counter packets 202 bytes 13600
> > }
> > }
> >
> > Signed-off-by: Máté Eckl <ecklm94@xxxxxxxxx>
> > ---
>
> ... here, as its removed automatically when using 'git am'.
I don't understand. You mean that I should not include examples in commit
messages?
>
> The ruleset above seems a bit strange because it uses two 'tproxy'
> ruleset.
Yeah, they were just sintactic examples, I will replace them with a meaningfull
one.
> Also, this patch should include a man page entry to document
> socket/tproxy.
As I didn't add man page for socket matching either, I thought that it could be
a separate commit, once the functionality and the code is accepted.
>
> A patch to add test cases would also be nice (can be as followup
> after patch has been applied of course).
>
I sent that.
> > + if (stmt->tproxy.addr) {
> > + addr_reg = get_register(ctx, NULL);
> > + registers++;
> > +
> > + if (stmt->tproxy.addr->ops->type != EXPR_RANGE) {
> > + netlink_gen_expr(ctx, stmt->tproxy.addr, addr_reg);
> > + netlink_put_register(nle, NFTNL_EXPR_TPROXY_REG_ADDR,
> > + addr_reg);
> > + }
>
> This looks weird. Why this != EXPR_RANGE check?
> It should never happen, because in evaluate.c you do this for
> ports, so why not also reject the unwanted expression type there?
Yes, you are right, I will remove this check.
>
> > if (stmt->tproxy.port != NULL) {
> > if (stmt->tproxy.port->ops->type == EXPR_RANGE)
> > return -EOPNOTSUPP;
>
> Might be a good idea to use stmt_error() though to indicate that this
> doesn't work.
>
> > + if (stmt->tproxy.port) {
> > + port_reg = get_register(ctx, NULL);
> > + registers++;
> > +
> > + if (stmt->tproxy.port->ops->type != EXPR_RANGE) {
> > + netlink_gen_expr(ctx, stmt->tproxy.port, port_reg);
> > + netlink_put_register(nle, nftnl_reg_port, port_reg);
> > + }
>
> Likewise, this is always true, right?
Yes, removed along with the one above.
>
> > +tproxy_stmt : TPROXY
> > + {
> > + $$ = tproxy_stmt_alloc(&@$);
> > + }
>
> I wonder what the use case for plain TPROXY is.
> Do we need to support that? How is it useful?
>
> We should probably enforce at least the port number, no?
>
> If not, it would be good to illustrate this with an example
> in the nft man page at least.
I think, it is a useful one. If I want to make proxy working only for web
traffic this simple tproxy statement is sufficient:
tcp dport 80 tproxy
if the proxy software is listening on port 80.
This would send packets to the proxy software as the original destination port
is used during socket lookup.
I can include this example in the man page.
This use-case seems quite meaningful to me.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
