On Wed, Mar 27, 2019 at 06:47:23PM -0600, David Ahern wrote:

On 3/27/19 4:52 PM, Alexei Starovoitov wrote:

quoted

quoted

+
+	/* We cannot add true routes via loopback here,
+	 * they would result in kernel looping; promote them to reject routes
+	 */
+	addr_type = ipv6_addr_type(&cfg->fc_dst);
+	if ((cfg->fc_flags & RTF_REJECT) ||
+	    (dev && (dev->flags & IFF_LOOPBACK) &&
+	     !(addr_type & IPV6_ADDR_LOOPBACK) &&
+	     !(cfg->fc_flags & RTF_LOCAL))) {
+		/* hold loopback dev/idev if we haven't done so. */
+		if (dev != net->loopback_dev) {
+			if (dev) {
+				dev_put(dev);
+				in6_dev_put(idev);
+			}
+			dev = net->loopback_dev;
+			dev_hold(dev);
+			idev = in6_dev_get(dev);
+			if (!idev) {
+				err = -ENODEV;
+				goto out;
+			}
+		}
+		cfg->fc_flags = RTF_REJECT | RTF_NONEXTHOP;

imo it would be cleaner not to mess with cfg.
Ideally it should be marked 'const'.

Existing code sets those flags but on a fib6_info. This is not used for
nexthop objects and is kept here to not duplicate this if branch in the
create_info that uses it. This check affects both which device is used
as well as the flags.

What stopping you from doing fib6_nh->nh_flags |= RTF_REJECT | RTF_NONEXTHOP ?
cfg should really be const.

quoted

quoted

+		goto set_dev;
+	}
+
+	if (cfg->fc_flags & RTF_GATEWAY) {
+		err = ip6_validate_gw(net, cfg, &dev, &idev, extack);
+		if (err)
+			goto out;
+
+		fib6_nh->nh_gw = cfg->fc_gateway;
+	}
+
+	err = -ENODEV;
+	if (!dev)
+		goto out;
+
+	if (idev->cnf.disable_ipv6) {
+		NL_SET_ERR_MSG(extack, "IPv6 is disabled on nexthop device");
+		err = -EACCES;
+		goto out;
+	}
+
+	if (!(dev->flags & IFF_UP) && !cfg->fc_ignore_dev_down) {
+		NL_SET_ERR_MSG(extack, "Nexthop device is not up");
+		err = -ENETDOWN;
+		goto out;
+	}
+
+	if (!(cfg->fc_flags & (RTF_LOCAL | RTF_ANYCAST)) &&
+	    !netif_carrier_ok(dev))
+		fib6_nh->nh_flags |= RTNH_F_LINKDOWN;
+
+set_dev:
+	fib6_nh->nh_dev = dev;
+	err = 0;
+out:
+	if (idev)
+		in6_dev_put(idev);
+
+	if (err) {
+		lwtstate_put(fib6_nh->nh_lwtstate);

lwtstate_put() is missing in the error path of existing code.
Is this a bug fix?
Why there is nothing about this in commit log?

Existing code has a different cleanup path.

This is done explicitly here per a request from Ido in v1 that the new
function be symmetric in its cleanup on an error.

I saw that comment.
What I don't see is where existing code doing that cleanup.
Could you please point it out?