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

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

[Vasily Averin]

Currently bridge silently drops defragmented ipv4 packets if nf_conntrack module
is not loaded on the node. However ipv4 fragmentation and defragmentation
does not require enabled connection tracking.

Removing superfluous skb->nfct check allows to re-fragment previously
defragmented packets and give them chance to reach destination side.

v2: removed #if IS_ENABLED(CONFIG_NF_CONNTRACK_IPV4) added because
of using of nfct check

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

diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 80e1b0f..d50c154 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -859,12 +859,11 @@ static unsigned int br_nf_forward_arp(const struct nf_hook_ops *ops,
 	return NF_STOLEN;
 }
 
-#if IS_ENABLED(CONFIG_NF_CONNTRACK_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))
@@ -876,12 +875,6 @@ static int br_nf_dev_queue_xmit(struct sk_buff *skb)
 
 	return ret;
 }
-#else
-static int br_nf_dev_queue_xmit(struct sk_buff *skb)
-{
-        return br_dev_queue_push_xmit(skb);
-}
-#endif
 
 /* PF_BRIDGE/POST_ROUTING ********************************************/
 static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
-- 
1.7.5.4

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.