Received-SPF: pass (google.com: domain of linux-nfs+bounces-1284-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: "Jeff Layton" <jlayton@kernel.org>, "Dai Ngo" <dai.ngo@oracle.com>,
 "Olga Kornievskaia" <kolga@netapp.com>, "Tom Talpey" <tom@talpey.com>,
 "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH] nfsd: fix RELEASE_LOCKOWNER
In-reply-to: <D841692A-1D9D-49F9-B497-AA40B975C5E9@oracle.com>
References: <170589589641.23031.16356786177193106749@noble.neil.brown.name>,
 <Za57adpDbKJavMRO@tissot.1015granger.net>,
 <170596063560.23031.1725209290511630080@noble.neil.brown.name>,
 <3162C5BC-8E7C-4A9A-815C-09297B56FA17@oracle.com>,
 <170596630337.23031.332959396445243083@noble.neil.brown.name>,
 <D841692A-1D9D-49F9-B497-AA40B975C5E9@oracle.com>
Date: Tue, 23 Jan 2024 11:18:41 +1100
Message-id: <170596912197.23031.1418374526012433512@noble.neil.brown.name>

On Tue, 23 Jan 2024, Chuck Lever III wrote:
>=20
>=20
> > On Jan 22, 2024, at 6:31=E2=80=AFPM, NeilBrown <neilb@suse.de> wrote:
> >=20
> > On Tue, 23 Jan 2024, Chuck Lever III wrote:
> >>=20
> >>=20
> >>> On Jan 22, 2024, at 4:57=E2=80=AFPM, NeilBrown <neilb@suse.de> wrote:
> >>>=20
> >>> On Tue, 23 Jan 2024, Chuck Lever wrote:
> >>>> On Mon, Jan 22, 2024 at 02:58:16PM +1100, NeilBrown wrote:
> >>>>>=20
> >>>>> The test on so_count in nfsd4_release_lockowner() is nonsense and
> >>>>> harmful.  Revert to using check_for_locks(), changing that to not sle=
ep.
> >>>>>=20
> >>>>> First: harmful.
> >>>>> As is documented in the kdoc comment for nfsd4_release_lockowner(), t=
he
> >>>>> test on so_count can transiently return a false positive resulting in=
 a
> >>>>> return of NFS4ERR_LOCKS_HELD when in fact no locks are held.  This is
> >>>>> clearly a protocol violation and with the Linux NFS client it can cau=
se
> >>>>> incorrect behaviour.
> >>>>>=20
> >>>>> If NFS4_RELEASE_LOCKOWNER is sent while some other thread is still
> >>>>> processing a LOCK request which failed because, at the time that requ=
est
> >>>>> was received, the given owner held a conflicting lock, then the nfsd
> >>>>> thread processing that LOCK request can hold a reference (conflock) to
> >>>>> the lock owner that causes nfsd4_release_lockowner() to return an
> >>>>> incorrect error.
> >>>>>=20
> >>>>> The Linux NFS client ignores that NFS4ERR_LOCKS_HELD error because it
> >>>>> never sends NFS4_RELEASE_LOCKOWNER without first releasing any locks,=
 so
> >>>>> it knows that the error is impossible.  It assumes the lock owner was=
 in
> >>>>> fact released so it feels free to use the same lock owner identifier =
in
> >>>>> some later locking request.
> >>>>>=20
> >>>>> When it does reuse a lock owner identifier for which a previous RELEA=
SE
> >>>>> failed, it will naturally use a lock_seqid of zero.  However the serv=
er,
> >>>>> which didn't release the lock owner, will expect a larger lock_seqid =
and
> >>>>> so will respond with NFS4ERR_BAD_SEQID.
> >>>>>=20
> >>>>> So clearly it is harmful to allow a false positive, which testing
> >>>>> so_count allows.
> >>>>>=20
> >>>>> The test is nonsense because ... well... it doesn't mean anything.
> >>>>>=20
> >>>>> so_count is the sum of three different counts.
> >>>>> 1/ the set of states listed on so_stateids
> >>>>> 2/ the set of active vfs locks owned by any of those states
> >>>>> 3/ various transient counts such as for conflicting locks.
> >>>>>=20
> >>>>> When it is tested against '2' it is clear that one of these is the
> >>>>> transient reference obtained by find_lockowner_str_locked().  It is n=
ot
> >>>>> clear what the other one is expected to be.
> >>>>>=20
> >>>>> In practice, the count is often 2 because there is precisely one state
> >>>>> on so_stateids.  If there were more, this would fail.
> >>>>>=20
> >>>>> It my testing I see two circumstances when RELEASE_LOCKOWNER is calle=
d.
> >>>>> In one case, CLOSE is called before RELEASE_LOCKOWNER.  That results =
in
> >>>>> all the lock states being removed, and so the lockowner being discard=
ed
> >>>>> (it is removed when there are no more references which usually happens
> >>>>> when the lock state is discarded).  When nfsd4_release_lockowner() fi=
nds
> >>>>> that the lock owner doesn't exist, it returns success.
> >>>>>=20
> >>>>> The other case shows an so_count of '2' and precisely one state listed
> >>>>> in so_stateid.  It appears that the Linux client uses a separate lock
> >>>>> owner for each file resulting in one lock state per lock owner, so th=
is
> >>>>> test on '2' is safe.  For another client it might not be safe.
> >>>>>=20
> >>>>> So this patch changes check_for_locks() to use the (newish)
> >>>>> find_any_file_locked() so that it doesn't take a reference on the
> >>>>> nfs4_file and so never calls nfsd_file_put(), and so never sleeps.
> >>>>=20
> >>>> More to the point, find_any_file_locked() was added by commit
> >>>> e0aa651068bf ("nfsd: don't call nfsd_file_put from client states
> >>>> seqfile display"), which was merged several months /after/ commit
> >>>> ce3c4ad7f4ce ("NFSD: Fix possible sleep during
> >>>> nfsd4_release_lockowner()").
> >>>=20
> >>> Yes.  To flesh out the history:
> >>> nfsd_file_put() was added in v5.4.  In earlier kernels check_for_locks()
> >>> would never sleep.  However the problem patch was backported 4.9, 4.14,
> >>> and 4.19 and should be reverted.
> >>=20
> >> I don't see "NFSD: Fix possible sleep during nfsd4_release_lockowner()"
> >> in any of those kernels. All but 4.19 are now EOL.
> >=20
> > I hadn't checked which were EOL.  Thanks for finding the 4.19 patch and
> > requesting a revert.
> >=20
> >>=20
> >>=20
> >>> find_any_file_locked() was added in v6.2 so when this patch is
> >>> backported to 5.4, 5.10, 5.15, 5.17 - 6.1 it needs to include
> >>> find_and_file_locked()
> >>=20
> >> I think I'd rather leave those unperturbed until someone hits a real
> >> problem. Unless you have a distribution kernel that needs to see
> >> this fix in one of the LTS kernels? The supported stable/LTS kernels
> >> are 5.4, 5.10, 5.15, and 6.1.
> >=20
> > Why not fix the bug?  It's a real bug that a real customer really hit.
>=20
> That's what I'm asking. Was there a real customer issue? Because
> ce3c4ad7f4ce was the result of a might_sleep splat, not an issue
> in the field.
>=20
> Since this hit a real user, is there a BugLink: or Closes: tag
> I can add?

Unfortunately not.  Our bugs for paying customers are private - so they
can feel comfortable providing all the details we need.
The link is https://bugzilla.suse.com/show_bug.cgi?id=3D1218968
that won't help anyone outside SUSE.

But definitely a real bug.

>=20
>=20
> > I've fixed it in all SLE kernels - we don't depend on LTS though we do
> > make use of the stable trees in various ways.
> >=20
> > But it's your call.
>=20
> Well, it's not my call whether it's backported. Anyone can ask.
> It /is/ my call if I do the work.

That's fair.  I'm happy to create the manual backport patch.  I'm not
going to test those kernels.  I generally assume that someone is testing
stable kernels - so obvious problems will be found by someone else, and
non-obvious problems I wouldn't find even if I did some testing :-(

Thanks,
NeilBrown


>=20
> Mostly the issue is having the time to manage 5-6 stable
> kernels as well as upstream development. I'd like to ensure
> that any patches for which we manually request a backport have
> been applied and tested by someone with a real NFS rig, and
> there is a real problem getting addressed.
>=20
> It's getting better with kdevops... I can cherry pick any
> recent kernel and run it through a number of tests without
> needing to hand-hold. Haven't tried that with the older stable
> kernels, though; those require older compilers and what-not.
>=20
>=20
> --
> Chuck Lever
>=20
>=20
>=20