Re: [PATCH v2 net-next 05/13] ipv6: Create init helper for fib6_nh

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Mar 27, 2019 at 06:47:23PM -0600, David Ahern wrote:
> 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.

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

> > 
> >> +		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?