Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Arkady]

Nicolai,

Regarding the debugfs_create_file() which ignores .mmap field in the
struct file_operations. The patch is a part of the kernel 4.8 in the
Ubuntu 16.10 and breaks quite a few online code examples of mmap
drivers.

Is is acceptable to replace the debugfs_create_file() by a macro and
fail the compilation if the .mmap field is not initialized.
Alternatively debugfs_create_file() can return an error if .mmap is
not NULL.

Thank you, Arkady.

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Nicolai Stange]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

Nicolai Stange <nicstange@gmail.com> writes:

> Sasha Levin <sasha.levin@oracle.com> writes:
>
>> On 05/18/2016 12:05 PM, Greg Kroah-Hartman wrote:
>>> On Wed, May 18, 2016 at 11:18:16AM -0400, Sasha Levin wrote:
>>>> On 05/18/2016 11:01 AM, Nicolai Stange wrote:
>>>>> Thanks a million for reporting!
>>>>>
>>>>> 1.) Do you have lockdep enabled?
>>>>
>>>> Yup, nothing there.
>>>>
>>>>> 2.) Does this happen before or after userspace init has been spawned,
>>>>>     i.e. does the lockup happen at debugfs file creation time or
>>>>>     possibly at usage time?
>>>>
>>>> So I looked closer, and it seems to happen after starting syzkaller, which
>>>> as far as I know tries to open many different debugfs files.
>>>>
>>>> Is there debug code I can add it that'll help us figure out what's up?
>>> 
>>> Trying to figure out _which_ debugfs file is causing this would be
>>> great, if at all possible.  strace?
>>
>> What seems to be failing is syzkaller's attempt to mmap the coverage
>> debugfs file. So this isn't actually a kernel deadlock but syzkaller
>> misbehaves when that scenario happens.
>>
>> Either way, it only fails to mmap with that commit that I've pointed
>> out.
>
> That info is really helpful here: the proxy file_operations introduced by
> this commit doesn't have a ->mmap() defined, i.e. it is NULL from the
> VFS layer's point of view.
>
> The simple reason is that at the time I submitted this series, my
> Coccinelle script didn't find any debugfs user with a ->mmap()
> defined. Thus either that script was broken or things have changed in
> the meanwhile.

Thankfully, it's the latter :)
See the attached cocci script I used back then.

It now reports:

  ./drivers/staging/android/sync_debug.c:330:1-20: unsupported file_operations given to debugfs
  ./kernel/kcov.c:267:6-25: unsupported file_operations given to debugfs

The kcov's ->mmap() has been introduced by

  5c9a8750a640 ("kernel: add kcov code coverage")

dated from March this year.

Since that kcov debugfs file is never removed, it needs no protecting
proxy and thus, a replacement of debugfs_create_file() by
debugfs_create_file_unsafe() will do the trick here.

I'll send patches addressing the above two issues.

>>
>> 	th->cover_fd = open("/sys/kernel/debug/kcov", O_RDWR);
>> 	if (th->cover_fd == -1)
>> 		fail("open of /sys/kernel/debug/kcov failed");
>> 	if (ioctl(th->cover_fd, KCOV_INIT_TRACE, kCoverSize))
>> 		fail("cover enable write failed");
>> 	th->cover_data = (uintptr_t*)mmap(NULL, kCoverSize * sizeof(th->cover_data[0]), PROT_READ | PROT_WRITE, MAP_SHARED, th->cover_fd, 0);
>> 	if ((void*)th->cover_data == MAP_FAILED)
>> 		fail("cover mmap failed");
>>
>> And it's the mmap() that fails with -ENODEV.


[-- Attachment #2: debugfs_unsupp_fops.cocci --]
[-- Type: text/plain, Size: 644 bytes --]

virtual report
virtual org

@unsupp_fops@
identifier fops;
expression e;
identifier m != {owner, open, release, llseek, read, write, poll, unlocked_ioctl};
@@
struct file_operations fops = {
 .m = e,
};

@unsupp_debugfs_fops@
expression name, mode, parent, data;
identifier unsupp_fops.fops;
position p;
@@
debugfs_create_file@p(name, mode, parent, data, &fops)



@script:python depends on report@
p << unsupp_debugfs_fops.p;
@@
coccilib.report.print_report(p[0], "unsupported file_operations given to debugfs")

@script:python depends on org@
p << unsupp_debugfs_fops.p;
@@
cocci.print_main("unsupported file_operations given to debugfs", p)

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Sasha Levin <sasha.levin@oracle.com> writes:

> On 05/18/2016 12:05 PM, Greg Kroah-Hartman wrote:
>> On Wed, May 18, 2016 at 11:18:16AM -0400, Sasha Levin wrote:
>>> On 05/18/2016 11:01 AM, Nicolai Stange wrote:
>>>> Thanks a million for reporting!
>>>>
>>>> 1.) Do you have lockdep enabled?
>>>
>>> Yup, nothing there.
>>>
>>>> 2.) Does this happen before or after userspace init has been spawned,
>>>>     i.e. does the lockup happen at debugfs file creation time or
>>>>     possibly at usage time?
>>>
>>> So I looked closer, and it seems to happen after starting syzkaller, which
>>> as far as I know tries to open many different debugfs files.
>>>
>>> Is there debug code I can add it that'll help us figure out what's up?
>> 
>> Trying to figure out _which_ debugfs file is causing this would be
>> great, if at all possible.  strace?
>
> What seems to be failing is syzkaller's attempt to mmap the coverage
> debugfs file. So this isn't actually a kernel deadlock but syzkaller
> misbehaves when that scenario happens.
>
> Either way, it only fails to mmap with that commit that I've pointed
> out.

