Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\))
Subject: Re: [PATCH v2] rcu: Add necessary WRITE_ONCE()
From:   Alan Huang <mmpgouride@gmail.com>
In-Reply-To: <2fd1169a-a695-4bff-9611-a84dd02025b2@paulmck-laptop>
Date:   Sat, 24 Jun 2023 02:31:12 +0800
Cc:     rcu@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <C75370D0-3A0C-48AA-AF20-ABCD74969F11@gmail.com>
References: <20230620171346.207076-1-mmpgouride@gmail.com>
 <50c4aa37-388b-449c-8184-00a9d69471fc@paulmck-laptop>
 <B9A94CB4-DB9B-43D2-9D34-ADB4F7EA737D@gmail.com>
 <2fd1169a-a695-4bff-9611-a84dd02025b2@paulmck-laptop>
To:     paulmck@kernel.org, will@kernel.org
Precedence: bulk


> 2023=E5=B9=B46=E6=9C=8823=E6=97=A5 =E4=B8=8B=E5=8D=881:17=EF=BC=8CPaul =
E. McKenney <paulmck@kernel.org> =E5=86=99=E9=81=93=EF=BC=9A
>=20
> On Wed, Jun 21, 2023 at 10:08:28AM +0800, Alan Huang wrote:
>>=20
>>> 2023=E5=B9=B46=E6=9C=8821=E6=97=A5 06:26=EF=BC=8CPaul E. McKenney =
<paulmck@kernel.org> =E5=86=99=E9=81=93=EF=BC=9A
>>>=20
>>> On Tue, Jun 20, 2023 at 05:13:46PM +0000, Alan Huang wrote:
>>>> Commit c54a2744497d("list: Add hlist_unhashed_lockless()") and
>>>> commit 860c8802ace1("rcu: Use WRITE_ONCE() for assignments to
>>>> ->pprev for hlist_nulls") added various WRITE_ONCE() to pair with
>>>> the READ_ONCE() in hlist_unhashed_lockless(), but there are still
>>>> some places where WRITE_ONCE() was not added, this commit adds =
that.
>>>>=20
>>>> Also add WRITE_ONCE() to pair with the READ_ONCE() in =
hlist_empty().
>>>>=20
>>>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>>>=20
>>> On hlist_nulls_add_tail_rcu(), good catch, thank you!
>>>=20
>>> On the others, are there really cases where a lockless read races =
with
>>> the update?  At first glance, that sounds like a usage bug.  For =
example,
>>> as I understand it, when you use something like hlist_del(), you are
>>> supposed to ensure that there are no concurrent readers.  Which is =
the
>>> point of the assignment of the special value LIST_POISON2, right?
>>=20
>> Do you mean there are cases where a lockless read races with =
hlist_add_head/hlist_add_before
>> hlist_add_behind/__hlist_del, but there is no real case where a =
lockless read races with the hlist_del_init/hlist_del
>> hlist_move_list?
>>=20
>> There may be no real case where a lockless read races with the =
hlist_del_init/hlist_del
>> hlist_move_list. But for the sake of completeness, I added those =
WRITE_ONCE, after all, if there is WRITE_ONCE
>> in __hlist_del, why not add WRITE_ONCE in its caller, like =
hlist_del()?
>=20
> You might well have located a larger issue.  We want to be able to use
> KCSAN to find unintended data races, but as you noted, there might
> be different requirements for RCU-protected linked lists and for
> lock-protected linked lists.  If there are, then there is probably
> existing linked-list code that is using the wrong primitive, for
> example, using (or failing to use) the one that Eric Dumazet provided.
> For example, mismatched API usage might be causing the differences in
> uses of _ONCE() primitives that you are calling out.

I noticed a thread:

https://lore.kernel.org/lkml/20200324153643.15527-2-will@kernel.org/

It seems like Will wanted to remove that hlist_unhashed_lockless()?
But I can=E2=80=99t find any further updates.

Will: Can you tell me what happened later?

>=20
> Would you be interested in digging into this?

I=E2=80=99d like to.

>=20
> You will of course need to be able to build and run kernels with KCSAN
> enabled, which is not hard to do given a laptop that can build a =
kernel
> and run a guest OS.

I=E2=80=99ll do that, :)

