Re: [Bridge] [PATCH 1/1] superfluous skb->nfct check in br_nf_dev_queue_xmit

[Vasily Averin <vvs@parallels.com>]

Please do not apply my patch, probably it breaks processing of VLAN packets.

Dear Patrick,
could you please explain why fragmentation of packets requires enabled
connection tracking?

During old patch discussion you told "everything related to fragmenting
is only needed with NF_CONNTRACK". However before adding (skb->nfct) check
bridge worked well with fragments, and I cannot understand what exactly in 
ip_fragment should not work with disabled connection trackng.

From my point of view its better to drop packets in ip_fragment(), 
where failcounters accounts these events instead silent dropping
in br_dev_queu_push_xmit().

So could you please explain, why we need to have skb->nfct check
in br_nf_dev_queue_xmit()?

Thank you,
	Vasily Averin

On 04/17/2014 03:15 PM, Vasily Averin wrote:
> skb->nfct check in br_nf_dev_queue_xmit() does not work if conntracks
> are not loaded on the node. This check does not allow to fragment skb
> combined from incoming fragments, as results this skb will be dropped
> silently in br_dev_queue_push_xmit()
> 
> This check was added in commit c197facc8ea08062f8f949aade6a33649ee06771
> netfilter: bridge: allow fragmentation of VLAN packets traversing a bridge
> 
> I believe this check is superfluous and should be removed.
> 
> Signed-off-by: Vasily Averin <vvs@openvz.org>
> ---
>  net/bridge/br_netfilter.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index 80e1b0f..6a8407c 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -864,7 +864,7 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
>  {
>  	int ret;
>  
> -	if (skb->nfct != NULL && skb->protocol == htons(ETH_P_IP) &&
> +	if (skb->protocol == htons(ETH_P_IP) &&
>  	    skb->len + nf_bridge_mtu_reduction(skb) > skb->dev->mtu &&
>  	    !skb_is_gso(skb)) {
>  		if (br_parse_ip_options(skb))
>