netdev - Re: [Patch net] rds: mark bound socket with SOCK_RCU

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_iQpXcydKdTyT25F5Bbk7y9BMJLTKMT42o3J0bbuodjXjetA@mail.gmail.com>
Date:   Mon, 10 Sep 2018 17:16:21 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:     Santosh Shilimkar <santosh.shilimkar@...cle.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        rds-devel@....oracle.com
Subject: Re: [Patch net] rds: mark bound socket with SOCK_RCU_FREE

On Mon, Sep 10, 2018 at 5:04 PM Sowmini Varadhan
<sowmini.varadhan@...cle.com> wrote:
>
> On (09/10/18 16:51), Cong Wang wrote:
> >
> >         __rds_create_bind_key(key, addr, port, scope_id);
> > -       rs = rhashtable_lookup_fast(&bind_hash_table, key, ht_parms);
> > +       rcu_read_lock();
> > +       rs = rhashtable_lookup(&bind_hash_table, key, ht_parms);
> >         if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
> >                 rds_sock_addref(rs);
> >         else
> >                 rs = NULL;
> > +       rcu_read_unlock();
>
> aiui, the rcu_read lock/unlock is only useful if the write
> side doing destructive operations  does something to make sure readers
> are done before doing the destructive opertion. AFAIK, that does
> not exist for rds socket management today

That is exactly why we need it here, right?

As you mentioned previously, the sock could be freed after
rhashtable_lookup_fast() but before rds_sock_addref(), extending
the RCU read section after rds_sock_addref() exactly solves
the problem here.

Or is there any other destructive problem you didn't mention?
Mind to be specific?


>
>
> > Although sock release path is not a very hot path, but blocking
> > it isn't a good idea either, especially when you can use call_rcu(),
> > which has the same effect.
> >
> > I don't see any reason we should prefer synchronize_rcu() here.
>
> Usually correctness (making sure all readers are done, before nuking a
> data structure) is a little bit more important than perforamance, aka
> "safety before speed" is what I've always been taught.
>

Hmm, so you are saying synchronize_rcu() is kinda more correct
than call_rcu()?? I never hear this before, would like to know why.

To my best knowledge, the only difference between them is the context,
one is blocking, the other is non-blocking. Their correctness must be
equivalent.