netdev - Re: [PATCH] net: Fix suspicious RCU usage in bpf_sk_reuseport

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: <20220816214923.x57nkwgm6rrhkoyb@kafai-mbp>
Date:   Tue, 16 Aug 2022 14:49:23 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     David Howells <dhowells@...hat.com>
Cc:     Hawkins Jiawei <yin31149@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org, bpf@...r.kernel.org
Subject: Re: [PATCH] net: Fix suspicious RCU usage in
 bpf_sk_reuseport_detach()

On Tue, Aug 16, 2022 at 02:09:46PM +0100, David Howells wrote:
> Hawkins Jiawei <yin31149@...il.com> wrote:
> 
> >  	if (socks) {
> >  		WRITE_ONCE(sk->sk_user_data, NULL);
> 
> Btw, shouldn't this be rcu_assign_pointer() or RCU_INIT_POINTER(), not
> WRITE_ONCE()?
It is not necessary.  The sk_user_data usage in reuseport_array
is protected by the sk_callback_lock alone.  The code
before the commit cf8c1e967224 is fine.  If the
__rcu_dereference_sk_user_data_with_flags() could be reused here as is,
an extra rcu_dereference is fine, so I did not mention it.
It seems it is not the case and new function naming is getting long,
so how about reverting the commit cf8c1e967224 and keep it as it was.