Re: [PATCH] fs_pin: publish pins with RCU list helpers

[Jan Kara]

On Thu 03-09-26 02:21:00, Karl Mehltretter wrote:
> pin_insert() initializes a pin before adding it under pin_lock.  The kill
> paths read the list heads under RCU without taking that lock.  Plain hlist
> insertion does not publish the earlier initialization to those readers.
> 
> KCSAN weak-memory checking reported acct_on() callback initialization
> racing with pin_kill() after the superblock-list publication.  The
> corresponding LKMM model permits the stale read with plain publication;
> the RCU release/acquire pair forbids it.
> 
> Use the RCU hlist add and dereference helpers for both pin lists.
> 
> Fixes: 2798d4ce6160 ("acct: get rid of acct_lock for acct->count")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@xxxxxxxxx>

Yes, it looks like the memory ordering is indeed insufficient here. Feel
free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
> Reviewer notes:
> 
> Tested on arm64 (Raspberry Pi 400, Cortex-A72) at cf72cbb39da8 with clang,
> strict KCSAN, and weak-memory modeling.  Concurrent acct(path) and
> read-only remounts produced one __arm64_sys_acct()/pin_kill() report in
> the baseline.  Disassembly identified both accesses as pin->kill and the
> write as modeled past pin_insert().
> 
> With this patch, the report was absent at five times the baseline
> exposure, while unrelated control races continued to fire.  The LKMM
> plain-publication test is "Sometimes"; its RCU counterpart is "Never".
> 
> This demonstrates modeled reordering, not an observed crash.  Plain KCSAN
> also found no report in about 400,000 acct() cycles.
> 
>  fs/fs_pin.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> --- a/fs/fs_pin.c
> +++ b/fs/fs_pin.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <linux/fs.h>
> +#include <linux/rculist.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include "internal.h"
> @@ -22,8 +23,8 @@
>  void pin_insert(struct fs_pin *pin, struct vfsmount *m)
>  {
>  	spin_lock(&pin_lock);
> -	hlist_add_head(&pin->s_list, &m->mnt_sb->s_pins);
> -	hlist_add_head(&pin->m_list, &real_mount(m)->mnt_pins);
> +	hlist_add_head_rcu(&pin->s_list, &m->mnt_sb->s_pins);
> +	hlist_add_head_rcu(&pin->m_list, &real_mount(m)->mnt_pins);
>  	spin_unlock(&pin_lock);
>  }
> 
> @@ -73,7 +74,7 @@
>  	while (1) {
>  		struct hlist_node *p;
>  		rcu_read_lock();
> -		p = READ_ONCE(m->mnt_pins.first);
> +		p = rcu_dereference(hlist_first_rcu(&m->mnt_pins));
>  		if (!p) {
>  			rcu_read_unlock();
>  			break;
> @@ -87,7 +88,7 @@
>  	while (1) {
>  		struct hlist_node *q;
>  		rcu_read_lock();
> -		q = READ_ONCE(p->first);
> +		q = rcu_dereference(hlist_first_rcu(p));
>  		if (!q) {
>  			rcu_read_unlock();
>  			break;
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR