Date: Fri, 7 Oct 2016 17:18:18 -0400
From: Calvin Owens <calvinowens@fb.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
CC: Jeff Layton <jlayton@poochiereds.net>,
        "J. Bruce Fields" <bfields@fieldses.org>,
        Rusty Russell <rusty@rustcorp.com.au>, <linux-fsdevel@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <kernel-team@fb.com>
Subject: Re: [PATCH] fs: Assert on module file_operations without an owner
Message-ID: <20161007211818.GA84073@calvinowens-mba>
References: <48414ef29337b54e2a842bd841f73f01ab74ebe7.1475872278.git.calvinowens@fb.com>
 <20161007204836.GR19539@ZenIV.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20161007204836.GR19539@ZenIV.linux.org.uk>
User-Agent: Mutt/1.6.1 (2016-04-27)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Oct 2016 21:18:29.0497 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1229
X-OriginatorOrg: fb.com
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-10-07_09:,,
 signatures=0
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2049
Lines: 41

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.