From patchwork Sun Nov 14 21:12:05 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: stephen hemminger X-Patchwork-Id: 71154 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 29C95B7117 for ; Mon, 15 Nov 2010 08:18:31 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932194Ab0KNVSR (ORCPT ); Sun, 14 Nov 2010 16:18:17 -0500 Received: from suva.vyatta.com ([76.74.103.44]:52961 "EHLO suva.vyatta.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756938Ab0KNVSM (ORCPT ); Sun, 14 Nov 2010 16:18:12 -0500 Received: from suva.vyatta.com (suva [127.0.0.1]) by suva.vyatta.com (8.13.7/8.13.7) with ESMTP id oAELHweA016832; Sun, 14 Nov 2010 13:17:59 -0800 Received: (from shemminger@localhost) by suva.vyatta.com (8.13.7/8.13.7/Submit) id oAELHwTN016831; Sun, 14 Nov 2010 13:17:58 -0800 Message-Id: <20101114211515.424372255@vyatta.com> User-Agent: quilt/0.48-1 Date: Sun, 14 Nov 2010 13:12:05 -0800 From: Stephen Hemminger To: David Miller , Eric Dumazet Cc: netdev@vger.kernel.org, bridge@lists.linux-foundation.org Subject: [PATCH 4/5] bridge: fix RCU races with bridge port References: <20101114211201.678755903@vyatta.com> Content-Disposition: inline; filename=br-port-rcu-race.patch Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org The macro br_port_exists() is not enough protection when only RCU is being used. There is a tiny race where other CPU has cleared port handler hook, but is bridge port flag might still be set. Signed-off-by: Stephen Hemminger --- net/bridge/br_fdb.c | 15 +++++++++------ net/bridge/br_if.c | 5 +---- net/bridge/br_netfilter.c | 13 +++++++------ net/bridge/br_netlink.c | 10 ++++++---- net/bridge/br_notify.c | 2 +- net/bridge/br_private.h | 16 +++++++++++++--- net/bridge/br_stp_bpdu.c | 8 ++++---- net/bridge/netfilter/ebtables.c | 11 +++++------ 8 files changed, 46 insertions(+), 34 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/net/bridge/br_netfilter.c 2010-11-14 12:36:22.970441653 -0800 +++ b/net/bridge/br_netfilter.c 2010-11-14 12:44:55.666987357 -0800 @@ -131,17 +131,18 @@ void br_netfilter_rtable_init(struct net static inline struct rtable *bridge_parent_rtable(const struct net_device *dev) { - if (!br_port_exists(dev)) - return NULL; - return &br_port_get_rcu(dev)->br->fake_rtable; + struct net_bridge_port *port; + + port = br_port_get_rcu(dev); + return port ? &port->br->fake_rtable : NULL; } static inline struct net_device *bridge_parent(const struct net_device *dev) { - if (!br_port_exists(dev)) - return NULL; + struct net_bridge_port *port; - return br_port_get_rcu(dev)->br->dev; + port = br_port_get_rcu(dev); + return port ? port->br->dev : NULL; } static inline struct nf_bridge_info *nf_bridge_alloc(struct sk_buff *skb) --- a/net/bridge/br_stp_bpdu.c 2010-11-14 12:36:22.982443121 -0800 +++ b/net/bridge/br_stp_bpdu.c 2010-11-14 12:44:55.666987357 -0800 @@ -141,10 +141,6 @@ void br_stp_rcv(const struct stp_proto * struct net_bridge *br; const unsigned char *buf; - if (!br_port_exists(dev)) - goto err; - p = br_port_get_rcu(dev); - if (!pskb_may_pull(skb, 4)) goto err; @@ -153,6 +149,10 @@ void br_stp_rcv(const struct stp_proto * if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0) goto err; + p = br_port_get_rcu(dev); + if (!p) + goto err; + br = p->br; spin_lock(&br->lock); --- a/net/bridge/netfilter/ebtables.c 2010-11-14 12:36:22.998445081 -0800 +++ b/net/bridge/netfilter/ebtables.c 2010-11-14 12:45:49.393153060 -0800 @@ -128,6 +128,7 @@ ebt_basic_match(const struct ebt_entry * const struct net_device *in, const struct net_device *out) { const struct ethhdr *h = eth_hdr(skb); + const struct net_bridge_port *p; __be16 ethproto; int verdict, i; @@ -148,13 +149,11 @@ ebt_basic_match(const struct ebt_entry * if (FWINV2(ebt_dev_check(e->out, out), EBT_IOUT)) return 1; /* rcu_read_lock()ed by nf_hook_slow */ - if (in && br_port_exists(in) && - FWINV2(ebt_dev_check(e->logical_in, br_port_get_rcu(in)->br->dev), - EBT_ILOGICALIN)) + if (in && (p = br_port_get_rcu(in)) != NULL && + FWINV2(ebt_dev_check(e->logical_in, p->br->dev), EBT_ILOGICALIN)) return 1; - if (out && br_port_exists(out) && - FWINV2(ebt_dev_check(e->logical_out, br_port_get_rcu(out)->br->dev), - EBT_ILOGICALOUT)) + if (out && (p = br_port_get_rcu(out)) != NULL && + FWINV2(ebt_dev_check(e->logical_out, p->br->dev), EBT_ILOGICALOUT)) return 1; if (e->bitmask & EBT_SOURCEMAC) { --- a/net/bridge/br_fdb.c 2010-11-14 12:36:23.022448019 -0800 +++ b/net/bridge/br_fdb.c 2010-11-14 12:44:55.670987817 -0800 @@ -238,15 +238,18 @@ struct net_bridge_fdb_entry *__br_fdb_ge int br_fdb_test_addr(struct net_device *dev, unsigned char *addr) { struct net_bridge_fdb_entry *fdb; + struct net_bridge_port *port; int ret; - if (!br_port_exists(dev)) - return 0; - rcu_read_lock(); - fdb = __br_fdb_get(br_port_get_rcu(dev)->br, addr); - ret = fdb && fdb->dst->dev != dev && - fdb->dst->state == BR_STATE_FORWARDING; + port = br_port_get_rcu(dev); + if (!port) + ret = 0; + else { + fdb = __br_fdb_get(port->br, addr); + ret = fdb && fdb->dst->dev != dev && + fdb->dst->state == BR_STATE_FORWARDING; + } rcu_read_unlock(); return ret; --- a/net/bridge/br_notify.c 2010-11-14 12:36:23.034449489 -0800 +++ b/net/bridge/br_notify.c 2010-11-14 12:44:55.670987817 -0800 @@ -32,7 +32,7 @@ struct notifier_block br_device_notifier static int br_device_event(struct notifier_block *unused, unsigned long event, void *ptr) { struct net_device *dev = ptr; - struct net_bridge_port *p = br_port_get(dev); + struct net_bridge_port *p; struct net_bridge *br; int err; --- a/net/bridge/br_private.h 2010-11-14 12:44:07.257410977 -0800 +++ b/net/bridge/br_private.h 2010-11-14 12:44:55.670987817 -0800 @@ -151,11 +151,21 @@ struct net_bridge_port #endif }; -#define br_port_get_rcu(dev) \ - ((struct net_bridge_port *) rcu_dereference(dev->rx_handler_data)) -#define br_port_get(dev) ((struct net_bridge_port *) dev->rx_handler_data) #define br_port_exists(dev) (dev->priv_flags & IFF_BRIDGE_PORT) +static inline struct net_bridge_port *br_port_get_rcu(const struct net_device *dev) +{ + return br_port_exists(dev) ? + rcu_dereference(dev->rx_handler_data) : NULL; +} + +static inline struct net_bridge_port *br_port_get(struct net_device *dev) +{ + return br_port_exists(dev) ? dev->rx_handler_data : NULL; +} + +#define br_port_get(dev) ((struct net_bridge_port *) dev->rx_handler_data) + struct br_cpu_netstats { u64 rx_packets; u64 rx_bytes; --- a/net/bridge/br_if.c 2010-11-14 12:36:23.046450958 -0800 +++ b/net/bridge/br_if.c 2010-11-14 12:44:55.670987817 -0800 @@ -475,11 +475,8 @@ int br_del_if(struct net_bridge *br, str { struct net_bridge_port *p; - if (!br_port_exists(dev)) - return -EINVAL; - p = br_port_get(dev); - if (p->br != br) + if (!p || p->br != br) return -EINVAL; del_nbp(p); --- a/net/bridge/br_netlink.c 2010-11-14 12:36:23.062452917 -0800 +++ b/net/bridge/br_netlink.c 2010-11-14 12:44:55.670987817 -0800 @@ -119,11 +119,13 @@ static int br_dump_ifinfo(struct sk_buff idx = 0; for_each_netdev(net, dev) { + struct net_bridge_port *port = br_port_get(dev); + /* not a bridge port */ - if (!br_port_exists(dev) || idx < cb->args[0]) + if (!port || idx < cb->args[0]) goto skip; - if (br_fill_ifinfo(skb, br_port_get(dev), + if (br_fill_ifinfo(skb, port, NETLINK_CB(cb->skb).pid, cb->nlh->nlmsg_seq, RTM_NEWLINK, NLM_F_MULTI) < 0) @@ -169,9 +171,9 @@ static int br_rtm_setlink(struct sk_buff if (!dev) return -ENODEV; - if (!br_port_exists(dev)) - return -EINVAL; p = br_port_get(dev); + if (!p) + return -EINVAL; /* if kernel STP is running, don't allow changes */ if (p->br->stp_enabled == BR_KERNEL_STP)