On 3/27/19 4:52 PM, Alexei Starovoitov wrote:
>> +
>> + /* 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.
>
>> + 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.
