Received-SPF: pass (google.com: domain of linux-kernel+bounces-177569-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 13 May 2024 13:41:32 +0100
From: Breno Leitao <leitao@debian.org>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: paulmck@kernel.org,
	"open list:FUSE: FILESYSTEM IN USERSPACE" <linux-fsdevel@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] fuse: annotate potential data-race in num_background
Message-ID: <ZkIKfFs-0lfflzV-@gmail.com>
References: <20240509125716.1268016-1-leitao@debian.org>
 <CAJfpeguh9upC5uqcb3uetoMm1W7difC86+-BxZZPjkXa-bNqLg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJfpeguh9upC5uqcb3uetoMm1W7difC86+-BxZZPjkXa-bNqLg@mail.gmail.com>

Hello Miklos,

On Fri, May 10, 2024 at 11:21:19AM +0200, Miklos Szeredi wrote:
> On Thu, 9 May 2024 at 14:57, Breno Leitao <leitao@debian.org> wrote:
> 
> > Annotated the reader with READ_ONCE() and the writer with WRITE_ONCE()
> > to avoid such complaint from KCSAN.
> 
> I'm not sure the write side part is really needed, since the lock is
> properly protecting against concurrent readers/writers within the
> locked region.

I understand that num_background is read from an unlocked region
(fuse_readahead()).

> Does KCSAN still complain if you just add the READ_ONCE() to fuse_readahead()?

I haven't checked, but, looking at the documentation it says that both part
needs to be marked. Here is an example very similar to ours here, from
tools/memory-model/Documentation/access-marking.txt

	Lock-Protected Writes With Lockless Reads
	-----------------------------------------

	For another example, suppose a shared variable "foo" is updated only
	while holding a spinlock, but is read locklessly.  The code might look
	as follows:

		int foo;
		DEFINE_SPINLOCK(foo_lock);

		void update_foo(int newval)
		{
			spin_lock(&foo_lock);
			WRITE_ONCE(foo, newval);
			ASSERT_EXCLUSIVE_WRITER(foo);
			do_something(newval);
			spin_unlock(&foo_wlock);
		}

		int read_foo(void)
		{
			do_something_else();
			return READ_ONCE(foo);
		}

	Because foo is read locklessly, all accesses are marked.


From my understanding, we need a WRITE_ONCE() inside the lock, because
the bg_lock lock in fuse_request_end() is invisible for fuse_readahead(),
and fuse_readahead() might read num_backgroud that was writen
non-atomically/corrupted (if there is no WRITE_ONCE()).

That said, if the reader (fuse_readahead()) can handle possible
corrupted data, we can mark is with data_race() annotation. Then I
understand we don't need to mark the write with WRITE_ONCE().

Here is what access-marking.txt says about this case:

	Here are some situations where data_race() should be used instead of
	READ_ONCE() and WRITE_ONCE():

	1.      Data-racy loads from shared variables whose values are used only
		for diagnostic purposes.

	2.      Data-racy reads whose values are checked against marked reload.

	3.      Reads whose values feed into error-tolerant heuristics.

	4.      Writes setting values that feed into error-tolerant heuristics.


Anyway, I am more than happy to test with only a READ_ONLY() in the
reader side, if that the approach you prefer.

Thanks!