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

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

[Vasily Averin]

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))
-- 
1.7.5.4

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

[Vasily Averin]

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

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

[Florian Westphal]

Vasily Averin <vvs@parallels.com> wrote:
> Please do not apply my patch, probably it breaks processing of VLAN packets.

Why would it break VLAN?
In fact, the same dicussion came up couple of days back and I think the
nfct test is wrong.  There is no guarantee that skb->nfct == NULL means
that packet was not defragmented via nf_defrag (e.g. rror in l4 protocol
tracker, nf_defrag_ipv4 loaded but no nf_conntrack_ipv4)

For ipv6 its even worse since we toss all ipv6 defragmented packets...

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

[Vasily Averin]

On 04/24/2014 08:32 PM, Florian Westphal wrote:
> Vasily Averin <vvs@parallels.com> wrote:
>> Please do not apply my patch, probably it breaks processing of VLAN packets.
> 
> Why would it break VLAN?
Sorry, seems it was false alert.

We got report about problem on RHEL6-based OpenVZ kernel:
large UDP and ICMP packets was dropped on bridge without incrementing of any failcounters.
Connection tracking was disabled on this node , nf_conntrack module was unloaded
Ftrace pointed that it was happen because nfct check.

I've unloaded nf_conntrack module and problem was reproduced again
After removing nfct check problem went away.

The check was added by patch that fixed processing of VLAN packets.
I did not tested this scenario and therefore asked to delay processing of my patch,
to investigate this case more carefully.

IS_VLAN_IP() check on RHEL6 kernels seems is not depend on nfct check,
in current mainline we do not have this code at all, 
therefore removing of nfct check should not affect this case too.

Therefore I believe that my patch is still correct, however now I think we also need 
to remove #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) in br_nf_dev_queue_xmit().

> In fact, the same dicussion came up couple of days back and I think the
> nfct test is wrong.  There is no guarantee that skb->nfct == NULL means
> that packet was not defragmented via nf_defrag (e.g. rror in l4 protocol
> tracker, nf_defrag_ipv4 loaded but no nf_conntrack_ipv4)

Thank you for explanation,
In my case ftrace also shows that packets was defragmented in ipv4_conntrack_defrag().
However this module does not depend on NF_CONNTRACK_IPV4.

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

[Florian Westphal]

Vasily Averin <vvs@parallels.com> wrote:
> We got report about problem on RHEL6-based OpenVZ kernel:
> large UDP and ICMP packets was dropped on bridge without incrementing of any failcounters.
> Connection tracking was disabled on this node , nf_conntrack module was unloaded
> Ftrace pointed that it was happen because nfct check.

Right.  If you unload the conntrack module this bug triggers
since nf_defrag_ipv4 will still defragment ipv4 packets (and
thus needs refragmentation).

> Therefore I believe that my patch is still correct, however now I think we also need 
> to remove #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) in br_nf_dev_queue_xmit().

I don't think so, DEFRAG_IPV4 is dependency glue, so it shouldn't be
possible to build kernel with CONNTRACK_IPV4=n and DEFRAG_IPV4=(m|y).

Could you please formally submit your patch for inclusion in nf.git?
For some reason I don't see your patch in
http://patchwork.ozlabs.org/project/netfilter-devel/list/

Thanks!

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

[Vasily Averin]

>> Therefore I believe that my patch is still correct, however now I think we also need 
>> to remove #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) in br_nf_dev_queue_xmit().
> 
> I don't think so, DEFRAG_IPV4 is dependency glue, so it shouldn't be
> possible to build kernel with CONNTRACK_IPV4=n and DEFRAG_IPV4=(m|y).

Could you please explain, why this #ifdef is required?

I'm going to remove this #ifdef because it was added together with nfct check.

Also I believe you are wrong with dependencies:
NF_CONNTRACK_IPV4 forces NF_DEFRAG_IPV4, not vice versa

net/ipv4/netfilter/Kconfig
config NF_DEFRAG_IPV4
        tristate
        default n

config NF_CONNTRACK_IPV4
        tristate "IPv4 connection tracking support (required for NAT)"
        depends on NF_CONNTRACK
        default m if NETFILTER_ADVANCED=n
        select NF_DEFRAG_IPV4

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

[Pablo Neira Ayuso]

On Mon, Apr 28, 2014 at 06:25:22PM +0400, Vasily Averin wrote:
> >> Therefore I believe that my patch is still correct, however now I think we also need 
> >> to remove #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) in br_nf_dev_queue_xmit().
> > 
> > I don't think so, DEFRAG_IPV4 is dependency glue, so it shouldn't be
> > possible to build kernel with CONNTRACK_IPV4=n and DEFRAG_IPV4=(m|y).
> 
> Could you please explain, why this #ifdef is required?
> 
> I'm going to remove this #ifdef because it was added together with nfct check.
> 
> Also I believe you are wrong with dependencies:
> NF_CONNTRACK_IPV4 forces NF_DEFRAG_IPV4, not vice versa

That's fine. Basically nf_conntrack *always* requires defragmentation.
But defragmentation is also required by tproxy, which doesn't force
you to have nf_conntrack.

> net/ipv4/netfilter/Kconfig
> config NF_DEFRAG_IPV4
>         tristate
>         default n
> 
> config NF_CONNTRACK_IPV4
>         tristate "IPv4 connection tracking support (required for NAT)"
>         depends on NF_CONNTRACK
>         default m if NETFILTER_ADVANCED=n
>         select NF_DEFRAG_IPV4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vasily Averin]

On 04/28/2014 06:25 PM, Vasily Averin wrote:
>>> Therefore I believe that my patch is still correct, however now I think we also need 
>>> to remove #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) in br_nf_dev_queue_xmit().
>>
>> I don't think so, DEFRAG_IPV4 is dependency glue, so it shouldn't be
>> possible to build kernel with CONNTRACK_IPV4=n and DEFRAG_IPV4=(m|y).
> 
> Also I believe you are wrong with dependencies:
> NF_CONNTRACK_IPV4 forces NF_DEFRAG_IPV4, not vice versa

If you do not want to remove this #ifdef at all
how do you like an idea to replace NF_CONNTRACK_IPV4 to NF_DEFRAG_IPV4?

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

[Patrick McHardy]

On Sun, Apr 20, 2014 at 09:33:41AM +0400, Vasily Averin wrote:
> 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?

It doesn't require connection tracking, but connection tracking is the
only reason why we should fragment here since connection tracking does
defragmentation.

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

A bridge should not fragment packets. This is only done to counter the
effects of connection tracking, hence we only do it if connection tracking
is enabled.

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