| Message ID | CAHA+R7PAzzz+CZfnOoweBdeLY8tRkr2bSDEpQgdtEKUeaZbWpQ@mail.gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 4 May 2015 11:45:41 -0700 Cong Wang <cwang@twopensource.com> wrote: > On Mon, May 4, 2015 at 6:39 AM, Dominick Grift <dac.override@gmail.com> wrote: > > On Thu, Apr 23, 2015 at 01:07:45PM -0400, Josh Boyer wrote: > >> Hi All, > >> > >> We've had a user report the following backtrace from the bridge module > >> with a recent Linus' tree. Has anything like this been reported yet? > >> If you have any questions on setup, the user is CC'd. > >> > >> josh > >> > >> [ 29.382235] br0: port 1(tap0) entered forwarding state > >> > >> [ 29.382286] =============================== > >> [ 29.382315] [ INFO: suspicious RCU usage. ] > >> [ 29.382344] 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 Not tainted > >> [ 29.382380] ------------------------------- > >> [ 29.382409] net/bridge/br_private.h:626 suspicious > >> rcu_dereference_check() usage! > > > > <snip> > > > > With 4.1.0-0.rc1.git1.1.fc23.x86_64 the situation seems to have slightly changed: > > > > Should be the same issue. Please give the attached patch a try, > it is compile-tested only. > > Thanks! Good analysis in identifying the issue. But the proposed patch doesn't seem right. The br->lock protects against changes to the bridge port state. vlan_info should be treated as part of the bridge state. The correct fix is to get vlan_info out of depending on RTNL and use br->lock to control modifications. -- 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
On Mon, May 4, 2015 at 1:27 PM, Stephen Hemminger <stephen@networkplumber.org> wrote: > On Mon, 4 May 2015 11:45:41 -0700 > Cong Wang <cwang@twopensource.com> wrote: > >> On Mon, May 4, 2015 at 6:39 AM, Dominick Grift <dac.override@gmail.com> wrote: >> > On Thu, Apr 23, 2015 at 01:07:45PM -0400, Josh Boyer wrote: >> >> Hi All, >> >> >> >> We've had a user report the following backtrace from the bridge module >> >> with a recent Linus' tree. Has anything like this been reported yet? >> >> If you have any questions on setup, the user is CC'd. >> >> >> >> josh >> >> >> >> [ 29.382235] br0: port 1(tap0) entered forwarding state >> >> >> >> [ 29.382286] =============================== >> >> [ 29.382315] [ INFO: suspicious RCU usage. ] >> >> [ 29.382344] 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 Not tainted >> >> [ 29.382380] ------------------------------- >> >> [ 29.382409] net/bridge/br_private.h:626 suspicious >> >> rcu_dereference_check() usage! >> > >> > <snip> >> > >> > With 4.1.0-0.rc1.git1.1.fc23.x86_64 the situation seems to have slightly changed: >> > >> >> Should be the same issue. Please give the attached patch a try, >> it is compile-tested only. >> >> Thanks! > > Good analysis in identifying the issue. But the proposed patch > doesn't seem right. > > The br->lock protects against changes to the bridge port state. > vlan_info should be treated as part of the bridge state. > > The correct fix is to get vlan_info out of depending on RTNL > and use br->lock to control modifications. It _looks like_ we only retrieve vlan info to fill netlink messages in timer context, so it doesn't seem we need to hold br->lock here. But I never look into br vlan code of course. -- 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
On 05/04/2015 04:27 PM, Stephen Hemminger wrote: > On Mon, 4 May 2015 11:45:41 -0700 > Cong Wang <cwang@twopensource.com> wrote: > >> On Mon, May 4, 2015 at 6:39 AM, Dominick Grift <dac.override@gmail.com> wrote: >>> On Thu, Apr 23, 2015 at 01:07:45PM -0400, Josh Boyer wrote: >>>> Hi All, >>>> >>>> We've had a user report the following backtrace from the bridge module >>>> with a recent Linus' tree. Has anything like this been reported yet? >>>> If you have any questions on setup, the user is CC'd. >>>> >>>> josh >>>> >>>> [ 29.382235] br0: port 1(tap0) entered forwarding state >>>> >>>> [ 29.382286] =============================== >>>> [ 29.382315] [ INFO: suspicious RCU usage. ] >>>> [ 29.382344] 4.1.0-0.rc0.git11.1.fc23.x86_64 #1 Not tainted >>>> [ 29.382380] ------------------------------- >>>> [ 29.382409] net/bridge/br_private.h:626 suspicious >>>> rcu_dereference_check() usage! >>> >>> <snip> >>> >>> With 4.1.0-0.rc1.git1.1.fc23.x86_64 the situation seems to have slightly changed: >>> >> >> Should be the same issue. Please give the attached patch a try, >> it is compile-tested only. >> >> Thanks! > > Good analysis in identifying the issue. But the proposed patch > doesn't seem right. > > The br->lock protects against changes to the bridge port state. > vlan_info should be treated as part of the bridge state. > > The correct fix is to get vlan_info out of depending on RTNL > and use br->lock to control modifications. > Changing the write side protection to be dependent on br->lock would then require rcu or lock to be held in br_getlink(). It all boils down to the same thing. br_fill_info() needs to be either in rcu or locked context. It's already in rtnl context, so Eric's proposal is the simplest one. -vlad > -- > 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 > -- 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
On Mon, May 04, 2015 at 11:45:41AM -0700, Cong Wang wrote: <snip> > Should be the same issue. Please give the attached patch a try, > it is compile-tested only. > > Thanks! <snip> Thank you. I have just booted with the above path and the "oops" is gone So although the patch may not be optimal, it does seem to take care of the issue.
On Mon, May 04, 2015 at 02:35:27PM -0700, Cong Wang wrote: <snip> > It _looks like_ we only retrieve vlan info to fill netlink > messages in timer context, so it doesn't seem we need to > hold br->lock here. > > But I never look into br vlan code of course. This is just a friendly reminder that this issue still exits in: 4.1.0-0.rc2.git3.1 Ignore me if you are aware of the above
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c index 4b5c236..8ba989a 100644 --- a/net/bridge/br_netlink.c +++ b/net/bridge/br_netlink.c @@ -325,22 +325,28 @@ static int br_fill_ifinfo(struct sk_buff *skb, struct nlattr *af; int err; + rcu_read_lock(); if (port) pv = nbp_get_vlan_info(port); else pv = br_get_vlan_info(br); - if (!pv || bitmap_empty(pv->vlan_bitmap, VLAN_N_VID)) + if (!pv || bitmap_empty(pv->vlan_bitmap, VLAN_N_VID)) { + rcu_read_unlock(); goto done; + } af = nla_nest_start(skb, IFLA_AF_SPEC); - if (!af) + if (!af) { + rcu_read_unlock(); goto nla_put_failure; + } if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED) err = br_fill_ifvlaninfo_compressed(skb, pv); else err = br_fill_ifvlaninfo(skb, pv); + rcu_read_unlock(); if (err) goto nla_put_failure; nla_nest_end(skb, af); diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 3362c29..860832e 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -617,13 +617,17 @@ int nbp_vlan_init(struct net_bridge_port *port); static inline struct net_port_vlans *br_get_vlan_info( const struct net_bridge *br) { - return rcu_dereference_rtnl(br->vlan_info); + return rcu_dereference_protected(br->vlan_info, + rcu_read_lock_held() || + lockdep_rtnl_is_held()); } static inline struct net_port_vlans *nbp_get_vlan_info( const struct net_bridge_port *p) { - return rcu_dereference_rtnl(p->vlan_info); + return rcu_dereference_protected(p->vlan_info, + rcu_read_lock_held() || + lockdep_rtnl_is_held()); } /* Since bridge now depends on 8021Q module, but the time bridge sees the