Ocfs2 devel - Nov 2022 - [PATCH] filelock: move file locking definitions to separate header file

On Fri, 2022-11-25 at 03:48 +0000, Al Viro wrote:
> On Sun, Nov 20, 2022 at 03:59:57PM -0500, Jeff Layton wrote:
> 
> > --- /dev/null
> > +++ b/include/linux/filelock.h
> > @@ -0,0 +1,428 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _LINUX_FILELOCK_H
> > +#define _LINUX_FILELOCK_H
> > +
> > +#include <linux/list.h>
> > +#include <linux/nfs_fs_i.h>
> 
> Umm... I'd add a comment along the lines of "struct file_lock has
> a BS union by fs type; NFS side of things needs nfs_fs_i.h"
> 
Ok.
> > +struct lock_manager_operations {
> > +	void *lm_mod_owner;
> > +	fl_owner_t (*lm_get_owner)(fl_owner_t);
> 
> Probably take fl_owner_t to some more neutral header...
> 
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.

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.
> > +#define locks_inode(f) file_inode(f)
> 
> Why do we still have that one, anyway?  Separate patch, obviously,
> but I would take Occam's Razor to that entity...
> 
I can spin up a patch to nuke that too. I count only 30 callsites
remaining anyway.
> > +struct files_struct;
> > +extern void show_fd_locks(struct seq_file *f,
> > +			 struct file *filp, struct files_struct *files);
> 
> If anything, that would be better off as fl_owner_t...  Again, a separate
> patch.
I'm not sure what you mean here. This prototype hasn't changed, and is
only called from procfs.

-- 
Jeff Layton <jlayton at kernel.org>

On Fri, Nov 25, 2022 at 08:23:45AM -0500, Jeff Layton wrote:
> 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.
> 
> 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.
Might be...
> > > +extern void show_fd_locks(struct seq_file *f,
> > > +			 struct file *filp, struct files_struct *files);
> > 
> > If anything, that would be better off as fl_owner_t...  Again, a
separate
> > patch.
> 
> I'm not sure what you mean here. This prototype hasn't changed, and
is
> only called from procfs.
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; at
that
point it might be pointing to freed memory, for all we know (and give false
positives if already reused).

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