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

Re: [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: [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: [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: [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: [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: [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: [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?

[PATCH] bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

[Vasily Averin]

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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined packet back,
and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters

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: [PATCH] bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

[Pablo Neira Ayuso]

On Wed, Apr 30, 2014 at 12:54:50PM +0400, Vasily Averin wrote:
> 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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined packet back,
> and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters

Patrick already mentioned that bridges should not defragment unless
conntrack is enabled.

Please, see: http://marc.info/?l=netfilter-devel&m=139878065822267&w=2

I think we have to consider some alternative way to fix what you
report.

Re: [PATCH] bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Apr 30, 2014 at 12:54:50PM +0400, Vasily Averin wrote:
> > 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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined packet back,
> > and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters
> 
> Patrick already mentioned that bridges should not defragment unless
> conntrack is enabled.
> 
> Please, see: http://marc.info/?l=netfilter-devel&m=139878065822267&w=2
> 
> I think we have to consider some alternative way to fix what you
> report.

Could you explain how br_nf_dev_queue_xmit could encouter
an IP packet that doesn't fit link MTU without netfilter
defrag?

Because I do not see any way of this happening, all bridge
ports must have the same MTU?

Re: [PATCH] bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

[Pablo Neira Ayuso]

On Wed, Apr 30, 2014 at 12:54:50PM +0400, Vasily Averin wrote:
> 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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined packet back,
> and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters

If no further objections, I'll push this original patch appending this comment
to the description:

[ It seems the only way to hit the ip_fragment code in the bridge xmit
  path is to have a fragment list whose reassembled fragments go over
  the mtu. This only happens if nf_defrag is enabled. Thanks to
  Florian Westphal for providing feedback to clarify this. ]

Re: [PATCH] bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

[Vasily Averin]

On 05/04/2014 04:54 PM, Pablo Neira Ayuso wrote:
> On Wed, Apr 30, 2014 at 12:54:50PM +0400, Vasily Averin wrote:
>> 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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined packet back,
>> and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters
> 
> If no further objections, I'll push this original patch appending this comment
> to the description:
> 
> [ It seems the only way to hit the ip_fragment code in the bridge xmit
>   path is to have a fragment list whose reassembled fragments go over
>   the mtu. This only happens if nf_defrag is enabled. Thanks to
>   Florian Westphal for providing feedback to clarify this. ]

I have not objections, however I still do not understand why #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) 
is required in br_dev_queue_push_xmit()?

If ipv4 defragmentation is required not only for conntracks but for TPROXY target and xt_socket match
I think we need to use NF_DEFRAG_IPV4 instead.

Re: [PATCH] bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

[Pablo Neira Ayuso]

On Sun, May 04, 2014 at 11:04:29PM +0400, Vasily Averin wrote:
> On 05/04/2014 04:54 PM, Pablo Neira Ayuso wrote:
> > On Wed, Apr 30, 2014 at 12:54:50PM +0400, Vasily Averin wrote:
> >> 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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined packet back,
> >> and therefore it is dropped in br_dev_queue_push_xmit without incrementing of any failcounters
> > 
> > If no further objections, I'll push this original patch appending this comment
> > to the description:
> > 
> > [ It seems the only way to hit the ip_fragment code in the bridge xmit
> >   path is to have a fragment list whose reassembled fragments go over
> >   the mtu. This only happens if nf_defrag is enabled. Thanks to
> >   Florian Westphal for providing feedback to clarify this. ]
> 
> I have not objections, however I still do not understand why #if
> IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) is required in
> br_dev_queue_push_xmit()?
> 
> If ipv4 defragmentation is required not only for conntracks but for
> TPROXY target and xt_socket match I think we need to use
> NF_DEFRAG_IPV4 instead.

Before your patch, this was checking for skb->nfct which is defined by

#if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)

in include/linux/skbuff.h.

But after removing that skb->nfct check, we can safely change it to
CONFIG_NF_DEFRAG_IPV4.

You can send me a new patch version including this change.

[PATCH v2] bridge: superfluous skb->nfct check in br_nf_dev_queue_xmit

[Vasily Averin]

[v2: #ifdef is changed from NF_CONNTRACK_IPV4 to NF_DEFRAG_IPV4]

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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined
packet back, and therefore it is dropped in br_dev_queue_push_xmit without
incrementing of any failcounters

It seems the only way to hit the ip_fragment code in the bridge xmit
path is to have a fragment list whose reassembled fragments go over
the mtu. This only happens if nf_defrag is enabled. Thanks to
Florian Westphal for providing feedback to clarify this.

Defragmentation ipv4 is required not only in conntracks but at least in
TPROXY target and socket match, therefore #ifdef is changed from
NF_CONNTRACK_IPV4 to NF_DEFRAG_IPV4

Signed-off-by: Vasily Averin <vvs@openvz.org>
---
 net/bridge/br_netfilter.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 80e1b0f..2acf7fa 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -859,12 +859,12 @@ static unsigned int br_nf_forward_arp(const struct nf_hook_ops *ops,
 	return NF_STOLEN;
 }
 
-#if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4)
+#if IS_ENABLED(CONFIG_NF_DEFRAG_IPV4)
 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: [PATCH v2] bridge: superfluous skb->nfct check in br_nf_dev_queue_xmit

[Pablo Neira Ayuso]

On Mon, May 05, 2014 at 12:17:48AM +0400, Vasily Averin wrote:
> [v2: #ifdef is changed from NF_CONNTRACK_IPV4 to NF_DEFRAG_IPV4]
> 
> 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 nfct check in br_nf_dev_queue_xmit does not allow re-fragment combined
> packet back, and therefore it is dropped in br_dev_queue_push_xmit without
> incrementing of any failcounters
> 
> It seems the only way to hit the ip_fragment code in the bridge xmit
> path is to have a fragment list whose reassembled fragments go over
> the mtu. This only happens if nf_defrag is enabled. Thanks to
> Florian Westphal for providing feedback to clarify this.
> 
> Defragmentation ipv4 is required not only in conntracks but at least in
> TPROXY target and socket match, therefore #ifdef is changed from
> NF_CONNTRACK_IPV4 to NF_DEFRAG_IPV4

Applied, thanks Vasily.

Re: [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))
> >