On 22/10/15 14:33, roopa wrote:
On 10/22/15, 3:24 AM, Robert Shearman wrote:
On 19/10/15 02:20, Roopa Prabhu wrote:
return dev;
}
+static int mpls_nh_assign_dev(struct net *net, struct mpls_nh *nh, int oif)
+{
+ struct net_device *dev = NULL;
+ int err = -ENODEV;
+
+ dev = find_outdev(net, nh, oif);
+ if (IS_ERR(dev)) {
+ err = PTR_ERR(dev);
+ dev = NULL;
+ goto errout;
+ }
+
+ /* Ensure this is a supported device */
+ err = -EINVAL;
+ if (!mpls_dev_get(dev))
+ goto errout;
+
+ /* For now just support ethernet devices */
+ if ((dev->type != ARPHRD_ETHER) && (dev->type != ARPHRD_LOOPBACK))
+ goto errout;
+
+ RCU_INIT_POINTER(nh->nh_dev, dev);
+ dev_put(dev);
If it's safe to not keep a reference to dev (i.e. because the device cannot go
away while a netlink message is being processed), then why not just change
find_outdev to use __dev_get_by_index?
yes, rtnl is held here. dev_get_by_index was used by the original code. So i
did not feel the need to change that.
I can see the need to get away with the dev_hold instead of doing a dev_hold
with dev_get_by_index, rcu assign and put because of rtnl. But, it seems
cleaner to use it the way it is today, given the other cases where the dev is
already held during lookup when oif is not specified.
If the dev can go away during this, then we could end up adding an mpls route
that points to a freed interface, because until we've added the route to the
label table mpls_ifdown won't find it.
dev should not go away because we have rtnl held at this point. no ?
If there is a problem i can think about moving the dev_put's for nexthop devs
after mpls_route_update
Yes, either do that or change find_outdev to avoid holding a reference
to the dev (which given that it's safe is my preference). The way it is
here is slightly confusing as it raises the question in the reader's
mind about whether a reference to dev is required or not.
Thanks,
Rob
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
