superfluous skb->nfct check in br_nf_dev_queue_xmit

PATCH Superseded Vasily Averin 4 participants 17 messages Apr 17, 2014 – May 5, 2014

Subsystems (3)

Networking netfilter Ethernet bridge

lore

Share & tools

lore

Vasily Averin

author

2014-04-17 11:15 UTC #0 lore raw

PATCH 1/1 superfluous skb->nfct check in br_nf_dev_queue_xmit

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 <[email protected]>

---
 net/bridge/br_netfilter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/bridge/br_netfilter.c +1 -1

index 80e1b0f..6a8407c 100644
@@ -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))

Signature · 1 line

1.7.5.4

Vasily Averin

author

2014-04-20 05:33 UTC #1 lore raw

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

Quoted 29 lines · 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 <[email protected]>
---
 net/bridge/br_netfilter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/bridge/br_netfilter.c +1 -1
index 80e1b0f..6a8407c 100644
@@ -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))

Florian Westphal

maintainer

2014-04-24 16:32 UTC #2 lore raw

Quoted 1 line · Vasily Averin

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

Vasily Averin

author

2014-04-28 12:37 UTC #3 lore raw

Quoted 4 lines · Florian Westphal

Quoted 1 line · Vasily Averin
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().

Quoted 4 lines

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.

Florian Westphal

maintainer

2014-04-28 13:16 UTC #4 lore raw

Quoted 4 lines · Vasily Averin

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

Quoted 2 lines

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!

Vasily Averin

author

2014-04-28 14:25 UTC #5 lore raw

Quoted 5 lines

Quoted 2 lines
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

Pablo Neira Ayuso

maintainer

2014-04-28 14:32 UTC #6 lore raw

Quoted 6 lines · Vasily Averin

Quoted 5 lines
Quoted 2 lines
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).

Quoted 6 lines

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.

Quoted 16 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Vasily Averin

author

2014-04-30 09:06 UTC #7 lore raw

In reply to Vasily Averin

Quoted 8 lines · Vasily Averin

Quoted 5 lines
Quoted 2 lines
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?

Vasily Averin

author

2014-04-30 08:56 UTC #8 lore raw

PATCH 1/1 bridge: Superfluous skb->nfct check in br_nf_dev_queue_xmit

In reply to Florian Westphal

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 <[email protected]>

---
 net/bridge/br_netfilter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/bridge/br_netfilter.c +1 -1

index 80e1b0f..6a8407c 100644
@@ -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))

Signature · 1 line

1.7.5.4

Pablo Neira Ayuso

maintainer

2014-04-30 09:49 UTC #9 lore raw

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

Quoted 5 lines · 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

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.

Florian Westphal

maintainer

2014-04-30 10:02 UTC #10 lore raw

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

Quoted 7 lines · Pablo Neira Ayuso

Quoted 5 lines · 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

Quoted 7 lines

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?

Pablo Neira Ayuso

maintainer

2014-05-04 12:54 UTC #11 lore raw

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

In reply to Vasily Averin

Quoted 5 lines · 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

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

Vasily Averin

author

2014-05-04 19:06 UTC #12 lore raw

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

Quoted 7 lines · Pablo Neira Ayuso

Quoted 5 lines · 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

Quoted 7 lines

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.

Pablo Neira Ayuso

maintainer

2014-05-04 19:25 UTC #13 lore raw

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

Quoted 16 lines · Vasily Averin

Quoted 7 lines · Pablo Neira Ayuso
Quoted 5 lines · 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
Quoted 7 lines
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. ]

Quoted 7 lines

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.

Vasily Averin

author

2014-05-04 20:19 UTC #14 lore raw

PATCH v2 1/1 bridge: superfluous skb->nfct check in br_nf_dev_queue_xmit

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

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 <[email protected]>

---
 net/bridge/br_netfilter.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

net/bridge/br_netfilter.c +2 -2

index 80e1b0f..2acf7fa 100644
@@ -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))

Signature · 1 line

1.7.5.4

Pablo Neira Ayuso

maintainer Applied

2014-05-05 14:08 UTC #15 lore raw

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

Quoted 9 lines · 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

Quoted 8 lines

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.

Patrick McHardy

2014-04-29 14:10 UTC #16 lore raw

In reply to Vasily Averin

Quoted 5 lines · 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?

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

Quoted 4 lines

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.

Quoted 41 lines

Quoted 2 lines
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
Quoted 29 lines · 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 <[email protected]>
---
 net/bridge/br_netfilter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
net/bridge/br_netfilter.c +1 -1
index 80e1b0f..6a8407c 100644
@@ -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))