netdev - Re: [PATCH net] ipv4: add reference counting to metrics

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: <CAM_iQpWDnNz5u9Jtk0MAywNs=N4F8ogvT3bRji3vYj1Tfk830g@mail.gmail.com>
Date:   Fri, 26 May 2017 10:08:34 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Andrey Konovalov <andreyknvl@...gle.com>,
        Julian Anastasov <ja@....bg>, netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] ipv4: add reference counting to metrics

On Thu, May 25, 2017 at 2:27 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> From: Eric Dumazet <edumazet@...gle.com>
>
> 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@...gle.com>
> Reported-by: Andrey Konovalov <andreyknvl@...gle.com>

Nice work!! Thanks for your effort of making it small!

Just one nit below.

> -const u32 dst_default_metrics[RTAX_MAX + 1] = {
> +const struct dst_metrics dst_default_metrics = {
>         /* This initializer is needed to force linker to place this variable
>          * into const section. Otherwise it might end into bss section.
>          * We really want to avoid false sharing on this variable, and catch
>          * any writes on it.
>          */
> -       [RTAX_MAX] = 0xdeadbeef,
> +       .refcnt = ATOMIC_INIT(1),
>  };

The code comment above is no longer needed since
we have to initialize refcnt to 1, instead of merely for const
section.

Anyway,

Acked-by: Cong Wang <xiyou.wangcong@...il.com>