That info is really helpful here: the proxy file_operations introduced by
this commit doesn't have a ->mmap() defined, i.e. it is NULL from the
VFS layer's point of view.

The simple reason is that at the time I submitted this series, my
Coccinelle script didn't find any debugfs user with a ->mmap()
defined. Thus either that script was broken or things have changed in
the meanwhile.

I'll look into this tomorrow.

Thank you very much for the effort you put into this!

>
> 	th->cover_fd = open("/sys/kernel/debug/kcov", O_RDWR);
> 	if (th->cover_fd == -1)
> 		fail("open of /sys/kernel/debug/kcov failed");
> 	if (ioctl(th->cover_fd, KCOV_INIT_TRACE, kCoverSize))
> 		fail("cover enable write failed");
> 	th->cover_data = (uintptr_t*)mmap(NULL, kCoverSize * sizeof(th->cover_data[0]), PROT_READ | PROT_WRITE, MAP_SHARED, th->cover_fd, 0);
> 	if ((void*)th->cover_data == MAP_FAILED)
> 		fail("cover mmap failed");
>
> And it's the mmap() that fails with -ENODEV.

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Sasha Levin]

On 05/18/2016 12:05 PM, Greg Kroah-Hartman wrote:
> On Wed, May 18, 2016 at 11:18:16AM -0400, Sasha Levin wrote:
>> On 05/18/2016 11:01 AM, Nicolai Stange wrote:
>>> Thanks a million for reporting!
>>>
>>> 1.) Do you have lockdep enabled?
>>
>> Yup, nothing there.
>>
>>> 2.) Does this happen before or after userspace init has been spawned,
>>>     i.e. does the lockup happen at debugfs file creation time or
>>>     possibly at usage time?
>>
>> So I looked closer, and it seems to happen after starting syzkaller, which
>> as far as I know tries to open many different debugfs files.
>>
>> Is there debug code I can add it that'll help us figure out what's up?
> 
> Trying to figure out _which_ debugfs file is causing this would be
> great, if at all possible.  strace?

What seems to be failing is syzkaller's attempt to mmap the coverage
debugfs file. So this isn't actually a kernel deadlock but syzkaller
misbehaves when that scenario happens.

Either way, it only fails to mmap with that commit that I've pointed
out.

	th->cover_fd = open("/sys/kernel/debug/kcov", O_RDWR);
	if (th->cover_fd == -1)
		fail("open of /sys/kernel/debug/kcov failed");
	if (ioctl(th->cover_fd, KCOV_INIT_TRACE, kCoverSize))
		fail("cover enable write failed");
	th->cover_data = (uintptr_t*)mmap(NULL, kCoverSize * sizeof(th->cover_data[0]), PROT_READ | PROT_WRITE, MAP_SHARED, th->cover_fd, 0);
	if ((void*)th->cover_data == MAP_FAILED)
		fail("cover mmap failed");

And it's the mmap() that fails with -ENODEV.


Thanks,
Sasha

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Sasha Levin]

On 05/18/2016 12:32 PM, Nicolai Stange wrote:
> Sasha Levin <sasha.levin@oracle.com> writes:
> 
>> On 05/18/2016 11:01 AM, Nicolai Stange wrote:
>>> Thanks a million for reporting!
>>>
>>> 1.) Do you have lockdep enabled?
>>
>> Yup, nothing there.
>>
>>> 2.) Does this happen before or after userspace init has been spawned,
>>>     i.e. does the lockup happen at debugfs file creation time or
>>>     possibly at usage time?
>>
>> So I looked closer, and it seems to happen after starting syzkaller, which
>> as far as I know tries to open many different debugfs files.
>>
>> Is there debug code I can add it that'll help us figure out what's up?
> 
> Could you try the patch below? I stared at the new full_proxy_open() for
> a while now and had to recognize the fact that if the original real_fops'
> ->open() fails, then its owning module's reference won't ever get
> dropped :(
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 6eb58a8..2e663d4 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -263,10 +263,14 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
>         if (real_fops->open) {
>                 r = real_fops->open(inode, filp);
> 
> -               if (filp->f_op != proxy_fops) {
> +               if (r) {
> +                       replace_fops(filp, d_inode(dentry)->i_fop);
> +                       goto free_proxy;
> +               } else if (filp->f_op != proxy_fops) {
>                         /* No protection against file removal anymore. */
>                         WARN(1, "debugfs file owner replaced proxy fops: %pd",
>                                 dentry);
> +                       replace_fops(filp, d_inode(dentry)->i_fop);
>                         goto free_proxy;
>                 }
>         }
> 
> 
> I don't see directly how this could lead to lockups, but I think it's
> better to rule out the obvious before inserting more or less random
> printks...
> 
> Thank you very much again,

Nope, that didn't do the trick.


Thanks,
Sasha

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Sasha Levin <sasha.levin@oracle.com> writes:

> On 05/18/2016 11:01 AM, Nicolai Stange wrote:
>> Thanks a million for reporting!
>> 
>> 1.) Do you have lockdep enabled?
>
> Yup, nothing there.
>
>> 2.) Does this happen before or after userspace init has been spawned,
>>     i.e. does the lockup happen at debugfs file creation time or
>>     possibly at usage time?
>
> So I looked closer, and it seems to happen after starting syzkaller, which
> as far as I know tries to open many different debugfs files.
>
> Is there debug code I can add it that'll help us figure out what's up?

