Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <47eaa40205fe86ca0418a4e8bed8a6ff9755571f.camel@kernel.org>
Subject: Re: [PATCH] lockd: set other missing fields when unlocking files
From:   Jeff Layton <jlayton@kernel.org>
To:     Trond Myklebust <trondmy@hammerspace.com>,
        Charles Edward Lever <chuck.lever@oracle.com>
Cc:     "trondmy@kernel.org" <trondmy@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Mon, 07 Nov 2022 15:22:28 -0500
In-Reply-To: <3E5DCADE-432D-4CAA-88E8-DD413EDE3626@hammerspace.com>
References: <20221106190239.404803-1-trondmy@kernel.org>
         <2b5cffddf1d4d5791758e267b7184f0263519335.camel@kernel.org>
         <D25AE1CE-E8D2-4BD3-83DA-A5C3222C5E03@oracle.com>
         <3E5DCADE-432D-4CAA-88E8-DD413EDE3626@hammerspace.com>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Mon, 2022-11-07 at 18:42 +0000, Trond Myklebust wrote:
>=20
> > On Nov 7, 2022, at 09:12, Chuck Lever III <chuck.lever@oracle.com> wrot=
e:
> >=20
> >=20
> >=20
> > > On Nov 7, 2022, at 5:48 AM, Jeff Layton <jlayton@kernel.org> wrote:
> > >=20
> > > On Sun, 2022-11-06 at 14:02 -0500, trondmy@kernel.org wrote:
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > >=20
> > > > vfs_lock_file() expects the struct file_lock to be fully initialise=
d by
> > > > the caller. Re-exported NFSv3 has been seen to Oops if the fl_file =
field
> > > > is NULL.
> > > >=20
> > > > Fixes: aec158242b87 ("lockd: set fl_owner when unlocking files")
> > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > ---
> > > > fs/lockd/svcsubs.c | 17 ++++++++++-------
> > > > 1 file changed, 10 insertions(+), 7 deletions(-)
> > > >=20
> > > > diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> > > > index e1c4617de771..3515f17eaf3f 100644
> > > > --- a/fs/lockd/svcsubs.c
> > > > +++ b/fs/lockd/svcsubs.c
> > > > @@ -176,7 +176,7 @@ nlm_delete_file(struct nlm_file *file)
> > > > }
> > > > }
> > > >=20
> > > > -static int nlm_unlock_files(struct nlm_file *file, fl_owner_t owne=
r)
> > > > +static int nlm_unlock_files(struct nlm_file *file, const struct fi=
le_lock *fl)
> > > > {
> > > > struct file_lock lock;
> > > >=20
> > > > @@ -184,12 +184,15 @@ static int nlm_unlock_files(struct nlm_file *=
file, fl_owner_t owner)
> > > > lock.fl_type  =3D F_UNLCK;
> > > > lock.fl_start =3D 0;
> > > > lock.fl_end   =3D OFFSET_MAX;
> > > > - lock.fl_owner =3D owner;
> > > > - if (file->f_file[O_RDONLY] &&
> > > > -    vfs_lock_file(file->f_file[O_RDONLY], F_SETLK, &lock, NULL))
> > > > + lock.fl_owner =3D fl->fl_owner;
> > > > + lock.fl_pid   =3D fl->fl_pid;
> > > > + lock.fl_flags =3D FL_POSIX;
> > > > +
> > > > + lock.fl_file =3D file->f_file[O_RDONLY];
> > > > + if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, N=
ULL))
> > > > goto out_err;
> > > > - if (file->f_file[O_WRONLY] &&
> > > > -    vfs_lock_file(file->f_file[O_WRONLY], F_SETLK, &lock, NULL))
> > > > + lock.fl_file =3D file->f_file[O_WRONLY];
> > > > + if (lock.fl_file && vfs_lock_file(lock.fl_file, F_SETLK, &lock, N=
ULL))
> > > > goto out_err;
> > > > return 0;
> > > > out_err:
> > > > @@ -226,7 +229,7 @@ nlm_traverse_locks(struct nlm_host *host, struc=
t nlm_file *file,
> > > > if (match(lockhost, host)) {
> > > >=20
> > > > spin_unlock(&flctx->flc_lock);
> > > > - if (nlm_unlock_files(file, fl->fl_owner))
> > > > + if (nlm_unlock_files(file, fl))
> > > > return 1;
> > > > goto again;
> > > > }
> > >=20
> > > Good catch.
> > >=20
> > > I wonder if we ought to roll an initializer function for file_locks t=
o
> > > make it harder for callers to miss setting some fields like this? One
> > > idea: we could change vfs_lock_file to *not* take a file argument, an=
d
> > > insist that the caller fill out fl_file when calling it? That would m=
ake
> > > it harder to screw this up.
> > >=20
> > > In any case, let's take this patch in the interim while we consider
> > > whether and how to clean this up.
> > >=20
> > > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> >=20
> > Since this doesn't fix breakage in 6.1-rc, I plan to take it for 6.2.
> > If all y'all feel the fix is more urgent than that, let me know.
>=20
>=20
> It is relevant to fixing https://bugzilla.kernel.org/show_bug.cgi?id=3D21=
6582
> No idea how urgent that is...
>

Seems like it's technically a regression then. Prior to aec158242b87,
those locks were being ignored. Now that we actually try to unlock them,
this causes a crash.

I move for sending it to mainline sooner rather than later.
--=20
Jeff Layton <jlayton@kernel.org>