From patchwork Mon Nov 14 17:12:47 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mike Marshall X-Patchwork-Id: 9428047 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DC6986047D for ; Mon, 14 Nov 2016 17:12:53 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C5AC428A67 for ; Mon, 14 Nov 2016 17:12:53 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BA83E28A6C; Mon, 14 Nov 2016 17:12:53 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E1E7028A67 for ; Mon, 14 Nov 2016 17:12:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932980AbcKNRMv (ORCPT ); Mon, 14 Nov 2016 12:12:51 -0500 Received: from mail-lf0-f48.google.com ([209.85.215.48]:35533 "EHLO mail-lf0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932260AbcKNRMu (ORCPT ); Mon, 14 Nov 2016 12:12:50 -0500 Received: by mail-lf0-f48.google.com with SMTP id b14so63085414lfg.2 for ; Mon, 14 Nov 2016 09:12:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=omnibond-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=+uOwXikUI9h2XBky1BtYkQGmWcsGpT/uZfZQT9DDxoU=; b=uQ8rH3L12VDNQ8gyhiVZzecV/YuE0FhR9JtRDDiSXiuAU8lZdIjxoDlXlY+5iwMDMx V45LnKLbcmTgRHTjCBF9RRPGGzTSiiVierjsIjJj9M9mAs8uuYHgiw1b2TH/Gt9zpUxt kHz9hJXTx4eMgA9efcnsKmmVskRpt1Uxk/xuhEt8n549GoLfVJijiN9BUc/A7+sfqzd+ y+mi1FAl5IyK8dNd1IKDhzaOgyvS32zYYtr/cpU/wb3G0nmkyS/VBvv+vEwzlMkV6uKy bL8hhZxHw/mtAkoZABhXtlUTjQ9VuzPwT2sH3oXKb7kgGE33hik9wzoAkqeotCGX/cVQ 9/qw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=+uOwXikUI9h2XBky1BtYkQGmWcsGpT/uZfZQT9DDxoU=; b=FlveBfninqSTa88GNXMG/U/wIfxj5TYF/z4xPAr46eaVTP+GiW63QR04eS6Mgu16ef Fz+8nfcCYPUeGK4Qa0ueVyzddnnmYEHbEuzfHtO69mNXQz55Vdhu773jJvYS+V19ce3K snL4hK32Y2Dow5QMZbCm1Of9kNeYBnshvthaYfoQSC+2x9Nuovzk0UC/oKO7jzepNUWx u3e7g16bss8zVCy/5GxXrEZ0lhGZX8yjnm3Ecf2DCTfEahqS4Alw2I80gZJR/NOlx5rp IbGUwB5D9obIomkOcs1wgonvu+KRzUa66u682Hn4tHKMg6AJfFDNBi9AuEneK+ZPAdh8 /zEg== X-Gm-Message-State: ABUngvexzzvyG2NSfHrDGS610abzkjO8Psop/pXvzH0gHJRVOCO9cfGRmkJOw2UemqjgLt82C+5/zjWxCZf6YA== X-Received: by 10.25.76.195 with SMTP id z186mr9239111lfa.104.1479143568621; Mon, 14 Nov 2016 09:12:48 -0800 (PST) MIME-Version: 1.0 Received: by 10.25.132.14 with HTTP; Mon, 14 Nov 2016 09:12:47 -0800 (PST) In-Reply-To: <877f876wo9.fsf@gmail.com> References: <20161031193823.GA11187@kroah.com> <87wpgoff0o.fsf@gmail.com> <877f876wo9.fsf@gmail.com> From: Mike Marshall Date: Mon, 14 Nov 2016 12:12:47 -0500 Message-ID: Subject: Re: debugfs question... To: Nicolai Stange Cc: Greg KH , Al Viro , linux-fsdevel , Linus Torvalds , Martin Brandenburg Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP OK, I did this: I changed my little tester program to read from /sys/kernel/debug/orangefs/debug-help a byte at a time and sleep for a second between bytes. Now I get nice error messages if I try to unload the module while someone is reading debug-help, and it unloads as normal when the reader is done: [root@be1 hubcap]# rmmod orangefs.ko rmmod: ERROR: Module orangefs is in use [root@be1 hubcap]# rmmod orangefs.ko rmmod: ERROR: Module orangefs is in use [root@be1 hubcap]# rmmod orangefs.ko rmmod: ERROR: Module orangefs is in use [root@be1 hubcap]# rmmod orangefs.ko [root@be1 hubcap]# If this seems right, I'll see about getting it pulled... Thanks! -Mike On Sun, Nov 13, 2016 at 1:51 PM, Nicolai Stange wrote: > Hi again, > > bad news: my previous analysis was completely wrong, c.f. below. > Good news (from my point of view): debugfs is correct, no fix needed for > it. > > Apologies for the confusion... > > > Nicolai Stange writes: > >> Greg KH writes: >> >>> On Mon, Oct 31, 2016 at 02:32:56PM -0400, Mike Marshall wrote: >>> >>>> But... really bad things happen if someone unloads the Orangefs >>>> module after my test program does the open and before the read >>>> starts. So I picked another debugfs-using-filesystem (f2fs) and >>>> pointed my tester-program at /sys/kernel/debug/f2fs/status, and >>>> the same bad thing happens there. >> >> [...] >> >>>> [ 1240.144316] Call Trace: >>>> [ 1240.144450] [] __fput+0xdf/0x1d0 >>>> [ 1240.144704] [] ____fput+0xe/0x10 >>>> [ 1240.144962] [] task_work_run+0x8e/0xc0 >>>> [ 1240.145243] [] do_exit+0x2ae/0xae0 >> >> >> Thank you very much for this detailed report! >> >> At least for the .../f2fs/status file, your splat at fput() can be >> readily explained with the full proxy's releaser not being protected >> against file removals in any way. >> >> Partly this is on purpose, c.f. the comment in full_proxy_release(). >> >> However, I should have at least tried to acquire a reference to the >> owning module before accessing some static struct file_operations or >> even calling some ->release() within it. Meh. > > This is what I got wrong: debugfs does acquire the needed references > correctly (details below). For the case of f2fs' "status" file, the > file_operations ->owner is simply not set as it should have been, > i.e. to THIS_MODULE. > > > Details on debugfs' reference acquisition: > The open proxy, full_proxy_open(), gets a reference to the "real" > file_operations, hence to its module. (Only in its error path it > releases it again). > > full_proxy_release() is in charge of dropping that reference again, but > only *after* it has attempted to call the "real" ->release(). > > So, as long as a file has been (successfully) opened, a reference to the > original file_operation's ->owner is owned, preventing it from getting > unloaded. > > > Can you confirm that you didn't set ->owner in your Orangefs related > tests, too? > > > Thanks, > > Nicolai --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/fs/orangefs/orangefs-debugfs.c b/fs/orangefs/orangefs-debugfs.c index d484068..38887cc 100644 --- a/fs/orangefs/orangefs-debugfs.c +++ b/fs/orangefs/orangefs-debugfs.c @@ -114,6 +114,7 @@ static ssize_t orangefs_debug_write(struct file *, }; const struct file_operations debug_help_fops = { + .owner = THIS_MODULE, .open = orangefs_debug_help_open, .read = seq_read, .release = seq_release, @@ -121,6 +122,7 @@ static ssize_t orangefs_debug_write(struct file *, }; static const struct file_operations kernel_debug_fops = { + .owner = THIS_MODULE, .open = orangefs_debug_open, .read = orangefs_debug_read, .write = orangefs_debug_write,