Could you try the patch below? I stared at the new full_proxy_open() for
a while now and had to recognize the fact that if the original real_fops'
->open() fails, then its owning module's reference won't ever get
dropped :(

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 6eb58a8..2e663d4 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -263,10 +263,14 @@ static int full_proxy_open(struct inode *inode, struct file *filp)
        if (real_fops->open) {
                r = real_fops->open(inode, filp);

-               if (filp->f_op != proxy_fops) {
+               if (r) {
+                       replace_fops(filp, d_inode(dentry)->i_fop);
+                       goto free_proxy;
+               } else if (filp->f_op != proxy_fops) {
                        /* No protection against file removal anymore. */
                        WARN(1, "debugfs file owner replaced proxy fops: %pd",
                                dentry);
+                       replace_fops(filp, d_inode(dentry)->i_fop);
                        goto free_proxy;
                }
        }


I don't see directly how this could lead to lockups, but I think it's
better to rule out the obvious before inserting more or less random
printks...

Thank you very much again,

Nicolai

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Greg Kroah-Hartman]

On Wed, May 18, 2016 at 11:18:16AM -0400, Sasha Levin wrote:
> On 05/18/2016 11:01 AM, Nicolai Stange wrote:
> > Thanks a million for reporting!
> > 
> > 1.) Do you have lockdep enabled?
> 
> Yup, nothing there.
> 
> > 2.) Does this happen before or after userspace init has been spawned,
> >     i.e. does the lockup happen at debugfs file creation time or
> >     possibly at usage time?
> 
> So I looked closer, and it seems to happen after starting syzkaller, which
> as far as I know tries to open many different debugfs files.
> 
> Is there debug code I can add it that'll help us figure out what's up?

Trying to figure out _which_ debugfs file is causing this would be
great, if at all possible.  strace?

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Sasha Levin]

On 05/18/2016 11:01 AM, Nicolai Stange wrote:
> Thanks a million for reporting!
> 
> 1.) Do you have lockdep enabled?

Yup, nothing there.

> 2.) Does this happen before or after userspace init has been spawned,
>     i.e. does the lockup happen at debugfs file creation time or
>     possibly at usage time?

So I looked closer, and it seems to happen after starting syzkaller, which
as far as I know tries to open many different debugfs files.

Is there debug code I can add it that'll help us figure out what's up?


Thanks,
Sasha

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Hi Sasha,

Sasha Levin <sasha.levin@oracle.com> writes:

