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 <[email protected]>
---
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
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.
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.
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
>
FYI, we noticed the following commit:
https://github.com/0day-ci/linux Calvin-Owens/fs-Assert-on-module-file_operations-without-an-owner/20161008-045103
commit 148e828376ac797fe7b6a9b36301e4d109db97b7 ("fs: Assert on module file_operations without an owner")
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu host -smp 2 -m 1G
caused below changes:
+----------------------------------------------------------------------------+------------+------------+
| | 2ab704a47e | 148e828376 |
+----------------------------------------------------------------------------+------------+------------+
| boot_successes | 2 | 5 |
| boot_failures | 4 | 13 |
| BUG:kernel_hang_in_test_stage | 2 | |
| WARNING:at_fs/sysfs/dir.c:#sysfs_warn_dup | 2 | 4 |
| calltrace:parport_pc_init | 2 | 4 |
| calltrace:SyS_finit_module | 2 | 4 |
| WARNING:at_lib/kobject.c:#kobject_add_internal | 2 | 4 |
| WARNING:at_fs/open.c:#do_dentry_open | 0 | 11 |
| calltrace:SyS_open | 0 | 11 |
| Kernel_panic-not_syncing:VFS:Unable_to_mount_root_fs_on_unknown-block(#,#) | 0 | 2 |
| calltrace:prepare_namespace | 0 | 2 |
+----------------------------------------------------------------------------+------------+------------+
[ 5.032473]
[ 5.039317] -1G-17/boot-1-debian-x86_64-2016-08-31.cgz-148e828376ac797fe7b6a9b36301e4d109db97b7-20161008-50461-l76q6n-0.yaml&job_state=running -o /dev/null
[ 5.323019] ------------[ cut here ]------------
[ 5.324673] WARNING: CPU: 0 PID: 407 at fs/open.c:719 do_dentry_open+0x210/0x350
[ 5.327689] No fops owner at ffffffffa01d5a00 in [nfsv4]
[ 5.329424] Modules linked in: rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver sr_mod cdrom sg ata_generic pata_acpi sb_edac edac_core crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel ppdev aesni_intel ata_piix lrw gf128mul glue_helper ablk_helper cryptd snd_pcm snd_timer snd soundcore pcspkr libata serio_raw i2c_piix4 parport_pc parport floppy acpi_cpufreq ip_tables
[ 5.342863] CPU: 0 PID: 407 Comm: wrapper Not tainted 4.8.0-09940-g148e828 #13
[ 5.345751] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[ 5.348544] ffffc9000072bb38 ffffffff81454e59 ffffc9000072bb88 0000000000000000
[ 5.351410] ffffc9000072bb78 ffffffff8107ff5b 000002cf3f41c9c0 ffff88003f962800
[ 5.354180] 0000000000000000 ffff88003f962810 ffff88003f2fbcc0 ffff88003f6ae1b8
[ 5.356944] Call Trace:
[ 5.357991] [<ffffffff81454e59>] dump_stack+0x63/0x8a
[ 5.359462] [<ffffffff8107ff5b>] __warn+0xcb/0xf0
[ 5.360872] [<ffffffff8107ffcf>] warn_slowpath_fmt+0x4f/0x60
[ 5.362443] [<ffffffff8120e0e0>] do_dentry_open+0x210/0x350
[ 5.364427] [<ffffffff813016e0>] ? nfs_force_lookup_revalidate+0x20/0x20
[ 5.366545] [<ffffffff8120e24f>] finish_open+0x2f/0x40
[ 5.368358] [<ffffffff81305422>] nfs_atomic_open+0x1e2/0x570
[ 5.370272] [<ffffffff81220924>] path_openat+0xd24/0x13e0
[ 5.372150] [<ffffffff811b81b6>] ? unmap_page_range+0x6c6/0x910
[ 5.374112] [<ffffffff811b72ec>] ? do_wp_page+0xfc/0x830
[ 5.375989] [<ffffffff8122217e>] do_filp_open+0x7e/0xe0
[ 5.377693] [<ffffffff8122119f>] ? getname_flags+0x4f/0x1f0
[ 5.379472] [<ffffffff81230bfa>] ? __alloc_fd+0xca/0x180
[ 5.381189] [<ffffffff8120f853>] do_sys_open+0x123/0x200
[ 5.382926] [<ffffffff8120f94e>] SyS_open+0x1e/0x20
[ 5.384577] [<ffffffff81942577>] entry_SYSCALL_64_fastpath+0x1a/0xa9
[ 5.386705] ---[ end trace 6c0092634b9a9a26 ]---
[ 5.386705] ---[ end trace 6c0092634b9a9a26 ]---
Thanks,
Xiaolong