netdev - Re: [PATCH net] ipv6: do not overwrite inetpeer metrics prematurely

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]

Message-Id: <20140307.155258.463380957582451300.davem@davemloft.net>
Date:	Fri, 07 Mar 2014 15:52:58 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	mkubecek@...e.cz
Cc:	netdev@...r.kernel.org, kuznet@....inr.ac.ru, jmorris@...ei.org,
	yoshfuji@...ux-ipv6.org, kaber@...sh.net
Subject: Re: [PATCH net] ipv6: do not overwrite inetpeer metrics prematurely

From: Michal Kubecek <mkubecek@...e.cz>
Date: Thu, 6 Mar 2014 21:06:58 +0100

> On Thu, Mar 06, 2014 at 02:24:25PM -0500, David Miller wrote:
>> 
>> > +static void rt6_metrics_to_peer(struct rt6_info *rt)
>> > +{
>> > +	struct inet_peer *peer = rt6_has_peer(rt) ? rt6_peer_ptr(rt) : NULL;
>> > +	struct dst_entry *dst = &rt->dst;
>> > +	unsigned long old = dst->_metrics;
>> > +
>> > +	if (!(rt->dst.flags & DST_HOST) || dst_metrics_read_only(dst))
>> > +		return;
>> > +	if (peer && dst_metrics_ptr(dst) == peer->metrics)
>> > +		return;
>> > +
>> > +	dst->ops->cow_metrics(dst, old);
>> > +	if (dst->_metrics != old) {
>> > +		u32 *old_p = __DST_METRICS_PTR(old);
>> > +
>> > +		memcpy(dst_metrics_ptr(dst), old_p, RTAX_MAX * sizeof(u32));
>> > +		kfree(old_p);
>> > +	}
>> > +}
>> 
>> Hmmm...  if inet_metrics_new() is true then ->cow_metrics() will copy the
>> metrics from old to new.  So you therefore shouldn't have to do the copy
>> explicitly here.
> 
> If we are replacing an existing host route with metrics, e.g.
> 
>   ip route add fec0::1 dev eth0 rto_min 1000
>   ip route change fec0::1 dev eth0 rto_min 1500
> 
> then peer will be the existing inetpeer and inet_metrics_new() will be
> false. However, we still need to copy the new metrics from the netlink
> message over the old ones.
> 
>> If inet_metrics_new() is not true, you are overwriting non-new metrics.
> 
> The only problem with this is IMHO that if inet_metrics_new() is true,
> i.e. when adding a new route with new inetpeer (or old inetpeer whose
> metrics were not used before), the memcpy() is done twice, once in
> ipv6_cow_metrics() and once in rt6_metrics_to_peer(). We are copying the
> same data twice so that the result is correct but it's not efficient and
> it's not nice.
> 
> The only way I can see to avoid this (except using own metrics always
> instead of those in struct inetpeer as we do for non-host routes) would
> be not to call ipv6_cow_metrics() at all and write a special function
> for this purpose in net/ipv6/route.c which would duplicate the parts of
> ipv6_cow_metrics() we really need (and add the free()). Do you think
> this is the way to go?

Thank you for explaining all of this, I would like to think about this
some more.

My initial suspicion is that the something about the test in cow
metrics might need to be adjusted.

The conceptual attributes we have built for inetpeer metrics, that of
"newness" and "read-only", might not be built adequately for the task
at hand here.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html