Re: [PATCH nf-next WIP] netfilter: nf_tables: Add SYNPROXY support

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Jun 17, 2019 at 09:49:43PM +0200, Fernando Fernandez Mancera wrote:
> Hi Pablo, comments below.
> 
> On 6/17/19 5:45 PM, Pablo Neira Ayuso wrote:
> > On Mon, Jun 17, 2019 at 12:32:35PM +0200, Fernando Fernandez Mancera wrote:
> >> Add SYNPROXY module support in nf_tables. It preserves the behaviour of the
> >> SYNPROXY target of iptables but structured in a different way to propose
> >> improvements in the future.
> >>
> >> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> >> ---
> >>  include/uapi/linux/netfilter/nf_SYNPROXY.h |   4 +
> >>  include/uapi/linux/netfilter/nf_tables.h   |  16 +
> >>  net/netfilter/Kconfig                      |  11 +
> >>  net/netfilter/Makefile                     |   1 +
> >>  net/netfilter/nft_synproxy.c               | 328 +++++++++++++++++++++
> >>  5 files changed, 360 insertions(+)
> >>  create mode 100644 net/netfilter/nft_synproxy.c
> >>
> [...]
> >> +
> >> +static void nft_synproxy_eval(const struct nft_expr *expr,
> >> +			      struct nft_regs *regs,
> >> +			      const struct nft_pktinfo *pkt)
> >> +{
> > 
> > You have to check if this is TCP traffic in first place, otherwise UDP
> > packets may enter this path :-).
> > 
> >> +	switch (nft_pf(pkt)) {
> >> +	case NFPROTO_IPV4:
> >> +		nft_synproxy_eval_v4(expr, regs, pkt);
> >> +		return;
> >> +#if IS_ENABLED(CONFIG_NF_TABLES_IPV6)
> >> +	case NFPROTO_IPV6:
> >> +		nft_synproxy_eval_v6(expr, regs, pkt);
> >> +		return;
> >> +#endif
> > 
> > Please, use skb->protocol instead of nft_pf(), I would like we can use
> > nft_synproxy from NFPROTO_NETDEV (ingress) and NFPROTO_BRIDGE families
> > too.
> > 
> 
> If I use skb->protocol no packet enters in the path. What do you
> recommend me? Other than that, the rest of the suggestions are done and
> it has been tested and it worked as expected. Thanks :-)

skb->protocol uses big endian representation, you have to check for:

        switch (skb->protocol) {
        case htons(ETH_P_IP):
                ...
                break;
        case htons(ETH_P_IPV6):
                ...
                break;
        }