netdev - Re: [PATCH v2 net] inet: frags: rework rhashtable dismantle

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]

Date:   Tue, 2 Oct 2018 07:04:26 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Thomas Graf <tgraf@...g.ch>,
        Herbert Xu <herbert@...dor.apana.org.au>
Subject: Re: [PATCH v2 net] inet: frags: rework rhashtable dismantle

On Tue, Oct 2, 2018 at 6:46 AM Dmitry Vyukov <dvyukov@...gle.com> wrote:
>
> On Tue, Oct 2, 2018 at 3:16 PM, Eric Dumazet <edumazet@...gle.com> wrote:
> > On Tue, Oct 2, 2018 at 1:19 AM Dmitry Vyukov <dvyukov@...gle.com> wrote:
> >>
> >> On Tue, Oct 2, 2018 at 7:49 AM, Eric Dumazet <edumazet@...gle.com> wrote:
> >>
> >>
> >> Does inet_frag_kill() hold fq->lock? I am missing how inet_frag_kill()
> >> and inet_frags_exit_net() are synchronized.
> >> Since you use smp_store_release()/READ_ONCE() they seem to run in
> >> parallel. But then isn't it possible that inet_frag_kill() reads
> >> nf->dead == 0, then inet_frags_exit_net() sets nf->dead, and then we
> >> have the same race on concurrent removal? Or, isn't it possible that
> >> inet_frag_kill() reads nf->dead == 1, but does not set
> >> INET_FRAG_HASH_DEAD yet, and then inet_frags_free_cb() misses the
> >> INET_FRAG_HASH_DEAD flag?
> >>
> >
> > Yes this is kind of implied in my patch.
> > I put the smp_store_release() and READ_ONCE exactly to document the
> > possible races.
> > This was the reason for my attempt in V1, doing a walk, but Herbert
> > said walk was not designed for doing deletes.
> >
> > Proper synch will need a synchronize_rcu(), and thus a future
> > conversion in net-next because we can not really
> > add new synchronize_rcu() calls in an (struct
> > pernet_operations.)exit() without considerable performance hit of
> > netns dismantles.
> >
> > So this will require a conversion of all inet_frags_exit_net() callers
> > to .exit_batch() to mitigate the cost.
> >
> > I thought of synchronize_rcu_bh() but this beast is going away soon anyway.
>
> But if this patch allows all the same races and corruptions, then
> what's the point?

Not really. The current races can last dozen of seconds, if youu have
one million frags.

With the fix, the race is in the order of one usec on typical hosts.