Received-SPF: pass (google.com: domain of linux-nfs+bounces-1723-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::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: "Chuck Lever III" <chuck.lever@oracle.com>
Cc: "linux-stable" <stable@vger.kernel.org>, "Chuck Lever" <cel@kernel.org>,
 "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 0/3] Fix RELEASE_LOCKOWNER
In-reply-to: <FF7C90E0-251D-48D2-908B-E2145B0B9BAE@oracle.com>
References:
 <170681433624.15250.5881267576986350500.stgit@klimt.1015granger.net>,
 <170682628772.13976.3551007711597007133@noble.neil.brown.name>,
 <FF7C90E0-251D-48D2-908B-E2145B0B9BAE@oracle.com>
Date: Sat, 03 Feb 2024 11:26:06 +1100
Message-id: <170691996664.13976.18138125578593325497@noble.neil.brown.name>

On Sat, 03 Feb 2024, Chuck Lever III wrote:
>=20
>=20
> > On Feb 1, 2024, at 5:24=E2=80=AFPM, NeilBrown <neilb@suse.de> wrote:
> >=20
> > On Fri, 02 Feb 2024, Chuck Lever wrote:
> >> Passes pynfs, fstests, and the git regression suite. Please apply
> >> these to origin/linux-5.4.y.
> >=20
> > I should have mentioned this a day or two ago but I hadn't quite made
> > all the connection yet...
> >=20
> > The RELEASE_LOCKOWNER bug was masking a double-free bug that was fixed
> > by
> > Commit 47446d74f170 ("nfsd4: add refcount for nfsd4_blocked_lock")
> > which landed in v5.17 and wasn't marked as a bugfix, and so has not gone =
to
> > stable kernels.
>=20
> Then, instructions to stable@vger.kernel.org:
>=20
> Do not apply the patches I just sent for 5.15, 5.10, and 5.4. I will
> apply 47446d74f170, run the tests again, and resend.
>=20
>=20
> > Any kernel earlier than v5.17 that receives the RELEASE_LOCKOWNER fix
> > also needs the nfsd4_blocked_lock fix.  There is a minor follow-up fix
> > for that nfsd4_blocked_lock fix which Chuck queued yesterday.
> >=20
> > The problem scenario is that an nfsd4_lock() call finds a conflicting
> > lock and so has a reference to a particular nfsd4_blocked_lock.  A concur=
rent
> > nfsd4_read_lockowner call frees all the nfsd4_blocked_locks including
> > the one held in nfsd4_lock().  nfsd4_lock then tries to free the
> > blocked_lock it has, and results in a double-free or a use-after-free.
> >=20
> > Before either patch is applied, the extra reference on the lock-owner
> > than nfsd4_lock holds causes nfsd4_realease_lockowner() to incorrectly
> > return an error and NOT free the blocks_lock.
> > With only the RELEASE_LOCKOWNER fix applied, the double-free happens.
>=20
> Our test suite currently does not exercise this use case, apparently.
> I didn't see a problem like this during testing.
>=20

Our OpenQA testing found it (as did our customer :-).
Quoting from a bugzilla that unfortunately is not public (though might
not be accessible to anyone with an account)

https://bugzilla.suse.com/show_bug.cgi?id=3D1219349

     LTP test nfslock01.sh randomly fails on the latest SLE-15SP4 and
     SLE-15SP5 KOTD.  The failures appear only when testing NFS protocol
     v4.0, other versions do not seem to be affected.  The test either
     gets stuck or sometimes triggers kernel oops.  The contents of the
     kernel backtrace vary.  All archs appear to be affected.=20

Does your test suite cover v4.0?  Does it include LTP ?

Thanks,
NeilBrown


>=20
> > With both patches applied the refcount on the nfsd4_blocked_lock prevents
> > the double-free.
> >=20
> > Kernels before 4.9 are (probably) not affected as they didn't have
> > find_or_allocate_block() which takes the second reference to a shared
> > object.  But that is ancient history - those kernels are well past EOL.
> >=20
> > Thanks,
> > NeilBrown
> >=20
> >=20
> >>=20
> >> ---
> >>=20
> >> Chuck Lever (2):
> >>      NFSD: Modernize nfsd4_release_lockowner()
> >>      NFSD: Add documenting comment for nfsd4_release_lockowner()
> >>=20
> >> NeilBrown (1):
> >>      nfsd: fix RELEASE_LOCKOWNER
> >>=20
> >>=20
> >> fs/nfsd/nfs4state.c | 65 +++++++++++++++++++++++++--------------------
> >> 1 file changed, 36 insertions(+), 29 deletions(-)
> >>=20
> >> --
> >> Chuck Lever
> >>=20
> >>=20
> >>=20
> >=20
>=20
> --
> Chuck Lever
>=20
>=20
>=20