Re: [PATCH] fs: Assert on module file_operations without an owner
On Friday 10/07 at 17:18 -0400, Calvin Owens wrote: > On Friday 10/07 at 21:48 +0100, Al Viro wrote: > > On Fri, Oct 07, 2016 at 01:35:52PM -0700, Calvin Owens wrote: > > > Omitting the owner field in file_operations declared in modules is an > > > easy mistake to make, and can result in crashes when the module is > > > unloaded while userspace is poking the file. > > > > > > This patch modifies fops_get() to WARN when it encounters a NULL owner, > > > since in this case it cannot take a reference on the containing module. > > > > NAK. This is complete crap - we do *NOT* need ->owner on a lot of > > file_operations. > > This isn't a theoretical issue: I have a proprietary module that makes this > mistake and crashes when poking a chrdev it exposes in userspace races with > unloading the module. > > Of course, the bug is in this silly module. I'm not arguing that it isn't. I > was hesitant to even mention this because I know waving at something in an OOT > module is a poor argument for changing anything in the proper kernel. > > But what I'm trying to do here is prevent people from making that mistake in > the future by yelling at them when they do. The implicit ignoring of a NULL > owner in try_module_get() in fops_get() is not necessarily obvious. Let's drop this, I should never have sent the patch in the first place. > > * we do not need that on file_operations of a regular file or > > directory on a normal filesystem, since that filesystem is not going > > away until the file has been closed - ->f_path.mnt is holding a reference > > to vfsmount, which is holding a reference to superblock, which is holding > > a reference to file_system_type, which is holding a reference to _its_ > > ->owner. > > * we do not need that on anything on procfs - module removal is > > legal while a procfs file is opened; its cleanup will be blocked for the > > duration of ->read(), ->write(), etc. calls. > > I see why this is true, and it's something I considered. But when there is > zero cost to being explicit and setting ->owner, why not do it? > > > If anything, we would be better off with modifications that would get > > rid of ->owner on file_operations. It's not trivial to do, but it might > > be not impossible. I'll look into this, I'm interested. Thanks, Calvin >
Re: [PATCH] fs: Assert on module file_operations without an owner
On Friday 10/07 at 21:48 +0100, Al Viro wrote: > On Fri, Oct 07, 2016 at 01:35:52PM -0700, Calvin Owens wrote: > > Omitting the owner field in file_operations declared in modules is an > > easy mistake to make, and can result in crashes when the module is > > unloaded while userspace is poking the file. > > > > This patch modifies fops_get() to WARN when it encounters a NULL owner, > > since in this case it cannot take a reference on the containing module. > > NAK. This is complete crap - we do *NOT* need ->owner on a lot of > file_operations. This isn't a theoretical issue: I have a proprietary module that makes this mistake and crashes when poking a chrdev it exposes in userspace races with unloading the module. Of course, the bug is in this silly module. I'm not arguing that it isn't. I was hesitant to even mention this because I know waving at something in an OOT module is a poor argument for changing anything in the proper kernel. But what I'm trying to do here is prevent people from making that mistake in the future by yelling at them when they do. The implicit ignoring of a NULL owner in try_module_get() in fops_get() is not necessarily obvious. > * we do not need that on file_operations of a regular file or > directory on a normal filesystem, since that filesystem is not going > away until the file has been closed - ->f_path.mnt is holding a reference > to vfsmount, which is holding a reference to superblock, which is holding > a reference to file_system_type, which is holding a reference to _its_ > ->owner. > * we do not need that on anything on procfs - module removal is > legal while a procfs file is opened; its cleanup will be blocked for the > duration of ->read(), ->write(), etc. calls. I see why this is true, and it's something I considered. But when there is zero cost to being explicit and setting ->owner, why not do it? > If anything, we would be better off with modifications that would get > rid of ->owner on file_operations. It's not trivial to do, but it might > be not impossible.
Re: [PATCH] fs: Assert on module file_operations without an owner
On Fri, Oct 07, 2016 at 01:35:52PM -0700, Calvin Owens wrote: > Omitting the owner field in file_operations declared in modules is an > easy mistake to make, and can result in crashes when the module is > unloaded while userspace is poking the file. > > This patch modifies fops_get() to WARN when it encounters a NULL owner, > since in this case it cannot take a reference on the containing module. NAK. This is complete crap - we do *NOT* need ->owner on a lot of file_operations. * we do not need that on file_operations of a regular file or directory on a normal filesystem, since that filesystem is not going away until the file has been closed - ->f_path.mnt is holding a reference to vfsmount, which is holding a reference to superblock, which is holding a reference to file_system_type, which is holding a reference to _its_ ->owner. * we do not need that on anything on procfs - module removal is legal while a procfs file is opened; its cleanup will be blocked for the duration of ->read(), ->write(), etc. calls. If anything, we would be better off with modifications that would get rid of ->owner on file_operations. It's not trivial to do, but it might be not impossible.
[PATCH] fs: Assert on module file_operations without an owner
Omitting the owner field in file_operations declared in modules is an
easy mistake to make, and can result in crashes when the module is
unloaded while userspace is poking the file.
This patch modifies fops_get() to WARN when it encounters a NULL owner,
since in this case it cannot take a reference on the containing module.
Signed-off-by: Calvin Owens
---
include/linux/fs.h | 13 -
kernel/module.c| 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 901e25d..fafda9e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2081,10 +2081,21 @@ extern struct dentry *mount_pseudo(struct
file_system_type *, char *,
unsigned long);
/* Alas, no aliases. Too much hassle with bringing module.h everywhere */
-#define fops_get(fops) \
+#define __fops_get(fops) \
(((fops) && try_module_get((fops)->owner) ? (fops) : NULL))
#define fops_put(fops) \
do { if (fops) module_put((fops)->owner); } while(0)
+
+#define unowned_fmt "No fops owner at %p in [%s]\n"
+#define fops_unowned(fops) \
+ (is_module_address((unsigned long)(fops)) && !(fops)->owner)
+#define fops_modname(fops) \
+ __module_address((unsigned long)(fops))->name
+#define fops_warn_unowned(fops) \
+ WARN(fops_unowned(fops), unowned_fmt, (fops), fops_modname(fops))
+#define fops_get(fops) \
+ ({ fops_warn_unowned(fops); __fops_get(fops); })
+
/*
* This one is to be used *ONLY* from ->open() instances.
* fops must be non-NULL, pinned down *and* module dependencies
diff --git a/kernel/module.c b/kernel/module.c
index 529efae..4443727 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4181,6 +4181,7 @@ bool is_module_address(unsigned long addr)
return ret;
}
+EXPORT_SYMBOL_GPL(is_module_address);
/*
* __module_address - get the module which contains an address.
--
2.9.3