> On 03/22/2016 09:11 AM, Nicolai Stange wrote:
>> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
>> still be attempted to access associated private file data through
>> previously opened struct file objects. If that data has been freed by
>> the caller of debugfs_remove*() in the meanwhile, the reading/writing
>> process would either encounter a fault or, if the memory address in
>> question has been reassigned again, unrelated data structures could get
>> overwritten.
>> 
>> However, since debugfs files are seldomly removed, usually from module
>> exit handlers only, the impact is very low.
>> 
>> Currently, there are ~1000 call sites of debugfs_create_file() spread
>> throughout the whole tree and touching all of those struct file_operations
>> in order to make them file removal aware by means of checking the result of
>> debugfs_use_file_start() from within their methods is unfeasible.
>> 
>> Instead, wrap the struct file_operations by a lifetime managing proxy at
>> file open:
>> - In debugfs_create_file(), the original fops handed in has got stashed
>>   away in ->d_fsdata already.
>> - In debugfs_create_file(), install a proxy file_operations factory,
>>   debugfs_full_proxy_file_operations, at ->i_fop.
>> 
>> This proxy factory has got an ->open() method only. It carries out some
>> lifetime checks and if successful, dynamically allocates and sets up a new
>> struct file_operations proxy at ->f_op. Afterwards, it forwards to the
>> ->open() of the original struct file_operations in ->d_fsdata, if any.
>> 
>> The dynamically set up proxy at ->f_op has got a lifetime managing wrapper
>> set for each of the methods defined in the original struct file_operations
>> in ->d_fsdata.
>> 
>> Its ->release()er frees the proxy again and forwards to the original
>> ->release(), if any.
>> 
>> In order not to mislead the VFS layer, it is strictly necessary to leave
>> those fields blank in the proxy that have been NULL in the original
>> struct file_operations also, i.e. aren't supported. This is why there is a
>> need for dynamically allocated proxies. The choice made not to allocate a
>> proxy instance for every dentry at file creation, but for every
>> struct file object instantiated thereof is justified by the expected usage
>> pattern of debugfs, namely that in general very few files get opened more
>> than once at a time.
>> 
>> The wrapper methods set in the struct file_operations implement lifetime
>> managing by means of the SRCU protection facilities already in place for
>> debugfs:
>> They set up a SRCU read side critical section and check whether the dentry
>> is still alive by means of debugfs_use_file_start(). If so, they forward
>> the call to the original struct file_operation stored in ->d_fsdata, still
>> under the protection of the SRCU read side critical section.
>> This SRCU read side critical section prevents any pending debugfs_remove()
>> and friends to return to their callers. Since a file's private data must
>> only be freed after the return of debugfs_remove(), the ongoing proxied
>> call is guarded against any file removal race.
>> 
>> If, on the other hand, the initial call to debugfs_use_file_start() detects
>> that the dentry is dead, the wrapper simply returns -EIO and does not
>> forward the call. Note that the ->poll() wrapper is special in that its
>> signature does not allow for the return of arbitrary -EXXX values and thus,
>> POLLHUP is returned here.
>> 
>> In order not to pollute debugfs with wrapper definitions that aren't ever
>> needed, I chose not to define a wrapper for every struct file_operations
>> method possible. Instead, a wrapper is defined only for the subset of
>> methods which are actually set by any debugfs users.
>> Currently, these are:
>> 
>>   ->llseek()
>>   ->read()
>>   ->write()
>>   ->unlocked_ioctl()
>>   ->poll()
>> 
>> The ->release() wrapper is special in that it does not protect the original
>> ->release() in any way from dead files in order not to leak resources.
>> Thus, any ->release() handed to debugfs must implement file lifetime
>> management manually, if needed.
>> For only 33 out of a total of 434 releasers handed in to debugfs, it could
>> not be verified immediately whether they access data structures that might
>> have been freed upon a debugfs_remove() return in the meanwhile.
>> 
>> Export debugfs_use_file_start() and debugfs_use_file_finish() in order to
>> allow any ->release() to manually implement file lifetime management.
>> 
>> For a set of common cases of struct file_operations implemented by the
>> debugfs_core itself, future patches will incorporate file lifetime
>> management directly within those in order to allow for their unproxied
>> operation. Rename the original, non-proxying "debugfs_create_file()" to
>> "debugfs_create_file_unsafe()" and keep it for future internal use by
>> debugfs itself. Factor out code common to both into the new
>> __debugfs_create_file().
>> 
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>

Thanks a million for reporting!

1.) Do you have lockdep enabled?

2.) Does this happen before or after userspace init has been spawned,
    i.e. does the lockup happen at debugfs file creation time or
    possibly at usage time?

Thank you,

Nicolai

Re: [PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Sasha Levin]