>=20
> 							Thanx, Paul
>=20
>> Thanks,
>> Alan
>>=20
>>>=20
>>> Or is there some use case that I am missing?
>>>=20
>>> If I am not missing something, then switching the non-RCU APIs to
>>> WRITE_ONCE() would be a step backwards, because it would make it =
harder
>>> for tools like KCSAN to find bugs.
>>>=20
>>> Thanx, Paul
>>>=20
>>>> ---
>>>> Changelog:
>>>> V1 -> V2:=20
>>>> Add WRITE_ONCE in hlist_del_init to pair with READ_ONCE in
>>>> hlist_unhashed_lockless.
>>>>=20
>>>> include/linux/list.h          | 9 +++++----
>>>> include/linux/list_nulls.h    | 2 +-
>>>> include/linux/rculist_nulls.h | 2 +-
>>>> 3 files changed, 7 insertions(+), 6 deletions(-)
>>>>=20
>>>> diff --git a/include/linux/list.h b/include/linux/list.h
>>>> index ac366958ea..3a29b95bfe 100644
>>>> --- a/include/linux/list.h
>>>> +++ b/include/linux/list.h
>>>> @@ -912,7 +912,7 @@ static inline void hlist_del(struct hlist_node =
*n)
>>>> {
>>>> __hlist_del(n);
>>>> n->next =3D LIST_POISON1;
>>>> - n->pprev =3D LIST_POISON2;
>>>> + WRITE_ONCE(n->pprev, LIST_POISON2);
>>>> }
>>>>=20
>>>> /**
>>>> @@ -925,7 +925,8 @@ static inline void hlist_del_init(struct =
hlist_node *n)
>>>> {
>>>> if (!hlist_unhashed(n)) {
>>>> __hlist_del(n);
>>>> - INIT_HLIST_NODE(n);
>>>> + n->next =3D NULL;
>>>> + WRITE_ONCE(n->pprev, NULL);
>>>> }
>>>> }
>>>>=20
>>>> @@ -1026,8 +1027,8 @@ static inline void hlist_move_list(struct =
hlist_head *old,
>>>> {
>>>> new->first =3D old->first;
>>>> if (new->first)
>>>> - new->first->pprev =3D &new->first;
>>>> - old->first =3D NULL;
>>>> + WRITE_ONCE(new->first->pprev, &new->first);
>>>> + WRITE_ONCE(old->first, NULL);
>>>> }
>>>>=20
>>>> #define hlist_entry(ptr, type, member) =
container_of(ptr,type,member)
>>>> diff --git a/include/linux/list_nulls.h =
b/include/linux/list_nulls.h
>>>> index fa6e8471bd..b63b0589fa 100644
>>>> --- a/include/linux/list_nulls.h
>>>> +++ b/include/linux/list_nulls.h
>>>> @@ -95,7 +95,7 @@ static inline void hlist_nulls_add_head(struct =
hlist_nulls_node *n,
>>>>=20
>>>> n->next =3D first;
>>>> WRITE_ONCE(n->pprev, &h->first);
>>>> - h->first =3D n;
>>>> + WRITE_ONCE(h->first, n);
>>>> if (!is_a_nulls(first))
>>>> WRITE_ONCE(first->pprev, &n->next);
>>>> }
>>>> diff --git a/include/linux/rculist_nulls.h =
b/include/linux/rculist_nulls.h
>>>> index ba4c00dd80..c65121655b 100644
>>>> --- a/include/linux/rculist_nulls.h
>>>> +++ b/include/linux/rculist_nulls.h
>>>> @@ -138,7 +138,7 @@ static inline void =
hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
>>>>=20
>>>> if (last) {
>>>> n->next =3D last->next;
>>>> - n->pprev =3D &last->next;
>>>> + WRITE_ONCE(n->pprev, &last->next);
>>>> rcu_assign_pointer(hlist_nulls_next_rcu(last), n);
>>>> } else {
>>>> hlist_nulls_add_head_rcu(n, h);
>>>> --=20
>>>> 2.34.1