netdev - Re: [PATCH] inet: frags: Remove unnecessary smp_store_release/READ

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+Y39u9VL+C27PVpfVZbNP_U8yFG35yLy6_KaxK2+Z9Gyw@mail.gmail.com>
Date:   Wed, 29 May 2019 07:43:51 +0200
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH] inet: frags: Remove unnecessary smp_store_release/READ_ONCE

On Wed, May 29, 2019 at 7:40 AM Herbert Xu <herbert@...dor.apana.org.au> wrote:
>
> On Tue, May 28, 2019 at 06:31:00AM -0700, Eric Dumazet wrote:
> >
> > This smp_store_release() is a left over of the first version of the patch, where
> > there was no rcu grace period enforcement.
> >
> > I do not believe there is harm letting this, but if you disagree
> > please send a patch ;)
>
> I see now that it is actually relying on the barrier/locking
> semantics of call_rcu vs. rcu_read_lock.  So the smp_store_release
> and READ_ONCE are simply unnecessary and could be confusing to
> future readers.
>
> ---8<---
> The smp_store_release call in fqdir_exit cannot protect the setting
> of fqdir->dead as claimed because its memory barrier is only
> guaranteed to be one-way and the barrier precedes the setting of
> fqdir->dead.
>
> IOW it doesn't provide any barriers between fq->dir and the following
> hash table destruction.
>
> In fact, the code is safe anyway because call_rcu does provide both
> the memory barrier as well as a guarantee that when the destruction
> work starts executing all RCU readers will see the updated value for
> fqdir->dead.
>
> Therefore this patch removes the unnecessary smp_store_release call
> as well as the corresponding READ_ONCE on the read-side in order to
> not confuse future readers of this code.  Comments have been added
> in their places.
>
> Signed-off-by: Herbert Xu <herbert@...dor.apana.org.au>
>
> diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
> index 2b816f1ebbb4..35e9784fab4e 100644
> --- a/net/ipv4/inet_fragment.c
> +++ b/net/ipv4/inet_fragment.c
> @@ -193,10 +193,12 @@ void fqdir_exit(struct fqdir *fqdir)
>  {
>         fqdir->high_thresh = 0; /* prevent creation of new frags */
>
> -       /* paired with READ_ONCE() in inet_frag_kill() :
> -        * We want to prevent rhashtable_remove_fast() calls
> +       fqdir->dead = true;
> +
> +       /* call_rcu is supposed to provide memory barrier semantics,
> +        * separating the setting of fqdir->dead with the destruction
> +        * work.  This implicit barrier is paired with inet_frag_kill().
>          */
> -       smp_store_release(&fqdir->dead, true);
>
>         INIT_RCU_WORK(&fqdir->destroy_rwork, fqdir_rwork_fn);
>         queue_rcu_work(system_wq, &fqdir->destroy_rwork);
> @@ -214,10 +216,12 @@ void inet_frag_kill(struct inet_frag_queue *fq)
>
>                 fq->flags |= INET_FRAG_COMPLETE;
>                 rcu_read_lock();
> -               /* This READ_ONCE() is paired with smp_store_release()
> -                * in inet_frags_exit_net().
> +               /* The RCU read lock provides a memory barrier
> +                * guaranteeing that if fqdir->dead is false then
> +                * the hash table destruction will not start until
> +                * after we unlock.  Paired with inet_frags_exit_net().
>                  */
> -               if (!READ_ONCE(fqdir->dead)) {
> +               if (!fqdir->dead) {

If fqdir->dead read/write are concurrent, then this still needs to be
READ_ONCE/WRITE_ONCE. Ordering is orthogonal to atomicity.

>                         rhashtable_remove_fast(&fqdir->rhashtable, &fq->node,
>                                                fqdir->f->rhash_params);
>                         refcount_dec(&fq->refcnt);