On 03/22/2016 09:11 AM, Nicolai Stange wrote:
> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
> still be attempted to access associated private file data through
> previously opened struct file objects. If that data has been freed by
> the caller of debugfs_remove*() in the meanwhile, the reading/writing
> process would either encounter a fault or, if the memory address in
> question has been reassigned again, unrelated data structures could get
> overwritten.
> 
> However, since debugfs files are seldomly removed, usually from module
> exit handlers only, the impact is very low.
> 
> Currently, there are ~1000 call sites of debugfs_create_file() spread
> throughout the whole tree and touching all of those struct file_operations
> in order to make them file removal aware by means of checking the result of
> debugfs_use_file_start() from within their methods is unfeasible.
> 
> Instead, wrap the struct file_operations by a lifetime managing proxy at
> file open:
> - In debugfs_create_file(), the original fops handed in has got stashed
>   away in ->d_fsdata already.
> - In debugfs_create_file(), install a proxy file_operations factory,
>   debugfs_full_proxy_file_operations, at ->i_fop.
> 
> This proxy factory has got an ->open() method only. It carries out some
> lifetime checks and if successful, dynamically allocates and sets up a new
> struct file_operations proxy at ->f_op. Afterwards, it forwards to the
> ->open() of the original struct file_operations in ->d_fsdata, if any.
> 
> The dynamically set up proxy at ->f_op has got a lifetime managing wrapper
> set for each of the methods defined in the original struct file_operations
> in ->d_fsdata.
> 
> Its ->release()er frees the proxy again and forwards to the original
> ->release(), if any.
> 
> In order not to mislead the VFS layer, it is strictly necessary to leave
> those fields blank in the proxy that have been NULL in the original
> struct file_operations also, i.e. aren't supported. This is why there is a
> need for dynamically allocated proxies. The choice made not to allocate a
> proxy instance for every dentry at file creation, but for every
> struct file object instantiated thereof is justified by the expected usage
> pattern of debugfs, namely that in general very few files get opened more
> than once at a time.
> 
> The wrapper methods set in the struct file_operations implement lifetime
> managing by means of the SRCU protection facilities already in place for
> debugfs:
> They set up a SRCU read side critical section and check whether the dentry
> is still alive by means of debugfs_use_file_start(). If so, they forward
> the call to the original struct file_operation stored in ->d_fsdata, still
> under the protection of the SRCU read side critical section.
> This SRCU read side critical section prevents any pending debugfs_remove()
> and friends to return to their callers. Since a file's private data must
> only be freed after the return of debugfs_remove(), the ongoing proxied
> call is guarded against any file removal race.
> 
> If, on the other hand, the initial call to debugfs_use_file_start() detects
> that the dentry is dead, the wrapper simply returns -EIO and does not
> forward the call. Note that the ->poll() wrapper is special in that its
> signature does not allow for the return of arbitrary -EXXX values and thus,
> POLLHUP is returned here.
> 
> In order not to pollute debugfs with wrapper definitions that aren't ever
> needed, I chose not to define a wrapper for every struct file_operations
> method possible. Instead, a wrapper is defined only for the subset of
> methods which are actually set by any debugfs users.
> Currently, these are:
> 
>   ->llseek()
>   ->read()
>   ->write()
>   ->unlocked_ioctl()
>   ->poll()
> 
> The ->release() wrapper is special in that it does not protect the original
> ->release() in any way from dead files in order not to leak resources.
> Thus, any ->release() handed to debugfs must implement file lifetime
> management manually, if needed.
> For only 33 out of a total of 434 releasers handed in to debugfs, it could
> not be verified immediately whether they access data structures that might
> have been freed upon a debugfs_remove() return in the meanwhile.
> 
> Export debugfs_use_file_start() and debugfs_use_file_finish() in order to
> allow any ->release() to manually implement file lifetime management.
> 
> For a set of common cases of struct file_operations implemented by the
> debugfs_core itself, future patches will incorporate file lifetime
> management directly within those in order to allow for their unproxied
> operation. Rename the original, non-proxying "debugfs_create_file()" to
> "debugfs_create_file_unsafe()" and keep it for future internal use by
> debugfs itself. Factor out code common to both into the new
> __debugfs_create_file().
> 
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>

Hey,

I'm seeing a hang on boot which was bisected (twice) to this commit. I don't
see any lockup messages, but everything just seems to stop.


Thanks,
Sasha

[PATCH v6 2/8] debugfs: prevent access to removed files' private data

[Nicolai Stange]

Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
still be attempted to access associated private file data through
previously opened struct file objects. If that data has been freed by
the caller of debugfs_remove*() in the meanwhile, the reading/writing
process would either encounter a fault or, if the memory address in
question has been reassigned again, unrelated data structures could get
overwritten.

However, since debugfs files are seldomly removed, usually from module
exit handlers only, the impact is very low.

Currently, there are ~1000 call sites of debugfs_create_file() spread
throughout the whole tree and touching all of those struct file_operations
in order to make them file removal aware by means of checking the result of
debugfs_use_file_start() from within their methods is unfeasible.

Instead, wrap the struct file_operations by a lifetime managing proxy at
file open:
- In debugfs_create_file(), the original fops handed in has got stashed
  away in ->d_fsdata already.
- In debugfs_create_file(), install a proxy file_operations factory,
  debugfs_full_proxy_file_operations, at ->i_fop.

This proxy factory has got an ->open() method only. It carries out some
lifetime checks and if successful, dynamically allocates and sets up a new
struct file_operations proxy at ->f_op. Afterwards, it forwards to the
->open() of the original struct file_operations in ->d_fsdata, if any.

The dynamically set up proxy at ->f_op has got a lifetime managing wrapper
set for each of the methods defined in the original struct file_operations
in ->d_fsdata.

Its ->release()er frees the proxy again and forwards to the original
->release(), if any.

In order not to mislead the VFS layer, it is strictly necessary to leave
those fields blank in the proxy that have been NULL in the original
struct file_operations also, i.e. aren't supported. This is why there is a
need for dynamically allocated proxies. The choice made not to allocate a
proxy instance for every dentry at file creation, but for every
struct file object instantiated thereof is justified by the expected usage
pattern of debugfs, namely that in general very few files get opened more
than once at a time.

The wrapper methods set in the struct file_operations implement lifetime
managing by means of the SRCU protection facilities already in place for
debugfs:
They set up a SRCU read side critical section and check whether the dentry
is still alive by means of debugfs_use_file_start(). If so, they forward
the call to the original struct file_operation stored in ->d_fsdata, still
under the protection of the SRCU read side critical section.
This SRCU read side critical section prevents any pending debugfs_remove()
and friends to return to their callers. Since a file's private data must
only be freed after the return of debugfs_remove(), the ongoing proxied
call is guarded against any file removal race.

