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

[Fernando Fernandez Mancera <ffmancera@riseup.net>]

On 6/17/19 11:55 PM, Pablo Neira Ayuso wrote:
> 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;
>         }
> 


Oh, I didn't know that. A patch series including tests and documentation
it is going to be ready soon if everything seem fine to you. After this,
I think we can implement some improvements. Thanks :-)