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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