If, on the other hand, the initial call to debugfs_use_file_start() detects
that the dentry is dead, the wrapper simply returns -EIO and does not
forward the call. Note that the ->poll() wrapper is special in that its
signature does not allow for the return of arbitrary -EXXX values and thus,
POLLHUP is returned here.

In order not to pollute debugfs with wrapper definitions that aren't ever
needed, I chose not to define a wrapper for every struct file_operations
method possible. Instead, a wrapper is defined only for the subset of
methods which are actually set by any debugfs users.
Currently, these are:

  ->llseek()
  ->read()
  ->write()
  ->unlocked_ioctl()
  ->poll()

The ->release() wrapper is special in that it does not protect the original
->release() in any way from dead files in order not to leak resources.
Thus, any ->release() handed to debugfs must implement file lifetime
management manually, if needed.
For only 33 out of a total of 434 releasers handed in to debugfs, it could
not be verified immediately whether they access data structures that might
have been freed upon a debugfs_remove() return in the meanwhile.

Export debugfs_use_file_start() and debugfs_use_file_finish() in order to
allow any ->release() to manually implement file lifetime management.

For a set of common cases of struct file_operations implemented by the
debugfs_core itself, future patches will incorporate file lifetime
management directly within those in order to allow for their unproxied
operation. Rename the original, non-proxying "debugfs_create_file()" to
"debugfs_create_file_unsafe()" and keep it for future internal use by
debugfs itself. Factor out code common to both into the new
__debugfs_create_file().

Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
 fs/debugfs/file.c       | 157 +++++++++++++++++++++++++++++++++++++++++++++++-
 fs/debugfs/inode.c      |  70 ++++++++++++++-------
 fs/debugfs/internal.h   |   6 +-
 include/linux/debugfs.h |  20 ++++++
 4 files changed, 226 insertions(+), 27 deletions(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 736ab3c..6eb58a8 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -23,9 +23,12 @@
 #include <linux/atomic.h>
 #include <linux/device.h>
 #include <linux/srcu.h>
+#include <asm/poll.h>
 
 #include "internal.h"
 
+struct poll_table_struct;
+
 static ssize_t default_read_file(struct file *file, char __user *buf,
 				 size_t count, loff_t *ppos)
 {
@@ -66,7 +69,7 @@ const struct file_operations debugfs_noop_file_operations = {
  * debugfs_use_file_start() must be followed by a matching call
  * to debugfs_use_file_finish().
  */
-static int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
+int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 	__acquires(&debugfs_srcu)
 {
 	*srcu_idx = srcu_read_lock(&debugfs_srcu);
@@ -75,6 +78,7 @@ static int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
 		return -EIO;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(debugfs_use_file_start);
 
 /**
  * debugfs_use_file_finish - mark the end of file data access
@@ -85,10 +89,11 @@ static int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
  * debugfs_remove_recursive() blocked by a former call to
  * debugfs_use_file_start() to proceed and return to its caller.
  */
-static void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
+void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu)
 {
 	srcu_read_unlock(&debugfs_srcu, srcu_idx);
 }
+EXPORT_SYMBOL_GPL(debugfs_use_file_finish);
 
 #define F_DENTRY(filp) ((filp)->f_path.dentry)
 
@@ -131,6 +136,154 @@ const struct file_operations debugfs_open_proxy_file_operations = {
 	.open = open_proxy_open,
 };
 
+#define PROTO(args...) args
+#define ARGS(args...) args
+
+#define FULL_PROXY_FUNC(name, ret_type, filp, proto, args)		\
+static ret_type full_proxy_ ## name(proto)				\
+{									\
+	const struct dentry *dentry = F_DENTRY(filp);			\
+	const struct file_operations *real_fops =			\
+		REAL_FOPS_DEREF(dentry);				\
+	int srcu_idx;							\
+	ret_type r;							\
+									\
+	r = debugfs_use_file_start(dentry, &srcu_idx);			\
+	if (likely(!r))						\
+		r = real_fops->name(args);				\
+	debugfs_use_file_finish(srcu_idx);				\
+	return r;							\
+}
+
+FULL_PROXY_FUNC(llseek, loff_t, filp,
+		PROTO(struct file *filp, loff_t offset, int whence),
+		ARGS(filp, offset, whence));
+
+FULL_PROXY_FUNC(read, ssize_t, filp,
+		PROTO(struct file *filp, char __user *buf, size_t size,
+			loff_t *ppos),
+		ARGS(filp, buf, size, ppos));
+
+FULL_PROXY_FUNC(write, ssize_t, filp,
+		PROTO(struct file *filp, const char __user *buf, size_t size,
+			loff_t *ppos),
+		ARGS(filp, buf, size, ppos));
+
+FULL_PROXY_FUNC(unlocked_ioctl, long, filp,
+		PROTO(struct file *filp, unsigned int cmd, unsigned long arg),
+		ARGS(filp, cmd, arg));
+
+static unsigned int full_proxy_poll(struct file *filp,
+				struct poll_table_struct *wait)
+{
+	const struct dentry *dentry = F_DENTRY(filp);
+	const struct file_operations *real_fops = REAL_FOPS_DEREF(dentry);
+	int srcu_idx;
+	unsigned int r = 0;
+
+	if (debugfs_use_file_start(dentry, &srcu_idx)) {
+		debugfs_use_file_finish(srcu_idx);
+		return POLLHUP;
+	}
+
+	r = real_fops->poll(filp, wait);
+	debugfs_use_file_finish(srcu_idx);
+	return r;
+}
+
+static int full_proxy_release(struct inode *inode, struct file *filp)
+{
+	const struct dentry *dentry = F_DENTRY(filp);
+	const struct file_operations *real_fops = REAL_FOPS_DEREF(dentry);
+	const struct file_operations *proxy_fops = filp->f_op;
+	int r = 0;
+
+	/*
+	 * We must not protect this against removal races here: the
+	 * original releaser should be called unconditionally in order
+	 * not to leak any resources. Releasers must not assume that
+	 * ->i_private is still being meaningful here.
+	 */
+	if (real_fops->release)
+		r = real_fops->release(inode, filp);
+
+	replace_fops(filp, d_inode(dentry)->i_fop);
+	kfree((void *)proxy_fops);
+	fops_put(real_fops);
+	return 0;
+}
+
+static void __full_proxy_fops_init(struct file_operations *proxy_fops,
+				const struct file_operations *real_fops)
+{
+	proxy_fops->release = full_proxy_release;
+	if (real_fops->llseek)
+		proxy_fops->llseek = full_proxy_llseek;
+	if (real_fops->read)
+		proxy_fops->read = full_proxy_read;
+	if (real_fops->write)
+		proxy_fops->write = full_proxy_write;
+	if (real_fops->poll)
+		proxy_fops->poll = full_proxy_poll;
+	if (real_fops->unlocked_ioctl)
+		proxy_fops->unlocked_ioctl = full_proxy_unlocked_ioctl;
+}
+
+static int full_proxy_open(struct inode *inode, struct file *filp)
+{
+	const struct dentry *dentry = F_DENTRY(filp);
+	const struct file_operations *real_fops = NULL;
+	struct file_operations *proxy_fops = NULL;
+	int srcu_idx, r;
+
+	r = debugfs_use_file_start(dentry, &srcu_idx);
+	if (r) {
+		r = -ENOENT;
+		goto out;
+	}
+
+	real_fops = REAL_FOPS_DEREF(dentry);
+	real_fops = fops_get(real_fops);
+	if (!real_fops) {
+		/* Huh? Module did not cleanup after itself at exit? */
+		WARN(1, "debugfs file owner did not clean up at exit: %pd",
+			dentry);
+		r = -ENXIO;
+		goto out;
+	}
+
+	proxy_fops = kzalloc(sizeof(*proxy_fops), GFP_KERNEL);
+	if (!proxy_fops) {
+		r = -ENOMEM;
+		goto free_proxy;
+	}
+	__full_proxy_fops_init(proxy_fops, real_fops);
+	replace_fops(filp, proxy_fops);
+
+	if (real_fops->open) {
+		r = real_fops->open(inode, filp);
+
+		if (filp->f_op != proxy_fops) {
+			/* No protection against file removal anymore. */
+			WARN(1, "debugfs file owner replaced proxy fops: %pd",
+				dentry);
+			goto free_proxy;
+		}
+	}
+
+	goto out;
+free_proxy:
+	kfree(proxy_fops);
+	fops_put(real_fops);
+out:
+	debugfs_use_file_finish(srcu_idx);
+	return r;
+}
+
+const struct file_operations debugfs_full_proxy_file_operations = {
+	.open = full_proxy_open,
+};
+
 static struct dentry *debugfs_create_mode(const char *name, umode_t mode,
 					  struct dentry *parent, void *value,
 				          const struct file_operations *fops,
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 26880ad..42a9b34 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -299,6 +299,37 @@ static struct dentry *end_creating(struct dentry *dentry)
 	return dentry;
 }
 
+static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
+				struct dentry *parent, void *data,
+				const struct file_operations *proxy_fops,
+				const struct file_operations *real_fops)
+{
+	struct dentry *dentry;
+	struct inode *inode;
+
+	if (!(mode & S_IFMT))
+		mode |= S_IFREG;
+	BUG_ON(!S_ISREG(mode));
+	dentry = start_creating(name, parent);
+
+	if (IS_ERR(dentry))
+		return NULL;
+
+	inode = debugfs_get_inode(dentry->d_sb);
+	if (unlikely(!inode))
+		return failed_creating(dentry);
+
+	inode->i_mode = mode;
+	inode->i_private = data;
+
+	inode->i_fop = proxy_fops;
+	dentry->d_fsdata = (void *)real_fops;
+
+	d_instantiate(dentry, inode);
+	fsnotify_create(d_inode(dentry->d_parent), dentry);
+	return end_creating(dentry);
+}
+
 /**
  * debugfs_create_file - create a file in the debugfs filesystem
  * @name: a pointer to a string containing the name of the file to create.
@@ -329,33 +360,24 @@ struct dentry *debugfs_create_file(const char *name, umode_t mode,
 				   struct dentry *parent, void *data,
 				   const struct file_operations *fops)
 {
-	struct dentry *dentry;
-	struct inode *inode;
-
-	if (!(mode & S_IFMT))
-		mode |= S_IFREG;
-	BUG_ON(!S_ISREG(mode));
-	dentry = start_creating(name, parent);
-
-	if (IS_ERR(dentry))
-		return NULL;
-
-	inode = debugfs_get_inode(dentry->d_sb);
-	if (unlikely(!inode))
-		return failed_creating(dentry);
 
-	inode->i_mode = mode;
-	inode->i_private = data;
+	return __debugfs_create_file(name, mode, parent, data,
+				fops ? &debugfs_full_proxy_file_operations :
+					&debugfs_noop_file_operations,
+				fops);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_file);
 
-	inode->i_fop = fops ? &debugfs_open_proxy_file_operations
-		: &debugfs_noop_file_operations;
-	dentry->d_fsdata = (void *)fops;
+struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
+				   struct dentry *parent, void *data,
+				   const struct file_operations *fops)
+{
 
-	d_instantiate(dentry, inode);
-	fsnotify_create(d_inode(dentry->d_parent), dentry);
-	return end_creating(dentry);
+	return __debugfs_create_file(name, mode, parent, data,
+				fops ? &debugfs_open_proxy_file_operations :
+					&debugfs_noop_file_operations,
+				fops);
 }
-EXPORT_SYMBOL_GPL(debugfs_create_file);
 
 /**
  * debugfs_create_file_size - create a file in the debugfs filesystem
@@ -574,6 +596,7 @@ void debugfs_remove(struct dentry *dentry)
 	inode_unlock(d_inode(parent));
 	if (!ret)
 		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+
 	synchronize_srcu(&debugfs_srcu);
 }
 EXPORT_SYMBOL_GPL(debugfs_remove);
@@ -652,6 +675,7 @@ void debugfs_remove_recursive(struct dentry *dentry)
 	if (!__debugfs_remove(child, parent))
 		simple_release_fs(&debugfs_mount, &debugfs_mount_count);
 	inode_unlock(d_inode(parent));
+
 	synchronize_srcu(&debugfs_srcu);
 }
 EXPORT_SYMBOL_GPL(debugfs_remove_recursive);
diff --git a/fs/debugfs/internal.h b/fs/debugfs/internal.h
index c7aaa5c..bba5263 100644
--- a/fs/debugfs/internal.h
+++ b/fs/debugfs/internal.h
@@ -13,12 +13,14 @@
 #define _DEBUGFS_INTERNAL_H_
 
 struct file_operations;
-struct srcu_struct;
 
 /* declared over in file.c */
 extern const struct file_operations debugfs_noop_file_operations;
 extern const struct file_operations debugfs_open_proxy_file_operations;
+extern const struct file_operations debugfs_full_proxy_file_operations;
 
-extern struct srcu_struct debugfs_srcu;
+struct dentry *debugfs_create_file_unsafe(const char *name, umode_t mode,
+					struct dentry *parent, void *data,
+					const struct file_operations *fops);
 
 #endif /* _DEBUGFS_INTERNAL_H_ */
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index fcafe2d..a63e6ea 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -19,9 +19,11 @@
 #include <linux/seq_file.h>
 
 #include <linux/types.h>
+#include <linux/compiler.h>
 
 struct device;
 struct file_operations;
+struct srcu_struct;
 
 struct debugfs_blob_wrapper {
 	void *data;
@@ -41,6 +43,8 @@ struct debugfs_regset32 {
 
 extern struct dentry *arch_debugfs_dir;
 
+extern struct srcu_struct debugfs_srcu;
+
 #if defined(CONFIG_DEBUG_FS)
 
 struct dentry *debugfs_create_file(const char *name, umode_t mode,
@@ -65,6 +69,11 @@ struct dentry *debugfs_create_automount(const char *name,
 void debugfs_remove(struct dentry *dentry);
 void debugfs_remove_recursive(struct dentry *dentry);
 
+int debugfs_use_file_start(const struct dentry *dentry, int *srcu_idx)
+	__acquires(&debugfs_srcu);
+
+void debugfs_use_file_finish(int srcu_idx) __releases(&debugfs_srcu);
+
 struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, const char *new_name);
 
@@ -173,6 +182,17 @@ static inline void debugfs_remove(struct dentry *dentry)
 static inline void debugfs_remove_recursive(struct dentry *dentry)
 { }
 
+static inline int debugfs_use_file_start(const struct dentry *dentry,
+					int *srcu_idx)
+	__acquires(&debugfs_srcu)
+{
+	return 0;
+}
+
+static inline void debugfs_use_file_finish(int srcu_idx)
+	__releases(&debugfs_srcu)
+{ }
+
 static inline struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
                 struct dentry *new_dir, char *new_name)
 {
-- 
2.7.4