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: <6a093484bb977355db40c70ffa51386f3d4ed57b.camel@kernel.org>
Subject: Re: [PATCH] filelock: move file locking definitions to separate
 header file
From:   Jeff Layton <jlayton@kernel.org>
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Eric Van Hensbergen <ericvh@gmail.com>,
        Latchesar Ionkov <lucho@ionkov.net>,
        Dominique Martinet <asmadeus@codewreck.org>,
        Christian Schoenebeck <linux_oss@crudebyte.com>,
        David Howells <dhowells@redhat.com>,
        Marc Dionne <marc.dionne@auristor.com>,
        Xiubo Li <xiubli@redhat.com>,
        Ilya Dryomov <idryomov@gmail.com>,
        Steve French <sfrench@samba.org>, Paulo Alcantara <pc@cjr.nz>,
        Ronnie Sahlberg <lsahlber@redhat.com>,
        Shyam Prasad N <sprasad@microsoft.com>,
        Tom Talpey <tom@talpey.com>,
        Christine Caulfield <ccaulfie@redhat.com>,
        David Teigland <teigland@redhat.com>,
        Chuck Lever <chuck.lever@oracle.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Bob Peterson <rpeterso@redhat.com>,
        Andreas Gruenbacher <agruenba@redhat.com>,
        Namjae Jeon <linkinjeon@kernel.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        Mark Fasheh <mark@fasheh.com>,
        Joel Becker <jlbec@evilplan.org>,
        Joseph Qi <joseph.qi@linux.alibaba.com>,
        Mike Marshall <hubcap@omnibond.com>,
        Martin Brandenburg <martin@omnibond.com>,
        "Darrick J. Wong" <djwong@kernel.org>, hch@lst.de,
        linux-kernel@vger.kernel.org, v9fs-developer@lists.sourceforge.net,
        linux-afs@lists.infradead.org, linux-fsdevel@vger.kernel.org,
        ceph-devel@vger.kernel.org, linux-cifs@vger.kernel.org,
        samba-technical@lists.samba.org, cluster-devel@redhat.com,
        linux-nfs@vger.kernel.org, ocfs2-devel@oss.oracle.com,
        devel@lists.orangefs.org, linux-xfs@vger.kernel.org
Date:   Mon, 28 Nov 2022 05:53:54 -0500
In-Reply-To: <Y4Dw65Nzt4bX9esd@ZenIV>
References: <20221120210004.381842-1-jlayton@kernel.org>
         <Y4A6/ozhUncxbimi@ZenIV>
         <1d474f53670771f324745f597ec94b63a006d687.camel@kernel.org>
         <Y4Dw65Nzt4bX9esd@ZenIV>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.1 (3.46.1-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-11-25 at 16:44 +0000, Al Viro wrote:
> On Fri, Nov 25, 2022 at 08:23:45AM -0500, Jeff Layton wrote:
>=20
> > I left it in fs.h for now. Some of the file_operations prototypes need
> > that typedef, and I figure that anyone who is including filelock.h will
> > almost certainly need to include fs.h anyway. We could move it into a
> > separate header too, but it's probably not worth it.
> >=20
> > HCH mentioned years ago though that we should just get rid of fl_owner_=
t
> > altogether and just use 'void *'. I didn't do it at the time because I
> > was focused on other changes, but this might be a good time to change
> > it.
>=20
> Might be...
>=20
> > > > +extern void show_fd_locks(struct seq_file *f,
> > > > +			 struct file *filp, struct files_struct *files);
> > >=20
> > > If anything, that would be better off as fl_owner_t...  Again, a sepa=
rate
> > > patch.
> >=20
> > I'm not sure what you mean here. This prototype hasn't changed, and is
> > only called from procfs.
>=20
> Take a look at that function and its caller.  The use of 'files' argument=
 there
> is (and can be) only as an opaque pointer to be compared to ->fl_owner; a=
t that
> point it might be pointing to freed memory, for all we know (and give fal=
se
> positives if already reused).

Ok. What we want this function to do is show any traditional POSIX or
OFD locks that were set on a particular file. The logic in
__show_fd_locks looks right, but you're correct that we don't want
anyone dereferencing those pointers in that codepath.

Note too that this info is not wholly reliable. POSIX locks can merge
with other locks that were set within the same process (same
files_struct) but on different fds.

I think we want to get rid of fl_owner_t anyway. Maybe we should replace
it with an unsigned long instead of void * to discourage anyone from
trying to dereference those pointers?

> TBH, I'd never been able to finish the audit of files_struct pointers pas=
sed
> into locks subsystem; there definitely are moments when code from fs/lock=
s.c
> is dealing with pointers to already freed instances - show_fd_locks() at =
the
> very least.  They are not dereferenced, but beyond that...

Yeah. In general, we try to ensure that locks are torn down before the
file with which it is associated, but with some of the delayed freeing,
they can outlive the file at times. For example:

    https://tracker.ceph.com/issues/57986

--=20
Jeff Layton <jlayton@kernel.org>