On Tue, Apr 30, 2019 at 10:45 AM Martin KaFai Lau <ka...@fb.com> wrote:
>
> It is a followup after the fix in
> commit 9c69a1320515 ("route: Avoid crash from dereferencing NULL rt->from")
>
> rt6_do_redirect():
> 1. NULL checking is needed on rt->from because a parallel
> fib6_info delete could happen that sets rt->from to NULL.
> (e.g. rt6_remove_exception() and fib6_drop_pcpu_from()).
>
> 2. fib6_info_hold() is not enough. Same reason as (1).
> Meaning, holding dst->__refcnt cannot ensure
> rt->from is not NULL or rt->from->fib6_ref is not 0.
>
> Instead of using fib6_info_hold_safe() which ip6_rt_cache_alloc()
> is already doing, this patch chooses to extend the rcu section
> to keep "from" dereference-able after checking for NULL.
>
> inet6_rtm_getroute():
> 1. NULL checking is also needed on rt->from for a similar reason.
> Note that inet6_rtm_getroute() is using RTNL_FLAG_DOIT_UNLOCKED.
>
> Fixes: a68886a69180 ("net/ipv6: Make from in rt6_info rcu protected")
> Signed-off-by: Martin KaFai Lau <ka...@fb.com>
> ---
Acked-by: Wei Wang <wei...@google.com>
Nice fix. Thanks Martin.
> net/ipv6/route.c | 38 ++++++++++++++++++--------------------
> 1 file changed, 18 insertions(+), 20 deletions(-)
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index b4899f0de0d0..73ef72c208af 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3397,11 +3397,8 @@ static void rt6_do_redirect(struct dst_entry *dst,
> struct sock *sk, struct sk_bu
>
> rcu_read_lock();
> from = rcu_dereference(rt->from);
> - /* This fib6_info_hold() is safe here because we hold reference to rt
> - * and rt already holds reference to fib6_info.
> - */
> - fib6_info_hold(from);
> - rcu_read_unlock();
> + if (!from)
> + goto out;
>
> nrt = ip6_rt_cache_alloc(from, &msg->dest, NULL);
> if (!nrt)
> @@ -3413,10 +3410,7 @@ static void rt6_do_redirect(struct dst_entry *dst,
> struct sock *sk, struct sk_bu
>
> nrt->rt6i_gateway = *(struct in6_addr *)neigh->primary_key;
>
> - /* No need to remove rt from the exception table if rt is
> - * a cached route because rt6_insert_exception() will
> - * takes care of it
> - */
> + /* rt6_insert_exception() will take care of duplicated exceptions */
> if (rt6_insert_exception(nrt, from)) {
> dst_release_immediate(&nrt->dst);
> goto out;
> @@ -3429,7 +3423,7 @@ static void rt6_do_redirect(struct dst_entry *dst,
> struct sock *sk, struct sk_bu
> call_netevent_notifiers(NETEVENT_REDIRECT, &netevent);
>
> out:
> - fib6_info_release(from);
> + rcu_read_unlock();
> neigh_release(neigh);
> }
>
> @@ -5028,16 +5022,20 @@ static int inet6_rtm_getroute(struct sk_buff *in_skb,
> struct nlmsghdr *nlh,
>
> rcu_read_lock();
> from = rcu_dereference(rt->from);
> -
> - if (fibmatch)
> - err = rt6_fill_node(net, skb, from, NULL, NULL, NULL, iif,
> - RTM_NEWROUTE, NETLINK_CB(in_skb).portid,
> - nlh->nlmsg_seq, 0);
> - else
> - err = rt6_fill_node(net, skb, from, dst, &fl6.daddr,
> - &fl6.saddr, iif, RTM_NEWROUTE,
> - NETLINK_CB(in_skb).portid, nlh->nlmsg_seq,
> - 0);
> + if (from) {
> + if (fibmatch)
> + err = rt6_fill_node(net, skb, from, NULL, NULL, NULL,
> + iif, RTM_NEWROUTE,
> + NETLINK_CB(in_skb).portid,
> + nlh->nlmsg_seq, 0);
> + else
> + err = rt6_fill_node(net, skb, from, dst, &fl6.daddr,
> + &fl6.saddr, iif, RTM_NEWROUTE,
> + NETLINK_CB(in_skb).portid,
> + nlh->nlmsg_seq, 0);
> + } else {
> + err = -ENETUNREACH;
> + }
> rcu_read_unlock();
>
> if (err < 0) {
> --
> 2.17.1
>
