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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Mar 2019 18:47:23 -0600
From:   David Ahern <dsahern@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        David Ahern <dsahern@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, idosch@...lanox.com,
        jiri@...lanox.com, saeedm@...lanox.com
Subject: Re: [PATCH v2 net-next 05/13] ipv6: Create init helper for fib6_nh

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.