| Message ID | 1386257257-25258-1-git-send-email-jiri@resnulli.us |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Dec 05, 2013 at 04:27:37PM +0100, Jiri Pirko wrote: > br_stp_rcv() is reached by non-rx_handler path. That means there is no > guarantee that dev is bridge port and therefore simple NULL check of > ->rx_handler_data is not enough. There is need to check if dev is really > bridge port and since only rcu read lock is held here, do it by checking > ->rx_handler pointer. > > Note that synchronize_net() in netdev_rx_handler_unregister() ensures > this approach as valid. > > Introduced originally by: > commit f350a0a87374418635689471606454abc7beaa3a > "bridge: use rx_handler_data pointer to store net_bridge_port pointer" > > Fixed but not in the best way by: > commit b5ed54e94d324f17c97852296d61a143f01b227a > "bridge: fix RCU races with bridge port" > > Reintroduced by: > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 > "bridge: fix NULL pointer deref of br_port_get_rcu" > > Please apply to stable trees as well. Thanks. > > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 > > Reported-by: Laine Stump <laine@redhat.com> > Debugged-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition > > net/bridge/br_private.h | 10 ++++++++++ > net/bridge/br_stp_bpdu.c | 2 +- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h > index 229d820..045d56e 100644 > --- a/net/bridge/br_private.h > +++ b/net/bridge/br_private.h > @@ -426,6 +426,16 @@ netdev_features_t br_features_recompute(struct net_bridge *br, > int br_handle_frame_finish(struct sk_buff *skb); > rx_handler_result_t br_handle_frame(struct sk_buff **pskb); > > +static inline bool br_rx_handler_check_rcu(const struct net_device *dev) > +{ > + return rcu_dereference(dev->rx_handler) == br_handle_frame; > +} > + > +static inline struct net_bridge_port *br_port_get_check_rcu(const struct net_device *dev) > +{ > + return br_rx_handler_check_rcu(dev) ? br_port_get_rcu(dev) : NULL; > +} > + > /* br_ioctl.c */ > int br_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd); > int br_ioctl_deviceless_stub(struct net *net, unsigned int cmd, > diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c > index 8660ea3..bdb459d 100644 > --- a/net/bridge/br_stp_bpdu.c > +++ b/net/bridge/br_stp_bpdu.c > @@ -153,7 +153,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb, > if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0) > goto err; > > - p = br_port_get_rcu(dev); > + p = br_port_get_check_rcu(dev); > if (!p) > goto err; > > -- > 1.8.3.1 -- 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 Thu, 2013-12-05 at 16:27 +0100, Jiri Pirko wrote: > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 > > Reported-by: Laine Stump <laine@redhat.com> > Debugged-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > --- Acked-by: Eric Dumazet <edumazet@google.com> Thanks -- 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
From: Jiri Pirko <jiri@resnulli.us> Date: Thu, 5 Dec 2013 16:27:37 +0100 > br_stp_rcv() is reached by non-rx_handler path. That means there is no > guarantee that dev is bridge port and therefore simple NULL check of > ->rx_handler_data is not enough. There is need to check if dev is really > bridge port and since only rcu read lock is held here, do it by checking > ->rx_handler pointer. > > Note that synchronize_net() in netdev_rx_handler_unregister() ensures > this approach as valid. > > Introduced originally by: > commit f350a0a87374418635689471606454abc7beaa3a > "bridge: use rx_handler_data pointer to store net_bridge_port pointer" > > Fixed but not in the best way by: > commit b5ed54e94d324f17c97852296d61a143f01b227a > "bridge: fix RCU races with bridge port" > > Reintroduced by: > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 > "bridge: fix NULL pointer deref of br_port_get_rcu" > > Please apply to stable trees as well. Thanks. > > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 > > Reported-by: Laine Stump <laine@redhat.com> > Debugged-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > --- > v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition Applied and queued up for -stable, thanks Jiri. -- 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 Fri, 06 Dec 2013 15:43:21 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Jiri Pirko <jiri@resnulli.us> > Date: Thu, 5 Dec 2013 16:27:37 +0100 > > > br_stp_rcv() is reached by non-rx_handler path. That means there is no > > guarantee that dev is bridge port and therefore simple NULL check of > > ->rx_handler_data is not enough. There is need to check if dev is really > > bridge port and since only rcu read lock is held here, do it by checking > > ->rx_handler pointer. > > > > Note that synchronize_net() in netdev_rx_handler_unregister() ensures > > this approach as valid. > > > > Introduced originally by: > > commit f350a0a87374418635689471606454abc7beaa3a > > "bridge: use rx_handler_data pointer to store net_bridge_port pointer" > > > > Fixed but not in the best way by: > > commit b5ed54e94d324f17c97852296d61a143f01b227a > > "bridge: fix RCU races with bridge port" > > > > Reintroduced by: > > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 > > "bridge: fix NULL pointer deref of br_port_get_rcu" > > > > Please apply to stable trees as well. Thanks. > > > > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 > > > > Reported-by: Laine Stump <laine@redhat.com> > > Debugged-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > > --- > > v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition > > Applied and queued up for -stable, thanks Jiri. How come you ignored my simpler fix, that used the existing logic. I don't like introducing this especially into the stable; much prefer to go back to testing the flag as was being done before. -- 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
From: Stephen Hemminger <stephen@networkplumber.org> Date: Fri, 6 Dec 2013 13:10:28 -0800 > How come you ignored my simpler fix, that used the existing logic. > I don't like introducing this especially into the stable; much prefer > to go back to testing the flag as was being done before. I thought Jiri's response to your suggestion was adequate, that's all. I can revert and take your version if you want me to, just make a formal submission and get agreement from Jiri. Thanks. -- 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
Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: >On Fri, 06 Dec 2013 15:43:21 -0500 (EST) >David Miller <davem@davemloft.net> wrote: > >> From: Jiri Pirko <jiri@resnulli.us> >> Date: Thu, 5 Dec 2013 16:27:37 +0100 >> >> > br_stp_rcv() is reached by non-rx_handler path. That means there is no >> > guarantee that dev is bridge port and therefore simple NULL check of >> > ->rx_handler_data is not enough. There is need to check if dev is really >> > bridge port and since only rcu read lock is held here, do it by checking >> > ->rx_handler pointer. >> > >> > Note that synchronize_net() in netdev_rx_handler_unregister() ensures >> > this approach as valid. >> > >> > Introduced originally by: >> > commit f350a0a87374418635689471606454abc7beaa3a >> > "bridge: use rx_handler_data pointer to store net_bridge_port pointer" >> > >> > Fixed but not in the best way by: >> > commit b5ed54e94d324f17c97852296d61a143f01b227a >> > "bridge: fix RCU races with bridge port" >> > >> > Reintroduced by: >> > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 >> > "bridge: fix NULL pointer deref of br_port_get_rcu" >> > >> > Please apply to stable trees as well. Thanks. >> > >> > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 >> > >> > Reported-by: Laine Stump <laine@redhat.com> >> > Debugged-by: Michael S. Tsirkin <mst@redhat.com> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> >> > --- >> > v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition >> >> Applied and queued up for -stable, thanks Jiri. > >How come you ignored my simpler fix, that used the existing logic. >I don't like introducing this especially into the stable; much prefer >to go back to testing the flag as was being done before. Although your patch is technically sane, it depends on rtnl indirectly. My patch depends on rcu locking and synchronize_rcu which is direct. Therefore I think it is more appropriate. Jiri -- 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 Sat, 7 Dec 2013 09:51:05 +0100 Jiri Pirko <jiri@resnulli.us> wrote: > Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: > >On Fri, 06 Dec 2013 15:43:21 -0500 (EST) > >David Miller <davem@davemloft.net> wrote: > > > >> From: Jiri Pirko <jiri@resnulli.us> > >> Date: Thu, 5 Dec 2013 16:27:37 +0100 > >> > >> > br_stp_rcv() is reached by non-rx_handler path. That means there is no > >> > guarantee that dev is bridge port and therefore simple NULL check of > >> > ->rx_handler_data is not enough. There is need to check if dev is really > >> > bridge port and since only rcu read lock is held here, do it by checking > >> > ->rx_handler pointer. > >> > > >> > Note that synchronize_net() in netdev_rx_handler_unregister() ensures > >> > this approach as valid. > >> > > >> > Introduced originally by: > >> > commit f350a0a87374418635689471606454abc7beaa3a > >> > "bridge: use rx_handler_data pointer to store net_bridge_port pointer" > >> > > >> > Fixed but not in the best way by: > >> > commit b5ed54e94d324f17c97852296d61a143f01b227a > >> > "bridge: fix RCU races with bridge port" > >> > > >> > Reintroduced by: > >> > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 > >> > "bridge: fix NULL pointer deref of br_port_get_rcu" > >> > > >> > Please apply to stable trees as well. Thanks. > >> > > >> > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 > >> > > >> > Reported-by: Laine Stump <laine@redhat.com> > >> > Debugged-by: Michael S. Tsirkin <mst@redhat.com> > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > >> > --- > >> > v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition > >> > >> Applied and queued up for -stable, thanks Jiri. > > > >How come you ignored my simpler fix, that used the existing logic. > >I don't like introducing this especially into the stable; much prefer > >to go back to testing the flag as was being done before. > > Although your patch is technically sane, it depends on rtnl indirectly. > My patch depends on rcu locking and synchronize_rcu which is direct. > Therefore I think it is more appropriate. After more review and thought I agree. But could we put some comments in br_private.h to describe the dependency on ordering (synchronize_net). Acked-by: Stephen Hemminger <stephen@networkplumber.org> -- 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
Sat, Dec 07, 2013 at 06:42:35PM CET, stephen@networkplumber.org wrote: >On Sat, 7 Dec 2013 09:51:05 +0100 >Jiri Pirko <jiri@resnulli.us> wrote: > >> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: >> >On Fri, 06 Dec 2013 15:43:21 -0500 (EST) >> >David Miller <davem@davemloft.net> wrote: >> > >> >> From: Jiri Pirko <jiri@resnulli.us> >> >> Date: Thu, 5 Dec 2013 16:27:37 +0100 >> >> >> >> > br_stp_rcv() is reached by non-rx_handler path. That means there is no >> >> > guarantee that dev is bridge port and therefore simple NULL check of >> >> > ->rx_handler_data is not enough. There is need to check if dev is really >> >> > bridge port and since only rcu read lock is held here, do it by checking >> >> > ->rx_handler pointer. >> >> > >> >> > Note that synchronize_net() in netdev_rx_handler_unregister() ensures >> >> > this approach as valid. >> >> > >> >> > Introduced originally by: >> >> > commit f350a0a87374418635689471606454abc7beaa3a >> >> > "bridge: use rx_handler_data pointer to store net_bridge_port pointer" >> >> > >> >> > Fixed but not in the best way by: >> >> > commit b5ed54e94d324f17c97852296d61a143f01b227a >> >> > "bridge: fix RCU races with bridge port" >> >> > >> >> > Reintroduced by: >> >> > commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 >> >> > "bridge: fix NULL pointer deref of br_port_get_rcu" >> >> > >> >> > Please apply to stable trees as well. Thanks. >> >> > >> >> > RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 >> >> > >> >> > Reported-by: Laine Stump <laine@redhat.com> >> >> > Debugged-by: Michael S. Tsirkin <mst@redhat.com> >> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >> > Signed-off-by: Jiri Pirko <jiri@resnulli.us> >> >> > --- >> >> > v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition >> >> >> >> Applied and queued up for -stable, thanks Jiri. >> > >> >How come you ignored my simpler fix, that used the existing logic. >> >I don't like introducing this especially into the stable; much prefer >> >to go back to testing the flag as was being done before. >> >> Although your patch is technically sane, it depends on rtnl indirectly. >> My patch depends on rcu locking and synchronize_rcu which is direct. >> Therefore I think it is more appropriate. > >After more review and thought I agree. But could we put some comments >in br_private.h to describe the dependency on ordering (synchronize_net). Sure. I will send follow-up patch addressing it. Thanks. > >Acked-by: Stephen Hemminger <stephen@networkplumber.org> -- 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 12/07/2013 03:51 AM, Jiri Pirko wrote: > Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: >> On Fri, 06 Dec 2013 15:43:21 -0500 (EST) >> David Miller <davem@davemloft.net> wrote: >> >>> From: Jiri Pirko <jiri@resnulli.us> >>> Date: Thu, 5 Dec 2013 16:27:37 +0100 >>> >>>> br_stp_rcv() is reached by non-rx_handler path. That means there is no >>>> guarantee that dev is bridge port and therefore simple NULL check of >>>> ->rx_handler_data is not enough. There is need to check if dev is really >>>> bridge port and since only rcu read lock is held here, do it by checking >>>> ->rx_handler pointer. >>>> >>>> Note that synchronize_net() in netdev_rx_handler_unregister() ensures >>>> this approach as valid. >>>> >>>> Introduced originally by: >>>> commit f350a0a87374418635689471606454abc7beaa3a >>>> "bridge: use rx_handler_data pointer to store net_bridge_port pointer" >>>> >>>> Fixed but not in the best way by: >>>> commit b5ed54e94d324f17c97852296d61a143f01b227a >>>> "bridge: fix RCU races with bridge port" >>>> >>>> Reintroduced by: >>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 >>>> "bridge: fix NULL pointer deref of br_port_get_rcu" >>>> >>>> Please apply to stable trees as well. Thanks. >>>> >>>> RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 >>>> >>>> Reported-by: Laine Stump <laine@redhat.com> >>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us> >>>> --- >>>> v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition >>> >>> Applied and queued up for -stable, thanks Jiri. >> >> How come you ignored my simpler fix, that used the existing logic. >> I don't like introducing this especially into the stable; much prefer >> to go back to testing the flag as was being done before. > > Although your patch is technically sane, it depends on rtnl indirectly. Pardon my ignorance, but I've been staring at this and I can't for the life of me see the dependency. The IFF_BRIDGE_PORT flag is set after the rx_handler is registered, so we are safe there. The rcu primitives will guarantee that the flag will be set by the time rx_handler and rx_handler_data are set. The flag is cleared before rx_handler is unregistered, so it is still valid to check for it in stp code. Once the flag is cleared we may still have a valid rx_handler during the rcu grace period, but will still avoid doing processing. So, where is the dependency on the rtnl? Thanks -vlad > My patch depends on rcu locking and synchronize_rcu which is direct. > Therefore I think it is more appropriate. > > Jiri > > > > -- > 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
Sat, Dec 07, 2013 at 08:10:45PM CET, vyasevich@gmail.com wrote: >On 12/07/2013 03:51 AM, Jiri Pirko wrote: >> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: >>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST) >>> David Miller <davem@davemloft.net> wrote: >>> >>>> From: Jiri Pirko <jiri@resnulli.us> >>>> Date: Thu, 5 Dec 2013 16:27:37 +0100 >>>> >>>>> br_stp_rcv() is reached by non-rx_handler path. That means there is no >>>>> guarantee that dev is bridge port and therefore simple NULL check of >>>>> ->rx_handler_data is not enough. There is need to check if dev is really >>>>> bridge port and since only rcu read lock is held here, do it by checking >>>>> ->rx_handler pointer. >>>>> >>>>> Note that synchronize_net() in netdev_rx_handler_unregister() ensures >>>>> this approach as valid. >>>>> >>>>> Introduced originally by: >>>>> commit f350a0a87374418635689471606454abc7beaa3a >>>>> "bridge: use rx_handler_data pointer to store net_bridge_port pointer" >>>>> >>>>> Fixed but not in the best way by: >>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a >>>>> "bridge: fix RCU races with bridge port" >>>>> >>>>> Reintroduced by: >>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 >>>>> "bridge: fix NULL pointer deref of br_port_get_rcu" >>>>> >>>>> Please apply to stable trees as well. Thanks. >>>>> >>>>> RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 >>>>> >>>>> Reported-by: Laine Stump <laine@redhat.com> >>>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us> >>>>> --- >>>>> v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition >>>> >>>> Applied and queued up for -stable, thanks Jiri. >>> >>> How come you ignored my simpler fix, that used the existing logic. >>> I don't like introducing this especially into the stable; much prefer >>> to go back to testing the flag as was being done before. >> >> Although your patch is technically sane, it depends on rtnl indirectly. > >Pardon my ignorance, but I've been staring at this and I can't for >the life of me see the dependency. > >The IFF_BRIDGE_PORT flag is set after the rx_handler is registered, >so we are safe there. The rcu primitives will guarantee that the flag >will be set by the time rx_handler and rx_handler_data are set. > >The flag is cleared before rx_handler is unregistered, so it is >still valid to check for it in stp code. Once the flag is cleared >we may still have a valid rx_handler during the rcu grace period, but >will still avoid doing processing. > >So, where is the dependency on the rtnl? Imagine br would release the netdev and some other rx_handler user would enslave the same netdev. This two events would happen between IFF_BRIDGE_PORT flag check and rx_handler_data get. That is what rtnl_lock prevents from happening. > >Thanks >-vlad > >> My patch depends on rcu locking and synchronize_rcu which is direct. >> Therefore I think it is more appropriate. >> >> Jiri >> >> >> >> -- >> 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 12/07/2013 03:07 PM, Jiri Pirko wrote: > Sat, Dec 07, 2013 at 08:10:45PM CET, vyasevich@gmail.com wrote: >> On 12/07/2013 03:51 AM, Jiri Pirko wrote: >>> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: >>>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST) >>>> David Miller <davem@davemloft.net> wrote: >>>> >>>>> From: Jiri Pirko <jiri@resnulli.us> >>>>> Date: Thu, 5 Dec 2013 16:27:37 +0100 >>>>> >>>>>> br_stp_rcv() is reached by non-rx_handler path. That means there is no >>>>>> guarantee that dev is bridge port and therefore simple NULL check of >>>>>> ->rx_handler_data is not enough. There is need to check if dev is really >>>>>> bridge port and since only rcu read lock is held here, do it by checking >>>>>> ->rx_handler pointer. >>>>>> >>>>>> Note that synchronize_net() in netdev_rx_handler_unregister() ensures >>>>>> this approach as valid. >>>>>> >>>>>> Introduced originally by: >>>>>> commit f350a0a87374418635689471606454abc7beaa3a >>>>>> "bridge: use rx_handler_data pointer to store net_bridge_port pointer" >>>>>> >>>>>> Fixed but not in the best way by: >>>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a >>>>>> "bridge: fix RCU races with bridge port" >>>>>> >>>>>> Reintroduced by: >>>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 >>>>>> "bridge: fix NULL pointer deref of br_port_get_rcu" >>>>>> >>>>>> Please apply to stable trees as well. Thanks. >>>>>> >>>>>> RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 >>>>>> >>>>>> Reported-by: Laine Stump <laine@redhat.com> >>>>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us> >>>>>> --- >>>>>> v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition >>>>> >>>>> Applied and queued up for -stable, thanks Jiri. >>>> >>>> How come you ignored my simpler fix, that used the existing logic. >>>> I don't like introducing this especially into the stable; much prefer >>>> to go back to testing the flag as was being done before. >>> >>> Although your patch is technically sane, it depends on rtnl indirectly. >> >> Pardon my ignorance, but I've been staring at this and I can't for >> the life of me see the dependency. >> >> The IFF_BRIDGE_PORT flag is set after the rx_handler is registered, >> so we are safe there. The rcu primitives will guarantee that the flag >> will be set by the time rx_handler and rx_handler_data are set. >> >> The flag is cleared before rx_handler is unregistered, so it is >> still valid to check for it in stp code. Once the flag is cleared >> we may still have a valid rx_handler during the rcu grace period, but >> will still avoid doing processing. >> >> So, where is the dependency on the rtnl? > > > Imagine br would release the netdev and some other rx_handler user would > enslave the same netdev. This two events would happen between > IFF_BRIDGE_PORT flag check and rx_handler_data get. That is what > rtnl_lock prevents from happening. I don't think this can happen. Inside br_stp_rcv(), we are in an rcu critical section. The release of the netdev (rx_handler unregister) forces us to to wait until synchronize_net() completes. So, if under rcu we checked the flag and it's on, the rx_handler will not change for the duration of that rcu section and we can safely process the packet even if the port is in the middle of going away. Howe does the race you describe happen? The reason I ask, is that we check priv_flags under rcu in other places to make sure that the device we are passing data to can handle it. If it's not safe, then those other places are vulnerable too. It doesn't seem to me that it is unsafe. Thanks -vlad > >> >> Thanks >> -vlad >> >>> My patch depends on rcu locking and synchronize_rcu which is direct. >>> Therefore I think it is more appropriate. >>> >>> Jiri >>> >>> >>> >>> -- >>> 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
Mon, Dec 09, 2013 at 03:07:18AM CET, vyasevich@gmail.com wrote: >On 12/07/2013 03:07 PM, Jiri Pirko wrote: >> Sat, Dec 07, 2013 at 08:10:45PM CET, vyasevich@gmail.com wrote: >>> On 12/07/2013 03:51 AM, Jiri Pirko wrote: >>>> Fri, Dec 06, 2013 at 10:10:28PM CET, stephen@networkplumber.org wrote: >>>>> On Fri, 06 Dec 2013 15:43:21 -0500 (EST) >>>>> David Miller <davem@davemloft.net> wrote: >>>>> >>>>>> From: Jiri Pirko <jiri@resnulli.us> >>>>>> Date: Thu, 5 Dec 2013 16:27:37 +0100 >>>>>> >>>>>>> br_stp_rcv() is reached by non-rx_handler path. That means there is no >>>>>>> guarantee that dev is bridge port and therefore simple NULL check of >>>>>>> ->rx_handler_data is not enough. There is need to check if dev is really >>>>>>> bridge port and since only rcu read lock is held here, do it by checking >>>>>>> ->rx_handler pointer. >>>>>>> >>>>>>> Note that synchronize_net() in netdev_rx_handler_unregister() ensures >>>>>>> this approach as valid. >>>>>>> >>>>>>> Introduced originally by: >>>>>>> commit f350a0a87374418635689471606454abc7beaa3a >>>>>>> "bridge: use rx_handler_data pointer to store net_bridge_port pointer" >>>>>>> >>>>>>> Fixed but not in the best way by: >>>>>>> commit b5ed54e94d324f17c97852296d61a143f01b227a >>>>>>> "bridge: fix RCU races with bridge port" >>>>>>> >>>>>>> Reintroduced by: >>>>>>> commit 716ec052d2280d511e10e90ad54a86f5b5d4dcc2 >>>>>>> "bridge: fix NULL pointer deref of br_port_get_rcu" >>>>>>> >>>>>>> Please apply to stable trees as well. Thanks. >>>>>>> >>>>>>> RH bugzilla reference: https://bugzilla.redhat.com/show_bug.cgi?id=1025770 >>>>>>> >>>>>>> Reported-by: Laine Stump <laine@redhat.com> >>>>>>> Debugged-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us> >>>>>>> --- >>>>>>> v1->v2: moved br_port_get_check_rcu definition below br_handle_frame definition >>>>>> >>>>>> Applied and queued up for -stable, thanks Jiri. >>>>> >>>>> How come you ignored my simpler fix, that used the existing logic. >>>>> I don't like introducing this especially into the stable; much prefer >>>>> to go back to testing the flag as was being done before. >>>> >>>> Although your patch is technically sane, it depends on rtnl indirectly. >>> >>> Pardon my ignorance, but I've been staring at this and I can't for >>> the life of me see the dependency. >>> >>> The IFF_BRIDGE_PORT flag is set after the rx_handler is registered, >>> so we are safe there. The rcu primitives will guarantee that the flag >>> will be set by the time rx_handler and rx_handler_data are set. >>> >>> The flag is cleared before rx_handler is unregistered, so it is >>> still valid to check for it in stp code. Once the flag is cleared >>> we may still have a valid rx_handler during the rcu grace period, but >>> will still avoid doing processing. >>> >>> So, where is the dependency on the rtnl? >> >> >> Imagine br would release the netdev and some other rx_handler user would >> enslave the same netdev. This two events would happen between >> IFF_BRIDGE_PORT flag check and rx_handler_data get. That is what >> rtnl_lock prevents from happening. > >I don't think this can happen. Inside br_stp_rcv(), we are in an rcu >critical section. The release of the netdev (rx_handler unregister) >forces us to to wait until synchronize_net() completes. So, if under >rcu we checked the flag and it's on, the rx_handler will not change for >the duration of that rcu section and we can safely process the packet >even if the port is in the middle of going away. Howe does the race >you describe happen? You are right. It cannot happen. > >The reason I ask, is that we check priv_flags under rcu in other >places to make sure that the device we are passing data to can handle >it. If it's not safe, then those other places are vulnerable too. >It doesn't seem to me that it is unsafe. > >Thanks >-vlad > >> >>> >>> Thanks >>> -vlad >>> >>>> My patch depends on rcu locking and synchronize_rcu which is direct. >>>> Therefore I think it is more appropriate. >>>> >>>> Jiri >>>> >>>> >>>> >>>> -- >>>> 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
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h index 229d820..045d56e 100644 --- a/net/bridge/br_private.h +++ b/net/bridge/br_private.h @@ -426,6 +426,16 @@ netdev_features_t br_features_recompute(struct net_bridge *br, int br_handle_frame_finish(struct sk_buff *skb); rx_handler_result_t br_handle_frame(struct sk_buff **pskb); +static inline bool br_rx_handler_check_rcu(const struct net_device *dev) +{ + return rcu_dereference(dev->rx_handler) == br_handle_frame; +} + +static inline struct net_bridge_port *br_port_get_check_rcu(const struct net_device *dev) +{ + return br_rx_handler_check_rcu(dev) ? br_port_get_rcu(dev) : NULL; +} + /* br_ioctl.c */ int br_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd); int br_ioctl_deviceless_stub(struct net *net, unsigned int cmd, diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c index 8660ea3..bdb459d 100644 --- a/net/bridge/br_stp_bpdu.c +++ b/net/bridge/br_stp_bpdu.c @@ -153,7 +153,7 @@ void br_stp_rcv(const struct stp_proto *proto, struct sk_buff *skb, if (buf[0] != 0 || buf[1] != 0 || buf[2] != 0) goto err; - p = br_port_get_rcu(dev); + p = br_port_get_check_rcu(dev); if (!p) goto err;