Mon, Dec 09, 2013 at 12:58:35PM CET, mst@xxxxxxxxxx wrote: >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@xxxxxxxxxx> >> Debugged-by: Michael S. Tsirkin <mst@xxxxxxxxxx> >> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx> >> Signed-off-by: Jiri Pirko <jiri@xxxxxxxxxxx> >> --- >> 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; > >Actually this started to bother me. >rcu_dereference is for when we dereference, isn't it? >I think we should use rcu_access_pointer here. Yes. That can be done. That would safe a barrier on some archs. > > >> +} > > >Given all the confusion, how about we create an API to >access rx handler data outside rx handler itself in a >safe, documented way? > >If everyone agrees, we can then re-implement >br_port_get_check_rcu on top of this API. > >What do others think? I like this a lot. Acked-by: Jiri Pirko <jiri@xxxxxxxxxxx> > >--- > >netdevice: allow access to rx_handler_data outside rx handler > >rx_handler_data is easy to use correctly within >rx handler itself. Outside of that context, one must >validate the handler first. > >Add an API to do this in a uniform way. > >Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx> > >--> > >diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >index 7f0ed42..7a353b1 100644 >--- a/include/linux/netdevice.h >+++ b/include/linux/netdevice.h >@@ -1320,6 +1320,9 @@ struct net_device { > #endif > > rx_handler_func_t __rcu *rx_handler; >+ /* rx_handler itself can use rx_handler_data directly. >+ * Others must use netdev_rx_handler_data_rcu_dereference. >+ */ > void __rcu *rx_handler_data; > > struct netdev_queue __rcu *ingress_queue; >@@ -2399,6 +2402,31 @@ int netdev_rx_handler_register(struct net_device *dev, > void *rx_handler_data); > void netdev_rx_handler_unregister(struct net_device *dev); > >+/** >+ * netdev_rx_handler_data_rcu_dereference - access receive handler data >+ * @dev: device to get handler data for >+ * @rx_handler: receive handler used to register this data >+ * >+ * Check that the receive handler is valid for the device. >+ * Return handler data if it is, NULL otherwise. >+ * >+ * Use this function if you want to access rx handler data >+ * outside rx handler itself. >+ * >+ * The caller must invoke this function under RCU read lock. >+ * >+ * For a general description of rx_handler, see enum rx_handler_result. >+ */ >+static inline >+void *netdev_rx_handler_data_rcu_dereference(struct net_device *dev, >+ rx_handler_func_t *rx_handler) >+{ >+ if (rcu_access_pointer(dev->rx_handler) != rx_handler) >+ return NULL; >+ >+ return rcu_dereference(dev->rx_handler_data); >+} >+ > bool dev_valid_name(const char *name); > int dev_ioctl(struct net *net, unsigned int cmd, void __user *); > int dev_ethtool(struct net *net, struct ifreq *);
