debugfs question...

Commit Message

Mike Marshall Nov. 14, 2016, 5:12 p.m. UTC

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 <[e-mail]> 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 <[e-mail]> writes:
>
>> Greg KH <[e-mail]> 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]  [<ffffffff8122907f>] __fput+0xdf/0x1d0
>>>> [ 1240.144704]  [<ffffffff812291ae>] ____fput+0xe/0x10
>>>> [ 1240.144962]  [<ffffffff810b97de>] task_work_run+0x8e/0xc0
>>>> [ 1240.145243]  [<ffffffff8109b98e>] 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,