linux-kernel - Re: [PATCH] fuse: annotate potential data-race in num

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]

Date: Mon, 13 May 2024 13:41:32 +0100
From: Breno Leitao <leitao@...ian.org>
To: Miklos Szeredi <miklos@...redi.hu>
Cc: paulmck@...nel.org,
	"open list:FUSE: FILESYSTEM IN USERSPACE" <linux-fsdevel@...r.kernel.org>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fuse: annotate potential data-race in num_background

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@...ian.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!