Re: [PATCH 0/4] Fix routing metrics

[David Miller <davem@davemloft.net>]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Tue, 21 Feb 2012 09:18:27 +0100

> I need the dst->ops->metrics() method because I removed the direct
> reference to the inetpeer metrics from the dst_entry. I had to
> remove this direct reference to be able to free the old metrics
> safely. A dst_entry with a direct reference to old metrics
> could leave the rcu protected region and might then try to access
> already freed metrics (i.e. if a dst_entry with old metrics is already
> attached to a skb when the routing cache is flushed and the skb is queued
> for asynchronous processing). With this patchset we access the interpeer
> metrics via the inetpeer itself on every metrics access, so we ensure
> the metrics are not freed in the meantime.

Then a callback still seems like extreme overkill just to ensure the
RCU safety of metric pointer accesses.

It seems much simpler to me to just kill the inetpeer when we find out
we actually do need to change the metrics, instead of trying to change
the metric memory from underneath it.  Just make a new inetpeer and let
the old one with the old outdated metrics simply die off as the stray
references disappear.

Remove the old inetpeer from the tree (so it cannot be found in a
lookup), and then any dangling old, invalid, routing cache entries
referring to it will hold a reference count.  And once that final
reference drops, we'll know we can safely free the inetpeer up.  So
we'll use stale metrics for a while from these old invalid routing
cache entries, but that's perfectly fine.

The whole thing is about not doing something non-trivial in the fast
path, which is what your patch does.  Your approach causes us to incur
a cost every single metric access just because we "might" access stale
metrics from a old invalid routing cache entry.  This is not the
common case at all.