On 29/11/15 03:38, Roopa Prabhu wrote:

From: Roopa Prabhu <redacted>
  		}
  	}

-	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,