MIME-Version: 1.0
In-Reply-To: <lsq.1503062000.279019452@decadent.org.uk>
References: <lsq.1503061998.818387115@decadent.org.uk> <lsq.1503062000.279019452@decadent.org.uk>
From: Eric Dumazet <edumazet@google.com>
Date: Fri, 18 Aug 2017 06:53:16 -0700
Message-ID: <CANn89iKuQYk4RkmW8y08k+LO_-O+FMj9O-ySv5YF=vFsWc46+A@mail.gmail.com>
Subject: Re: [PATCH 3.16 119/134] ipv4: restore rt->fi for reference counting
To: Ben Hutchings <ben@decadent.org.uk>
Cc: LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        "David S. Miller" <davem@davemloft.net>,
        Andrey Konovalov <andreyknvl@google.com>,
        WANG Cong <xiyou.wangcong@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4178
Lines: 110

On Fri, Aug 18, 2017 at 6:13 AM, Ben Hutchings <ben@decadent.org.uk> wrote:
> 3.16.47-rc1 review patch.  If anyone has any objections, please let me know.
>
> ------------------
>
> From: WANG Cong <xiyou.wangcong@gmail.com>
>
> commit 82486aa6f1b9bc8145e6d0fa2bc0b44307f3b875 upstream.
>
> IPv4 dst could use fi->fib_metrics to store metrics but fib_info
> itself is refcnt'ed, so without taking a refcnt fi and
> fi->fib_metrics could be freed while dst metrics still points to
> it. This triggers use-after-free as reported by Andrey twice.
>
> This patch reverts commit 2860583fe840 ("ipv4: Kill rt->fi") to
> restore this reference counting. It is a quick fix for -net and
> -stable, for -net-next, as Eric suggested, we can consider doing
> reference counting for metrics itself instead of relying on fib_info.
>
> IPv6 is very different, it copies or steals the metrics from mx6_config
> in fib6_commit_metrics() so probably doesn't need a refcnt.
>
> Decnet has already done the refcnt'ing, see dn_fib_semantic_match().
>
> Fixes: 2860583fe840 ("ipv4: Kill rt->fi")
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Tested-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> [bwh: Backported to 3.16:
>  - Update all 5 places where rtable is initialised
>  - Open-code fib_info_hold()
>  - Adjust context]
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---

I thought we refined this later with :

commit 3fb07daff8e99243366a081e5129560734de4ada
Author: Eric Dumazet <edumazet@google.com>
Date:   Thu May 25 14:27:35 2017 -0700

    ipv4: add reference counting to metrics

    Andrey Konovalov reported crashes in ipv4_mtu()

    I could reproduce the issue with KASAN kernels, between
    10.246.7.151 and 10.246.7.152 :

    1) 20 concurrent netperf -t TCP_RR -H 10.246.7.152 -l 1000 &

    2) At the same time run following loop :
    while :
    do
     ip ro add 10.246.7.152 dev eth0 src 10.246.7.151 mtu 1500
     ip ro del 10.246.7.152 dev eth0 src 10.246.7.151 mtu 1500
    done

    Cong Wang attempted to add back rt->fi in commit
    82486aa6f1b9 ("ipv4: restore rt->fi for reference counting")
    but this proved to add some issues that were complex to solve.

    Instead, I suggested to add a refcount to the metrics themselves,
    being a standalone object (in particular, no reference to other objects)

    I tried to make this patch as small as possible to ease its backport,
    instead of being super clean. Note that we believe that only ipv4 dst
    need to take care of the metric refcount. But if this is wrong,
    this patch adds the basic infrastructure to extend this to other
    families.

    Many thanks to Julian Anastasov for reviewing this patch, and Cong Wang
    for his efforts on this problem.

    Fixes: 2860583fe840 ("ipv4: Kill rt->fi")
    Signed-off-by: Eric Dumazet <edumazet@google.com>
    Reported-by: Andrey Konovalov <andreyknvl@google.com>
    Reviewed-by: Julian Anastasov <ja@ssi.bg>
    Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>


And then :

commit 187e5b3ac84d3421d2de3aca949b2791fbcad554
Author: Eric Dumazet <edumazet@google.com>
Date:   Tue Aug 15 05:26:17 2017 -0700

    ipv4: fix NULL dereference in free_fib_info_rcu()

    If fi->fib_metrics could not be allocated in fib_create_info()
    we attempt to dereference a NULL pointer in free_fib_info_rcu() :

        m = fi->fib_metrics;
        if (m != &dst_default_metrics && atomic_dec_and_test(&m->refcnt))
                kfree(m);

    Before my recent patch, we used to call kfree(NULL) and nothing wrong
    happened.

    Instead of using RCU to defer freeing while we are under memory stress,
    it seems better to take immediate action.

    This was reported by syzkaller team.

    Fixes: 3fb07daff8e9 ("ipv4: add reference counting to metrics")
    Signed-off-by: Eric Dumazet <edumazet@google.com>
    Reported-by: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>