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

[Vasily Averin <vvs@parallels.com>]

On 05/03/2014 02:55 AM, Florian Westphal wrote:
> Vasily Averin <vvs@parallels.com> wrote:
>> 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.

Thanks, I've submitted separated patch to fix it.

>> ---[patch rfc]---
>> This patch adds per network namespace flag to manage ipv4 defragmentation
>> in bridge.
> 
> 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 ]

I think you are right, seems skb->nfct check will be extra anyway.

However my patch fixes wrong processing packets in bridge in case disabled conntracks.
Probably Patrick can elaborate in more details why this is bad.

I have noticed only that currently bridge without conntracks can silently merge 2 small
packets if their common size < mtu. Probably it can be unexpected for processing on destination side.
However if we enable connection tracking -- we'll get the same situation again.

> 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?

I'm sorry, you are right, changes of nf_conntrack_ipv4 pernet_operations was lost 
I'll resend resend updated patch soon.