Re: [PATCH RFC] Bridge: do not defragment packets unless connection tracking is enabled

[Florian Westphal <fw@strlen.de>]

Vasily Averin <vvs@parallels.com> wrote:
> Also I have one more question -- about defrag user check.
> 
> In my patch I use
> if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) && 
>     (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN))
> because nf_ct_defrag_user can add zone.
> 
> I've found defrag user check in ip_expire() -- but it does not take
> account of zone.
> Is it a bug in ip_expire() or I missed something?

Looks like a bug to me.

> ---[patch rfc]---
> Currently bridge can silently drop ipv4 fragments.
> If node have loaded nf_defrag_ipv4 module but have no nf_conntrack_ipv4,
> br_nf_pre_routing defragments incoming ipv4 fragments, but skb->nfct check
> in br_nf_dev_queue_xmit does not allow to re-fragment combined packet back,
> and therefore it is dropped in br_dev_queue_push_xmit without incrementing
> of any failcounters.
> 
> According to Patrick McHardy, bridge should not defragment and fragment
> packets unless conntrack is enabled.
> 
> This patch adds per network namespace flag to manage ipv4 defragmentation
> in bridge.
> 
> Signed-off-by: Vasily Averin <vvs@openvz.org>

Are we sure this is required rather than just removing the skb->nfct
test in br_nf_dev_queue_xmit() and be done with it?

Because that seems a lot saner to me, I fail to see how

if (skb->protocol == htons(ETH_P_IP) &&
           skb->len + nf_bridge_mtu_reduction(skb) >
		    skb->dev->mtu && !skb_is_gso(skb)) {

Would evaluate as 'true' without nf_defrag_ipv4 module loaded.

[ its from br_nf_dev_queue_xmit function ]

>  struct nf_generic_net {
>  	struct nf_proto_net pn;
>  	unsigned int timeout;
> +	bool br_ipv4_defrag_disabled;
>  };

>  struct nf_tcp_net {
> diff --git a/net/ipv4/netfilter/nf_defrag_ipv4.c b/net/ipv4/netfilter/nf_defrag_ipv4.c
> index d807822..5f773d4 100644
> --- a/net/ipv4/netfilter/nf_defrag_ipv4.c
> +++ b/net/ipv4/netfilter/nf_defrag_ipv4.c
> @@ -87,6 +87,20 @@ static unsigned int ipv4_conntrack_defrag(const struct nf_hook_ops *ops,
>  		enum ip_defrag_users user =
>  			nf_ct_defrag_user(ops->hooknum, skb);
>  
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK) && defined (CONFIG_BRIDGE_NETFILTER)
> +		if ((user >= IP_DEFRAG_CONNTRACK_BRIDGE_IN) &&
> +		    (user <= __IP_DEFRAG_CONNTRACK_BRIDGE_IN)) {
> +#ifdef CONFIG_NET_NS
> +			struct net *net = skb->sk->sk_net;
> +#else
> +			struct net *net = &init_net;
> +#endif
> +			/* A bridge should not defragment and fragment packets. 
> +			   We only do it if connection tracking is enabled. */
> +			if (net->ct.nf_ct_proto.generic.br_ipv4_defrag_disabled)
> +				return NF_ACCEPT;
> +		}
> +#endif

I wonder who would be responsible to set br_ipv4_defrag_disabled to false
to enable conntracking on a bridge again?

Did i miss something in the patch?

Thanks.