[PATCH nf-next] netfilter: bridge: Fix non-untagged fragment packet

[PATCH nf-next] netfilter: bridge: Fix non-untagged fragment packet

[wenxu]

From: wenxu <wenxu@ucloud.cn>

ip netns exec ns1 ip a a dev eth0 10.0.0.7/24
ip netns exec ns2 ip link a link eth0 name vlan type vlan id 200
ip netns exec ns2 ip a a dev vlan 10.0.0.8/24

ip l add dev br0 type bridge vlan_filtering 1
brctl addif br0 veth1
brctl addif br0 veth2

bridge vlan add dev veth1 vid 200 pvid untagged
bridge vlan add dev veth2 vid 200

A two fragment packets send from ns2 contained with vlan tag 200.
In the bridge conntrack, packet will defrag to one skb with fraglist.
When the packet forward to ns1 through veth1, the first skb vlan tag
will be cleared for "untagged" flags. But the vlan tag in the second
skb still tagged, which lead the second fragment send with tag 200 to
ns1.
So if the first fragment packet don't contain vlan tag, all of the
remain should not contain vlan tag..

Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
Signed-off-by: wenxu <wenxu@ucloud.cn>
---
 net/bridge/netfilter/nf_conntrack_bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
index b675cd7..4f5444d 100644
--- a/net/bridge/netfilter/nf_conntrack_bridge.c
+++ b/net/bridge/netfilter/nf_conntrack_bridge.c
@@ -331,6 +331,8 @@ static int nf_ct_bridge_frag_restore(struct sk_buff *skb,
 	}
 	if (data->vlan_present)
 		__vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci);
+	else if (skb_vlan_tag_present(skb))
+		__vlan_hwaccel_clear_tag(skb);
 
 	skb_copy_to_linear_data_offset(skb, -ETH_HLEN, data->mac, ETH_HLEN);
 	skb_reset_mac_header(skb);
-- 
1.8.3.1

Re: [PATCH nf-next] netfilter: bridge: Fix non-untagged fragment packet

[Pablo Neira Ayuso]

On Wed, Jun 19, 2019 at 10:35:07PM +0800, wenxu@ucloud.cn wrote:
[...]
> So if the first fragment packet don't contain vlan tag, all of the
> remain should not contain vlan tag..

If I understand correctly, the problem is this:

* First fragment comes with no vlan tag.
* Second fragment comes with vlan tag.

If you have a vlan setup, you have to use ct zone to map the vlan id
to the corresponding ct zone.

nf_ct_br_defrag4() calls:

        err = ip_defrag(state->net, skb,
                                IP_DEFRAG_CONNTRACK_BRIDGE_IN + zone_id);

if ct zones are used, first fragment will go to defrag queue
IP_DEFRAG_CONNTRACK_BRIDGE_IN + 0, while second fragment will go to
IP_DEFRAG_CONNTRACK_BRIDGE_IN + zone_id.

So they will go to different defrag queues.

> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
>  net/bridge/netfilter/nf_conntrack_bridge.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c b/net/bridge/netfilter/nf_conntrack_bridge.c
> index b675cd7..4f5444d 100644
> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
> @@ -331,6 +331,8 @@ static int nf_ct_bridge_frag_restore(struct sk_buff *skb,
>  	}
>  	if (data->vlan_present)
>  		__vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci);
> +	else if (skb_vlan_tag_present(skb))
> +		__vlan_hwaccel_clear_tag(skb);
>  
>  	skb_copy_to_linear_data_offset(skb, -ETH_HLEN, data->mac, ETH_HLEN);
>  	skb_reset_mac_header(skb);
> -- 
> 1.8.3.1
> 

Re: [PATCH nf-next] netfilter: bridge: Fix non-untagged fragment packet

[wenxu]

在 2019/6/20 18:48, Pablo Neira Ayuso 写道:
> On Wed, Jun 19, 2019 at 10:35:07PM +0800, wenxu@ucloud.cn wrote:
> [...]
>> So if the first fragment packet don't contain vlan tag, all of the
>> remain should not contain vlan tag..
> If I understand correctly, the problem is this:
>
> * First fragment comes with no vlan tag.
> * Second fragment comes with vlan tag.
>
> If you have a vlan setup, you have to use ct zone to map the vlan id
> to the corresponding ct zone.
>
> nf_ct_br_defrag4() calls:
>
>         err = ip_defrag(state->net, skb,
>                                 IP_DEFRAG_CONNTRACK_BRIDGE_IN + zone_id);
>
> if ct zones are used, first fragment will go to defrag queue
> IP_DEFRAG_CONNTRACK_BRIDGE_IN + 0, while second fragment will go to
> IP_DEFRAG_CONNTRACK_BRIDGE_IN + zone_id.
>
> So they will go to different defrag queues.
>
It's not correct.

The problem is both the first and second fragment comes with vlan tag (It's make sense).

After the defrag(in fast mode), the two skb chains to a one skb.  When the packet send to the veth1 port which with flags "untagged". So the only the first skb clear the vlan tag, but the second one also contain vlan tag.  In the refrag which also in the fast mode only split the chian skb.  So it leads the first skb with no vlan tag which is correct. But the second skb wit vlan tag which is not correct

Re: [PATCH nf-next] netfilter: bridge: Fix non-untagged fragment packet

[Florian Westphal]

wenxu@ucloud.cn <wenxu@ucloud.cn> wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> ip netns exec ns1 ip a a dev eth0 10.0.0.7/24
> ip netns exec ns2 ip link a link eth0 name vlan type vlan id 200
> ip netns exec ns2 ip a a dev vlan 10.0.0.8/24
> 
> ip l add dev br0 type bridge vlan_filtering 1
> brctl addif br0 veth1
> brctl addif br0 veth2
> 
> bridge vlan add dev veth1 vid 200 pvid untagged
> bridge vlan add dev veth2 vid 200
> 
> A two fragment packets send from ns2 contained with vlan tag 200.
> In the bridge conntrack, packet will defrag to one skb with fraglist.
> When the packet forward to ns1 through veth1, the first skb vlan tag
> will be cleared for "untagged" flags. But the vlan tag in the second
> skb still tagged, which lead the second fragment send with tag 200 to
> ns1.
> So if the first fragment packet don't contain vlan tag, all of the
> remain should not contain vlan tag..
> 
> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
> Signed-off-by: wenxu <wenxu@ucloud.cn>

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH nf-next] netfilter: bridge: Fix non-untagged fragment packet

[Pablo Neira Ayuso]

On Wed, Jun 19, 2019 at 10:35:07PM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
> 
> ip netns exec ns1 ip a a dev eth0 10.0.0.7/24
> ip netns exec ns2 ip link a link eth0 name vlan type vlan id 200
> ip netns exec ns2 ip a a dev vlan 10.0.0.8/24
> 
> ip l add dev br0 type bridge vlan_filtering 1
> brctl addif br0 veth1
> brctl addif br0 veth2
> 
> bridge vlan add dev veth1 vid 200 pvid untagged
> bridge vlan add dev veth2 vid 200
> 
> A two fragment packets send from ns2 contained with vlan tag 200.
> In the bridge conntrack, packet will defrag to one skb with fraglist.
> When the packet forward to ns1 through veth1, the first skb vlan tag
> will be cleared for "untagged" flags. But the vlan tag in the second
> skb still tagged, which lead the second fragment send with tag 200 to
> ns1.
> So if the first fragment packet don't contain vlan tag, all of the
> remain should not contain vlan tag..

Applied, thanks for explaining.