Received-SPF: pass (google.com: domain of linux-kernel+bounces-91002-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 4 Mar 2024 12:31:20 -0500
From: Benjamin LaHaise <ben@communityfibre.ca>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Edward Adam Davis <eadavis@qq.com>,
	syzbot+b91eb2ed18f599dd3c31@syzkaller.appspotmail.com,
	brauner@kernel.org, jack@suse.cz, linux-aio@kvack.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	syzkaller-bugs@googlegroups.com, viro@zeniv.linux.org.uk
Subject: Re: [PATCH] fs/aio: fix uaf in sys_io_cancel
Message-ID: <20240304173120.GP20455@kvack.org>
References: <0000000000006945730612bc9173@google.com> <tencent_DC4C9767C786D2D2FDC64F099FAEFDEEC106@qq.com> <14f85d0c-8303-4710-b8b1-248ce27a6e1f@acm.org> <20240304170343.GO20455@kvack.org> <73949a4d-6087-4d8c-bae0-cda60e733442@acm.org>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <73949a4d-6087-4d8c-bae0-cda60e733442@acm.org>
User-Agent: Mutt/1.4.2.2i

On Mon, Mar 04, 2024 at 09:15:47AM -0800, Bart Van Assche wrote:
> On 3/4/24 09:03, Benjamin LaHaise wrote:
> >This is just so wrong there aren't even words to describe it.  I
> >recommending reverting all of Bart's patches since they were not reviewed
> >by anyone with a sufficient level of familiarity with fs/aio.c to get it
> >right.
> 
> Where were you while my patches were posted for review on the fsdevel
> mailing list and before these were sent to Linus?

That doesn't negate the need for someone with experience in a given
subsystem to sign off on the patches.  There are at least 2 other people I
would trust to properly review these patches, and none of their signoffs
are present either.

> A revert request should include a detailed explanation of why the revert
> should happen. Just claiming that something is wrong is not sufficient
> to motivate a revert.

A revert is justified when a series of patches is buggy and had
insufficient review prior to merging.  Mainline is not the place to be
testing half baked changes.  Getting cancellation semantics and locking
right is *VERY HARD*, and the results of getting it wrong are very subtle
and user exploitable.  Using the "a kernel warning hit" approach for work
on cancellation is very much a sign that the patches were half baked.

What testing did you do on your patch series?  The commit messages show
nothing of interest regarding testing.  Why are you touching the kiocb
after ownership has already been passed on to another entity?  This is as
bad as touching memory that has been freed.  You clearly do not understand
how that code works.

		-ben
-- 
"Thought is the essence of where you are now."