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?
