Máté Eckl <ecklm94@xxxxxxxxx> wrote:
> There are some changes compared to the iptables implementation:
> - tproxy statement is not terminal here
> - no transport protocol criterion is necessary to set target ip address
> + const struct nft_tproxy *priv = nft_expr_priv(expr);
> + struct sk_buff *skb = pkt->skb;
> + struct sock *sk = skb->sk;
> + const struct iphdr *iph = ip_hdr(skb);
> + struct udphdr _hdr, *hp;
> + __be32 taddr = 0;
> + __be16 tport = 0;
> +
> + hp = skb_header_pointer(skb, ip_hdrlen(skb), sizeof(_hdr), &_hdr);
> + if (!hp)
> + regs->verdict.code = NFT_BREAK;
This is missing needed 'return'.
> + /* UDP has no TCP_TIME_WAIT state, so we never enter here */
> + if (sk && sk->sk_state == TCP_TIME_WAIT)
> + /* reopening a TIME_WAIT connection needs special handling */
> + sk = nf_tproxy_handle_time_wait4(nft_net(pkt), skb, taddr, tport, sk);
> + else if (!sk)
> + /* no, there's no established connection, check if
> + * there's a listener on the redirected addr/port */
> + sk = nf_tproxy_get_sock_v4(nft_net(pkt), skb, hp, iph->protocol,
> + iph->saddr, taddr,
> + hp->source, tport,
> + skb->dev, NF_TPROXY_LOOKUP_LISTENER);
This looks like its subtly broken, inherited from xt_TPROXY.
Above skb_header_pointer uses sizeof(udphdr) only, but nf_tproxy_get_sock_v4
assumes it gets tcphdr (it checks th->doff, and that might be garbage).
So I suggest to remove 'hp' argument from nf_tproxy_get_sock_v4/6 and repeat
the skb_header_pointer() call there, using struct tcphdr size as backend
storage for TCP case.
This will need to be a extra patch vs. nf.git tree.
> + if (sk && nf_tproxy_sk_is_transparent(sk)) {
> + nf_tproxy_assign_sock(skb, sk);
> + }
No need for extra { }, see scripts/checkpatch.pl (no need to follow
every advice that script provides of course, decide for yourself).
> + /* NOTE: assign_sock consumes our sk reference */
> + if (sk && nf_tproxy_sk_is_transparent(sk)) {
> + nf_tproxy_assign_sock(skb, sk);
> + return;
> + }
> +
> + regs->verdict.code = NFT_BREAK;
> +}
So ipv4 and ipv6 behavce differenty?
Why does one set BREAK but not the other?
I *guess* its best to BREAK in case sk wasn't assigned for both.
> + plen = sizeof(u16);
> + if (tb[NFTA_TPROXY_REG_PORT]) {
> + priv->sreg_port = nft_parse_register(tb[NFTA_TPROXY_REG_PORT]);
> + err = nft_validate_register_load(priv->sreg_port, plen);
I think you can remove plen and just pass sizeof(__be16).
--
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
