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@xxxxxxxxxxxxxx>
> ---
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);
>
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
