Received-SPF: pass (google.com: domain of linux-nfs+bounces-2383-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: Trond Myklebust <trond.myklebust@hammerspace.com>,
 Anna Schumaker <anna@kernel.org>
Cc: linux-nfs@vger.kernel.org
Subject:
 Question about possible use-after-free in nfs_direct_write_reschedule()
Date: Tue, 19 Mar 2024 11:36:28 +1100
Message-id: <171080858885.13576.7878757943353384571@noble.neil.brown.name>


I've been reviewing some nfs/direct.c patches for possible backport to
one of our older enterprise kernels and I've found something that looks
wrong.

It isn't clear to me how to exercise the code so I haven't be able to
trigger a problem.  I'm hoping that someone could either explain when
this code runs, or confirm if the code is correct or not.

Commit 954998b60caa ("NFS: Fix error handling for O_DIRECT write scheduling")

adds an extra call to nfs_release_request() but I cannot find any place
that an extra reference is taken.

The code currently reads:

	while (!list_empty(&reqs)) {
		req =3D nfs_list_entry(reqs.next);
		nfs_list_remove_request(req);
		nfs_unlock_and_release_request(req);
		if (desc.pg_error =3D=3D -EAGAIN) {
			nfs_mark_request_commit(req, NULL, &cinfo, 0);
		} else {
			spin_lock(&dreq->lock);
			nfs_direct_truncate_request(dreq, req);
			spin_unlock(&dreq->lock);
			nfs_release_request(req);
		}
	}

after the nfs_unlock_and_release_request() call I would expect that the
request could be freed, so that nfs_mark_request_commit() or the
nfs_release_request() could cause a problem.

Superficially it looks like the call should be simply
nfs_unlock_request().  This would follow the
list_remove;unlock;mark_commit pattern also found in
nfs_direct_write_reschedule_io().

Do we need:
--- a/fs/nfs/direct.c
+++ b/fs/nfs/direct.c
@@ -581,7 +581,7 @@ static void nfs_direct_write_reschedule(struct nfs_direct=
_req *dreq)
 	while (!list_empty(&reqs)) {
 		req =3D nfs_list_entry(reqs.next);
 		nfs_list_remove_request(req);
-		nfs_unlock_and_release_request(req);
+		nfs_unlock_request(req);
 		if (desc.pg_error =3D=3D -EAGAIN) {
 			nfs_mark_request_commit(req, NULL, &cinfo, 0);
 		} else {

??

Thanks,
NeilBrown