On 29/11/15 03:38, Roopa Prabhu wrote:
From: Roopa Prabhu <[email protected]>
}
}
- nh_index = hash % rt->rt_nhn;
+ return hash;
+}
+
+static struct mpls_nh *mpls_select_multipath(struct mpls_route *rt,
+ struct sk_buff *skb, bool bos)
+{
+ u32 hash = 0;
+ int nh_index = 0;
+ int n = 0;
+
+ /* No need to look further into packet if there's only
+ * one path
+ */
+ if (rt->rt_nhn == 1)
+ goto out;
+
+ if (rt->rt_nhn_alive <= 0)
+ return NULL;
+
+ hash = mpls_multipath_hash(rt, skb, bos);
+ nh_index = hash % rt->rt_nhn_alive;
There's a possibility that the compiler could generate multiple reads to
rt_nhn_alive and thus see partial updates. If this happens, then we
could end up accessing a nexthop pointer that is actually beyond the end
of the nexthop array.
I don't think any form of memory barrier is necessary so I would
therefore suggest fixing this by changing the line above to:
nh_index = hash % ACCESS_ONCE(rt->rt_nhn_alive);
If we assume that it's OK to drop packets for a short time around the
change, then the rt->rt_nhn_alive <= 0 check above is fine as is.
Similarly if we assume that it's OK for packets to go via nexthops they
wouldn't normally do transiently then the rt->rt_nhn_alive == rt->rt_nhn
check below is also fine as is.
However, it might look confusing to a casual observer, so perhaps we
should consider stashing the alive nexthop count in a variable, still
getting it using the ACCESS_ONCE macro?
+ if (rt->rt_nhn_alive == rt->rt_nhn)
+ goto out;
+ for_nexthops(rt) {
+ if (nh->nh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN))
+ continue;
+ if (n == nh_index)
+ return nh;
+ n++;
+ } endfor_nexthops(rt);
+
out:
return &rt->rt_nh[nh_index];
}
...
return ERR_PTR(err);
}
-static void mpls_ifdown(struct net_device *dev)
+static void mpls_ifdown(struct net_device *dev, int event)
{
struct mpls_route __rcu **platform_label;
struct net *net = dev_net(dev);
- struct mpls_dev *mdev;
unsigned index;
platform_label = rtnl_dereference(net->mpls.platform_label);
for (index = 0; index < net->mpls.platform_labels; index++) {
struct mpls_route *rt = rtnl_dereference(platform_label[index]);
+
if (!rt)
continue;
- for_nexthops(rt) {
+
+ change_nexthops(rt) {
if (rtnl_dereference(nh->nh_dev) != dev)
continue;
- nh->nh_dev = NULL;
+ switch (event) {
+ case NETDEV_DOWN:
+ case NETDEV_UNREGISTER:
+ nh->nh_flags |= RTNH_F_DEAD;
+ /* fall through */
+ case NETDEV_CHANGE:
+ nh->nh_flags |= RTNH_F_LINKDOWN;
+ rt->rt_nhn_alive--;
For the similar reasons as above, to prevent mpls_select_multipath
seeing partial updates I think this should be:
ACCESS_ONCE(rt->rt_nhn_alive) = rt->rt_nhn_alive - 1;
Again, I don't think any memory barrier is required here, but I could be
mistaken.
No special treatment of nh->nh_flags is required if we assume that it's
OK transiently for packets to be dropped or go via a different nexthop
than in steady state.
+ break;
+ }
+ if (event == NETDEV_UNREGISTER)
+ RCU_INIT_POINTER(nh->nh_dev, NULL);
} endfor_nexthops(rt);
}
- mdev = mpls_dev_get(dev);
- if (!mdev)
- return;
- mpls_dev_sysctl_unregister(mdev);
+ return;
+}
+
+static void mpls_ifup(struct net_device *dev, unsigned int nh_flags)
+{
+ struct mpls_route __rcu **platform_label;
+ struct net *net = dev_net(dev);
+ unsigned index;
+ int alive;
+
+ platform_label = rtnl_dereference(net->mpls.platform_label);
+ for (index = 0; index < net->mpls.platform_labels; index++) {
+ struct mpls_route *rt = rtnl_dereference(platform_label[index]);
+
+ if (!rt)
+ continue;
+
+ alive = 0;
+ change_nexthops(rt) {
+ struct net_device *nh_dev =
+ rtnl_dereference(nh->nh_dev);
+
+ if (!(nh->nh_flags & nh_flags)) {
+ alive++;
+ continue;
+ }
+ if (nh_dev != dev)
+ continue;
+ alive++;
+ nh->nh_flags &= ~nh_flags;
+ } endfor_nexthops(rt);
- RCU_INIT_POINTER(dev->mpls_ptr, NULL);
+ rt->rt_nhn_alive = alive;
For the similar reasons as above, to prevent mpls_select_multipath
seeing partial updates I think this should be:
ACCESS_ONCE(rt->rt_nhn_alive) = alive;
Again, I don't think any memory barrier is required here, but I could be
mistaken.
+ }
- kfree_rcu(mdev, rcu);
+ return;
}
static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
