* [PATCH] netfilter: bridge: unshare bridge info before change it
@ 2014-09-29 7:35 Gao feng
2014-11-04 0:45 ` Gao feng
0 siblings, 1 reply; 6+ messages in thread
From: Gao feng @ 2014-09-29 7:35 UTC (permalink / raw)
To: netfilter-devel; +Cc: pablo, Gao feng
Many packets may share the same bridge information,
we should unshare the bridge info before we change it,
otherwise other packets will go to PF_INET(6)/PRE_ROUTING
second time or the pkt_type of other packets will be
incorrect.
The problem occurs when we do nfqueue after br_nf_pre_routing
and before bf_nf_pre_routing_finish, if the packet is gso,
the new segs will share the same bridge info. and netfilter
may use skb_clone, this will cause many packets share the
same bridge info too.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
include/linux/netfilter_bridge.h | 54 ++++++++++++++++++++-
net/bridge/br_netfilter.c | 101 +++++++++++++++++++--------------------
2 files changed, 100 insertions(+), 55 deletions(-)
diff --git a/include/linux/netfilter_bridge.h b/include/linux/netfilter_bridge.h
index 8ab1c27..25cfeab 100644
--- a/include/linux/netfilter_bridge.h
+++ b/include/linux/netfilter_bridge.h
@@ -53,14 +53,64 @@ static inline unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb)
return 0;
}
+static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
+{
+ skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
+ if (likely(skb->nf_bridge))
+ atomic_set(&(skb->nf_bridge->use), 1);
+
+ return skb->nf_bridge;
+}
+
+static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
+{
+ struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+
+ if (atomic_read(&nf_bridge->use) > 1) {
+ struct nf_bridge_info *tmp = nf_bridge_alloc(skb);
+
+ if (tmp) {
+ memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info));
+ atomic_set(&tmp->use, 1);
+ }
+ nf_bridge_put(nf_bridge);
+ nf_bridge = tmp;
+ }
+ return nf_bridge;
+}
+
+static inline struct nf_bridge_info *
+nf_bridge_set_mask(struct sk_buff *skb, unsigned int mask)
+{
+ if (!nf_bridge_unshare(skb))
+ return NULL;
+
+ skb->nf_bridge->mask |= mask;
+ return skb->nf_bridge;
+}
+
+static inline struct nf_bridge_info *
+nf_bridge_change_mask(struct sk_buff *skb, unsigned int mask)
+{
+ if (!nf_bridge_unshare(skb))
+ return NULL;
+
+ skb->nf_bridge->mask ^= mask;
+ return skb->nf_bridge;
+}
+
int br_handle_frame_finish(struct sk_buff *skb);
/* Only used in br_device.c */
static inline int br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
{
- struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+ struct nf_bridge_info *nf_bridge;
skb_pull(skb, ETH_HLEN);
- nf_bridge->mask ^= BRNF_BRIDGED_DNAT;
+ nf_bridge = nf_bridge_change_mask(skb, BRNF_BRIDGED_DNAT);
+ if (nf_bridge == NULL) {
+ kfree_skb(skb);
+ return 0;
+ }
skb_copy_to_linear_data_offset(skb, -(ETH_HLEN-ETH_ALEN),
skb->nf_bridge->data, ETH_HLEN-ETH_ALEN);
skb->dev = nf_bridge->physindev;
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index a615264..eeca74e 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -187,32 +187,6 @@ static inline struct net_device *bridge_parent(const struct net_device *dev)
return port ? port->br->dev : NULL;
}
-static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
-{
- skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
- if (likely(skb->nf_bridge))
- atomic_set(&(skb->nf_bridge->use), 1);
-
- return skb->nf_bridge;
-}
-
-static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
-{
- struct nf_bridge_info *nf_bridge = skb->nf_bridge;
-
- if (atomic_read(&nf_bridge->use) > 1) {
- struct nf_bridge_info *tmp = nf_bridge_alloc(skb);
-
- if (tmp) {
- memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info));
- atomic_set(&tmp->use, 1);
- }
- nf_bridge_put(nf_bridge);
- nf_bridge = tmp;
- }
- return nf_bridge;
-}
-
static inline void nf_bridge_push_encap_header(struct sk_buff *skb)
{
unsigned int len = nf_bridge_encap_header_len(skb);
@@ -345,20 +319,25 @@ int nf_bridge_copy_header(struct sk_buff *skb)
* bridge PRE_ROUTING hook. */
static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
{
- struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+ struct nf_bridge_info *nf_bridge;
struct rtable *rt;
- if (nf_bridge->mask & BRNF_PKT_TYPE) {
+ if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
skb->pkt_type = PACKET_OTHERHOST;
- nf_bridge->mask ^= BRNF_PKT_TYPE;
+ nf_bridge = nf_bridge_change_mask(skb,
+ BRNF_PKT_TYPE | BRNF_NF_BRIDGE_PREROUTING);
+ } else {
+ nf_bridge = nf_bridge_change_mask(skb,
+ BRNF_NF_BRIDGE_PREROUTING);
}
- nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+
+ if (nf_bridge == NULL)
+ goto drop;
rt = bridge_parent_rtable(nf_bridge->physindev);
- if (!rt) {
- kfree_skb(skb);
- return 0;
- }
+ if (!rt)
+ goto drop;
+
skb_dst_set_noref(skb, &rt->dst);
skb->dev = nf_bridge->physindev;
@@ -366,8 +345,11 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
nf_bridge_push_encap_header(skb);
NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
br_handle_frame_finish, 1);
-
+out:
return 0;
+drop:
+ kfree_skb(skb);
+ goto out;
}
/* Obtain the correct destination MAC address, while preserving the original
@@ -387,7 +369,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
dst = skb_dst(skb);
neigh = dst_neigh_lookup_skb(dst, skb);
if (neigh) {
- int ret;
+ int ret = 0;
if (neigh->hh.hh_len) {
neigh_hh_bridge(&neigh->hh, skb);
@@ -403,8 +385,10 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
skb->nf_bridge->data,
ETH_HLEN-ETH_ALEN);
/* tell br_dev_xmit to continue with forwarding */
- nf_bridge->mask |= BRNF_BRIDGED_DNAT;
- ret = neigh->output(neigh, skb);
+ if (nf_bridge_set_mask(skb, BRNF_BRIDGED_DNAT) == NULL)
+ kfree_skb(skb);
+ else
+ ret = neigh->output(neigh, skb);
}
neigh_release(neigh);
return ret;
@@ -456,15 +440,24 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
{
struct net_device *dev = skb->dev;
struct iphdr *iph = ip_hdr(skb);
- struct nf_bridge_info *nf_bridge = skb->nf_bridge;
+ struct nf_bridge_info *nf_bridge;
struct rtable *rt;
int err;
- if (nf_bridge->mask & BRNF_PKT_TYPE) {
+ if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
skb->pkt_type = PACKET_OTHERHOST;
- nf_bridge->mask ^= BRNF_PKT_TYPE;
+ nf_bridge = nf_bridge_change_mask(skb,
+ BRNF_PKT_TYPE | BRNF_NF_BRIDGE_PREROUTING);
+ } else {
+ nf_bridge = nf_bridge_change_mask(skb,
+ BRNF_NF_BRIDGE_PREROUTING);
+ }
+
+ if (nf_bridge == NULL) {
+ kfree_skb(skb);
+ return 0;
}
- nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
+
if (dnat_took_place(skb)) {
if ((err = ip_route_input(skb, iph->daddr, iph->saddr, iph->tos, dev))) {
struct in_device *in_dev = __in_dev_get_rcu(dev);
@@ -750,7 +743,11 @@ static int br_nf_forward_finish(struct sk_buff *skb)
in = nf_bridge->physindev;
if (nf_bridge->mask & BRNF_PKT_TYPE) {
skb->pkt_type = PACKET_OTHERHOST;
- nf_bridge->mask ^= BRNF_PKT_TYPE;
+
+ if (!nf_bridge_change_mask(skb, BRNF_PKT_TYPE)) {
+ kfree_skb(skb);
+ return 0;
+ }
}
nf_bridge_update_protocol(skb);
} else {
@@ -782,11 +779,6 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
if (!skb->nf_bridge)
return NF_ACCEPT;
- /* Need exclusive nf_bridge_info since we might have multiple
- * different physoutdevs. */
- if (!nf_bridge_unshare(skb))
- return NF_DROP;
-
parent = bridge_parent(out);
if (!parent)
return NF_DROP;
@@ -803,14 +795,16 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
nf_bridge = skb->nf_bridge;
if (skb->pkt_type == PACKET_OTHERHOST) {
skb->pkt_type = PACKET_HOST;
- nf_bridge->mask |= BRNF_PKT_TYPE;
+ nf_bridge = nf_bridge_set_mask(skb,
+ BRNF_PKT_TYPE | BRNF_BRIDGED);
+ } else {
+ /* The physdev module checks on this */
+ nf_bridge = nf_bridge_set_mask(skb, BRNF_BRIDGED);
}
- if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
+ if (!nf_bridge || (pf == NFPROTO_IPV4 && br_parse_ip_options(skb)))
return NF_DROP;
- /* The physdev module checks on this */
- nf_bridge->mask |= BRNF_BRIDGED;
nf_bridge->physoutdev = skb->dev;
if (pf == NFPROTO_IPV4)
skb->protocol = htons(ETH_P_IP);
@@ -911,7 +905,8 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
* about the value of skb->pkt_type. */
if (skb->pkt_type == PACKET_OTHERHOST) {
skb->pkt_type = PACKET_HOST;
- nf_bridge->mask |= BRNF_PKT_TYPE;
+ if (!nf_bridge_set_mask(skb, BRNF_PKT_TYPE))
+ return NF_DROP;
}
nf_bridge_pull_encap_header(skb);
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] netfilter: bridge: unshare bridge info before change it
2014-09-29 7:35 [PATCH] netfilter: bridge: unshare bridge info before change it Gao feng
@ 2014-11-04 0:45 ` Gao feng
2014-11-04 19:00 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Gao feng @ 2014-11-04 0:45 UTC (permalink / raw)
To: netfilter-devel; +Cc: pablo
On 09/29/2014 03:35 PM, Gao feng wrote:
> Many packets may share the same bridge information,
> we should unshare the bridge info before we change it,
> otherwise other packets will go to PF_INET(6)/PRE_ROUTING
> second time or the pkt_type of other packets will be
> incorrect.
>
> The problem occurs when we do nfqueue after br_nf_pre_routing
> and before bf_nf_pre_routing_finish, if the packet is gso,
> the new segs will share the same bridge info. and netfilter
> may use skb_clone, this will cause many packets share the
> same bridge info too.
>
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
any comments?
> include/linux/netfilter_bridge.h | 54 ++++++++++++++++++++-
> net/bridge/br_netfilter.c | 101 +++++++++++++++++++--------------------
> 2 files changed, 100 insertions(+), 55 deletions(-)
>
> diff --git a/include/linux/netfilter_bridge.h b/include/linux/netfilter_bridge.h
> index 8ab1c27..25cfeab 100644
> --- a/include/linux/netfilter_bridge.h
> +++ b/include/linux/netfilter_bridge.h
> @@ -53,14 +53,64 @@ static inline unsigned int nf_bridge_mtu_reduction(const struct sk_buff *skb)
> return 0;
> }
>
> +static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
> +{
> + skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
> + if (likely(skb->nf_bridge))
> + atomic_set(&(skb->nf_bridge->use), 1);
> +
> + return skb->nf_bridge;
> +}
> +
> +static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
> +{
> + struct nf_bridge_info *nf_bridge = skb->nf_bridge;
> +
> + if (atomic_read(&nf_bridge->use) > 1) {
> + struct nf_bridge_info *tmp = nf_bridge_alloc(skb);
> +
> + if (tmp) {
> + memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info));
> + atomic_set(&tmp->use, 1);
> + }
> + nf_bridge_put(nf_bridge);
> + nf_bridge = tmp;
> + }
> + return nf_bridge;
> +}
> +
> +static inline struct nf_bridge_info *
> +nf_bridge_set_mask(struct sk_buff *skb, unsigned int mask)
> +{
> + if (!nf_bridge_unshare(skb))
> + return NULL;
> +
> + skb->nf_bridge->mask |= mask;
> + return skb->nf_bridge;
> +}
> +
> +static inline struct nf_bridge_info *
> +nf_bridge_change_mask(struct sk_buff *skb, unsigned int mask)
> +{
> + if (!nf_bridge_unshare(skb))
> + return NULL;
> +
> + skb->nf_bridge->mask ^= mask;
> + return skb->nf_bridge;
> +}
> +
> int br_handle_frame_finish(struct sk_buff *skb);
> /* Only used in br_device.c */
> static inline int br_nf_pre_routing_finish_bridge_slow(struct sk_buff *skb)
> {
> - struct nf_bridge_info *nf_bridge = skb->nf_bridge;
> + struct nf_bridge_info *nf_bridge;
>
> skb_pull(skb, ETH_HLEN);
> - nf_bridge->mask ^= BRNF_BRIDGED_DNAT;
> + nf_bridge = nf_bridge_change_mask(skb, BRNF_BRIDGED_DNAT);
> + if (nf_bridge == NULL) {
> + kfree_skb(skb);
> + return 0;
> + }
> skb_copy_to_linear_data_offset(skb, -(ETH_HLEN-ETH_ALEN),
> skb->nf_bridge->data, ETH_HLEN-ETH_ALEN);
> skb->dev = nf_bridge->physindev;
> diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
> index a615264..eeca74e 100644
> --- a/net/bridge/br_netfilter.c
> +++ b/net/bridge/br_netfilter.c
> @@ -187,32 +187,6 @@ static inline struct net_device *bridge_parent(const struct net_device *dev)
> return port ? port->br->dev : NULL;
> }
>
> -static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb)
> -{
> - skb->nf_bridge = kzalloc(sizeof(struct nf_bridge_info), GFP_ATOMIC);
> - if (likely(skb->nf_bridge))
> - atomic_set(&(skb->nf_bridge->use), 1);
> -
> - return skb->nf_bridge;
> -}
> -
> -static inline struct nf_bridge_info *nf_bridge_unshare(struct sk_buff *skb)
> -{
> - struct nf_bridge_info *nf_bridge = skb->nf_bridge;
> -
> - if (atomic_read(&nf_bridge->use) > 1) {
> - struct nf_bridge_info *tmp = nf_bridge_alloc(skb);
> -
> - if (tmp) {
> - memcpy(tmp, nf_bridge, sizeof(struct nf_bridge_info));
> - atomic_set(&tmp->use, 1);
> - }
> - nf_bridge_put(nf_bridge);
> - nf_bridge = tmp;
> - }
> - return nf_bridge;
> -}
> -
> static inline void nf_bridge_push_encap_header(struct sk_buff *skb)
> {
> unsigned int len = nf_bridge_encap_header_len(skb);
> @@ -345,20 +319,25 @@ int nf_bridge_copy_header(struct sk_buff *skb)
> * bridge PRE_ROUTING hook. */
> static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
> {
> - struct nf_bridge_info *nf_bridge = skb->nf_bridge;
> + struct nf_bridge_info *nf_bridge;
> struct rtable *rt;
>
> - if (nf_bridge->mask & BRNF_PKT_TYPE) {
> + if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
> skb->pkt_type = PACKET_OTHERHOST;
> - nf_bridge->mask ^= BRNF_PKT_TYPE;
> + nf_bridge = nf_bridge_change_mask(skb,
> + BRNF_PKT_TYPE | BRNF_NF_BRIDGE_PREROUTING);
> + } else {
> + nf_bridge = nf_bridge_change_mask(skb,
> + BRNF_NF_BRIDGE_PREROUTING);
> }
> - nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
> +
> + if (nf_bridge == NULL)
> + goto drop;
>
> rt = bridge_parent_rtable(nf_bridge->physindev);
> - if (!rt) {
> - kfree_skb(skb);
> - return 0;
> - }
> + if (!rt)
> + goto drop;
> +
> skb_dst_set_noref(skb, &rt->dst);
>
> skb->dev = nf_bridge->physindev;
> @@ -366,8 +345,11 @@ static int br_nf_pre_routing_finish_ipv6(struct sk_buff *skb)
> nf_bridge_push_encap_header(skb);
> NF_HOOK_THRESH(NFPROTO_BRIDGE, NF_BR_PRE_ROUTING, skb, skb->dev, NULL,
> br_handle_frame_finish, 1);
> -
> +out:
> return 0;
> +drop:
> + kfree_skb(skb);
> + goto out;
> }
>
> /* Obtain the correct destination MAC address, while preserving the original
> @@ -387,7 +369,7 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
> dst = skb_dst(skb);
> neigh = dst_neigh_lookup_skb(dst, skb);
> if (neigh) {
> - int ret;
> + int ret = 0;
>
> if (neigh->hh.hh_len) {
> neigh_hh_bridge(&neigh->hh, skb);
> @@ -403,8 +385,10 @@ static int br_nf_pre_routing_finish_bridge(struct sk_buff *skb)
> skb->nf_bridge->data,
> ETH_HLEN-ETH_ALEN);
> /* tell br_dev_xmit to continue with forwarding */
> - nf_bridge->mask |= BRNF_BRIDGED_DNAT;
> - ret = neigh->output(neigh, skb);
> + if (nf_bridge_set_mask(skb, BRNF_BRIDGED_DNAT) == NULL)
> + kfree_skb(skb);
> + else
> + ret = neigh->output(neigh, skb);
> }
> neigh_release(neigh);
> return ret;
> @@ -456,15 +440,24 @@ static int br_nf_pre_routing_finish(struct sk_buff *skb)
> {
> struct net_device *dev = skb->dev;
> struct iphdr *iph = ip_hdr(skb);
> - struct nf_bridge_info *nf_bridge = skb->nf_bridge;
> + struct nf_bridge_info *nf_bridge;
> struct rtable *rt;
> int err;
>
> - if (nf_bridge->mask & BRNF_PKT_TYPE) {
> + if (skb->nf_bridge->mask & BRNF_PKT_TYPE) {
> skb->pkt_type = PACKET_OTHERHOST;
> - nf_bridge->mask ^= BRNF_PKT_TYPE;
> + nf_bridge = nf_bridge_change_mask(skb,
> + BRNF_PKT_TYPE | BRNF_NF_BRIDGE_PREROUTING);
> + } else {
> + nf_bridge = nf_bridge_change_mask(skb,
> + BRNF_NF_BRIDGE_PREROUTING);
> + }
> +
> + if (nf_bridge == NULL) {
> + kfree_skb(skb);
> + return 0;
> }
> - nf_bridge->mask ^= BRNF_NF_BRIDGE_PREROUTING;
> +
> if (dnat_took_place(skb)) {
> if ((err = ip_route_input(skb, iph->daddr, iph->saddr, iph->tos, dev))) {
> struct in_device *in_dev = __in_dev_get_rcu(dev);
> @@ -750,7 +743,11 @@ static int br_nf_forward_finish(struct sk_buff *skb)
> in = nf_bridge->physindev;
> if (nf_bridge->mask & BRNF_PKT_TYPE) {
> skb->pkt_type = PACKET_OTHERHOST;
> - nf_bridge->mask ^= BRNF_PKT_TYPE;
> +
> + if (!nf_bridge_change_mask(skb, BRNF_PKT_TYPE)) {
> + kfree_skb(skb);
> + return 0;
> + }
> }
> nf_bridge_update_protocol(skb);
> } else {
> @@ -782,11 +779,6 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
> if (!skb->nf_bridge)
> return NF_ACCEPT;
>
> - /* Need exclusive nf_bridge_info since we might have multiple
> - * different physoutdevs. */
> - if (!nf_bridge_unshare(skb))
> - return NF_DROP;
> -
> parent = bridge_parent(out);
> if (!parent)
> return NF_DROP;
> @@ -803,14 +795,16 @@ static unsigned int br_nf_forward_ip(const struct nf_hook_ops *ops,
> nf_bridge = skb->nf_bridge;
> if (skb->pkt_type == PACKET_OTHERHOST) {
> skb->pkt_type = PACKET_HOST;
> - nf_bridge->mask |= BRNF_PKT_TYPE;
> + nf_bridge = nf_bridge_set_mask(skb,
> + BRNF_PKT_TYPE | BRNF_BRIDGED);
> + } else {
> + /* The physdev module checks on this */
> + nf_bridge = nf_bridge_set_mask(skb, BRNF_BRIDGED);
> }
>
> - if (pf == NFPROTO_IPV4 && br_parse_ip_options(skb))
> + if (!nf_bridge || (pf == NFPROTO_IPV4 && br_parse_ip_options(skb)))
> return NF_DROP;
>
> - /* The physdev module checks on this */
> - nf_bridge->mask |= BRNF_BRIDGED;
> nf_bridge->physoutdev = skb->dev;
> if (pf == NFPROTO_IPV4)
> skb->protocol = htons(ETH_P_IP);
> @@ -911,7 +905,8 @@ static unsigned int br_nf_post_routing(const struct nf_hook_ops *ops,
> * about the value of skb->pkt_type. */
> if (skb->pkt_type == PACKET_OTHERHOST) {
> skb->pkt_type = PACKET_HOST;
> - nf_bridge->mask |= BRNF_PKT_TYPE;
> + if (!nf_bridge_set_mask(skb, BRNF_PKT_TYPE))
> + return NF_DROP;
> }
>
> nf_bridge_pull_encap_header(skb);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] netfilter: bridge: unshare bridge info before change it
2014-11-04 0:45 ` Gao feng
@ 2014-11-04 19:00 ` Pablo Neira Ayuso
2014-11-05 2:01 ` Gao feng
0 siblings, 1 reply; 6+ messages in thread
From: Pablo Neira Ayuso @ 2014-11-04 19:00 UTC (permalink / raw)
To: Gao feng; +Cc: netfilter-devel
On Tue, Nov 04, 2014 at 08:45:09AM +0800, Gao feng wrote:
> On 09/29/2014 03:35 PM, Gao feng wrote:
> > Many packets may share the same bridge information,
> > we should unshare the bridge info before we change it,
> > otherwise other packets will go to PF_INET(6)/PRE_ROUTING
> > second time or the pkt_type of other packets will be
> > incorrect.
> >
> > The problem occurs when we do nfqueue after br_nf_pre_routing
> > and before bf_nf_pre_routing_finish, if the packet is gso,
> > the new segs will share the same bridge info. and netfilter
> > may use skb_clone, this will cause many packets share the
> > same bridge info too.
> >
> > Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> > ---
>
> any comments?
This doesn't apply cleanly. We modularized br_netfilter by the time
you sent this, see 54dc125. You'll have to rebase this patch.
Moreover, could you develop what you're noticing a bit more? Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: bridge: unshare bridge info before change it
2014-11-04 19:00 ` Pablo Neira Ayuso
@ 2014-11-05 2:01 ` Gao feng
2014-11-05 2:13 ` Gao feng
2014-11-13 14:13 ` Pablo Neira Ayuso
0 siblings, 2 replies; 6+ messages in thread
From: Gao feng @ 2014-11-05 2:01 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On 11/05/2014 03:00 AM, Pablo Neira Ayuso wrote:
> On Tue, Nov 04, 2014 at 08:45:09AM +0800, Gao feng wrote:
>> On 09/29/2014 03:35 PM, Gao feng wrote:
>>> Many packets may share the same bridge information,
>>> we should unshare the bridge info before we change it,
>>> otherwise other packets will go to PF_INET(6)/PRE_ROUTING
>>> second time or the pkt_type of other packets will be
>>> incorrect.
>>>
>>> The problem occurs when we do nfqueue after br_nf_pre_routing
>>> and before bf_nf_pre_routing_finish, if the packet is gso,
>>> the new segs will share the same bridge info. and netfilter
>>> may use skb_clone, this will cause many packets share the
>>> same bridge info too.
>>>
>>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>>> ---
>>
>> any comments?
>
> This doesn't apply cleanly. We modularized br_netfilter by the time
> you sent this, see 54dc125. You'll have to rebase this patch.
Get.
>
> Moreover, could you develop what you're noticing a bit more? Thanks.
>
first we setup NFQUEUE rule on ipv4 PREROUTING chain.
when gso packet came in from bridge, br_nf_pre_routing will
allocate nf_bridge_info for this gso packet. and call setup_pre_routing
to setup nf_bridge_info.(such as nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING)
then this packet goes to ipv4 prerouting chain, nfqnl_enqueue_packet
will call skb_segment to segment this gso packet. in skb_segment, the new
packets will copy gso packet's header(__copy_skb_header), so there will
be many packets share the same nf_bridge_info.
When these segmented packets being reinjected into kernel, they will continue
going through bridge netfilter, br_nf_pre_routing_finish will clean the
BRNF_NF_BRIDGE_PREROUTING for the first packet, setup it for the secondary
packet, clean it for the third packet...
if the dest of these packets is local machine, they will come into br_pass_frame_up.
then go to ipv4 prerouting chain again through netif_receive_skb. so ip_sabotage_in
will not stop half of these packet.
I only met the BRNF_NF_BRIDGE_PREROUTING flag problem, the other flags of nf_bridge_info's
mask may cause problem too.
One solution is allocate new bridge_info in nfqnl_enqueue_packet for segmented packet,
but __copy_skb_header may be called in the scene I described above, So I decide to
allocate new bridge_info before we change it.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: bridge: unshare bridge info before change it
2014-11-05 2:01 ` Gao feng
@ 2014-11-05 2:13 ` Gao feng
2014-11-13 14:13 ` Pablo Neira Ayuso
1 sibling, 0 replies; 6+ messages in thread
From: Gao feng @ 2014-11-05 2:13 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
On 11/05/2014 10:01 AM, Gao feng wrote:
> On 11/05/2014 03:00 AM, Pablo Neira Ayuso wrote:
> but __copy_skb_header may be called in the scene I described above,
I mean __copy_skb_header between br_nf_pre_routing and br_nf_pre_routing_finish
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] netfilter: bridge: unshare bridge info before change it
2014-11-05 2:01 ` Gao feng
2014-11-05 2:13 ` Gao feng
@ 2014-11-13 14:13 ` Pablo Neira Ayuso
1 sibling, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2014-11-13 14:13 UTC (permalink / raw)
To: Gao feng; +Cc: netfilter-devel
On Wed, Nov 05, 2014 at 10:01:10AM +0800, Gao feng wrote:
> On 11/05/2014 03:00 AM, Pablo Neira Ayuso wrote:
> > This doesn't apply cleanly. We modularized br_netfilter by the time
> > you sent this, see 54dc125. You'll have to rebase this patch.
>
> Get.
>
> >
> > Moreover, could you develop what you're noticing a bit more? Thanks.
> >
>
> first we setup NFQUEUE rule on ipv4 PREROUTING chain.
>
> when gso packet came in from bridge, br_nf_pre_routing will
> allocate nf_bridge_info for this gso packet. and call setup_pre_routing
> to setup nf_bridge_info.(such as nf_bridge->mask |= BRNF_NF_BRIDGE_PREROUTING)
>
> then this packet goes to ipv4 prerouting chain, nfqnl_enqueue_packet
> will call skb_segment to segment this gso packet. in skb_segment, the new
> packets will copy gso packet's header(__copy_skb_header), so there will
> be many packets share the same nf_bridge_info.
>
> When these segmented packets being reinjected into kernel, they will continue
> going through bridge netfilter, br_nf_pre_routing_finish will clean the
> BRNF_NF_BRIDGE_PREROUTING for the first packet, setup it for the secondary
> packet, clean it for the third packet...
>
> if the dest of these packets is local machine, they will come into br_pass_frame_up.
> then go to ipv4 prerouting chain again through netif_receive_skb. so ip_sabotage_in
> will not stop half of these packet.
I see, so this is manifesting when the packet follows the bridge input path.
Please, include this in the patch description.
> I only met the BRNF_NF_BRIDGE_PREROUTING flag problem, the other flags of nf_bridge_info's
> mask may cause problem too.
>
> One solution is allocate new bridge_info in nfqnl_enqueue_packet for segmented packet,
> but __copy_skb_header may be called in the scene I described above, So I decide to
> allocate new bridge_info before we change it.
I think the changes should be restricted to the br_netfilter scope. I
don't come up with any smaller / better solution at this moment, so
please rebase and resubmit.
BTW, br_netfilter seems to be using this:
nf_bridge->mask ^= ...
to always unset a bit that was previously set? In that case, I would
rename the function to _unset instead of _change, and use &~, at quick
look the use of xor for this there seems sloppy to me.
Thanks for your patience.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-13 14:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-29 7:35 [PATCH] netfilter: bridge: unshare bridge info before change it Gao feng
2014-11-04 0:45 ` Gao feng
2014-11-04 19:00 ` Pablo Neira Ayuso
2014-11-05 2:01 ` Gao feng
2014-11-05 2:13 ` Gao feng
2014-11-13 14:13 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).