debugfs_remove_recursive() while a file is in use by userspace

debugfs_remove_recursive() while a file is in use by userspace

[Rajat Jain]

Hi,

I wanted to understand the behavior taken when a module calls
debugfs_remove_recursive() on a directory, while files under that
directory may still be in use by the userspace (for instance an
ongoing read / write operation).

Does the function wait

(1) until all the currently executing file operation methods
(read/write/map etc) have returned?
OR
(2) until the user has given up all references (descriptors) to the
files under the directory (i.e. until release() method has been
called)?

Thanks,

Rajat

Re: debugfs_remove_recursive() while a file is in use by userspace

[Al Viro]

On Mon, Dec 28, 2015 at 12:27:22PM -0800, Rajat Jain wrote:
> Hi,
> 
> I wanted to understand the behavior taken when a module calls
> debugfs_remove_recursive() on a directory, while files under that
> directory may still be in use by the userspace (for instance an
> ongoing read / write operation).
> 
> Does the function wait
> 
> (1) until all the currently executing file operation methods
> (read/write/map etc) have returned?

No

> OR
> (2) until the user has given up all references (descriptors) to the
> files under the directory (i.e. until release() method has been
> called)?

... and no.  Don't use debugfs for objects that might have a finite
lifetime, or you'll get trouble.  And don't use debugfs outside of
well-isolated testing systems.

Re: debugfs_remove_recursive() while a file is in use by userspace

[Greg Kroah-Hartman]

On Mon, Dec 28, 2015 at 12:27:22PM -0800, Rajat Jain wrote:
> Hi,
> 
> I wanted to understand the behavior taken when a module calls
> debugfs_remove_recursive() on a directory, while files under that
> directory may still be in use by the userspace (for instance an
> ongoing read / write operation).

Bad things :(

> Does the function wait
> 
> (1) until all the currently executing file operation methods
> (read/write/map etc) have returned?

Nope.

> OR
> (2) until the user has given up all references (descriptors) to the
> files under the directory (i.e. until release() method has been
> called)?

Nope.

There are some patches on the mailing list that I need to review that
hopefully should resolve this problem, as it's been known for a very
long time.

In short, just don't remove debugfs files unless your module is
unloading, and all should be good as modules are never auto-unloaded.
If you remove debugfs files when a device is removed, be careful.

thanks,

greg k-h

Re: debugfs_remove_recursive() while a file is in use by userspace

[Rajat Jain]

Thanks Greg and Al for the quick turnaround.

Essentially I have a device that supports something called "contexts"
that can be "created" and "destroyed" during the life of the device. I
want to expose some debug files for the context when it is created,
and destroy the files when the context is destroyed. However, I'm not
sure how do I ensure that the user is not in the middle of reading /
writing / mmaping to those files. Also how do I know that user is
still not holding a reference to the file structure.

It seems like debugfs is currently not a good choice for this? Would
you recommend me to any other fs or subsystem that I should use for
this?

Would hanging those files under the sysfs node for the device sound
any better (by representing each "context" using an embedded kobject)?

Thanks,

Rajat


On Mon, Dec 28, 2015 at 12:31 PM, Greg Kroah-Hartman <greg@kroah.com> wrote:
> On Mon, Dec 28, 2015 at 12:27:22PM -0800, Rajat Jain wrote:
>> Hi,
>>
>> I wanted to understand the behavior taken when a module calls
>> debugfs_remove_recursive() on a directory, while files under that
>> directory may still be in use by the userspace (for instance an
>> ongoing read / write operation).
>
> Bad things :(
>
>> Does the function wait
>>
>> (1) until all the currently executing file operation methods
>> (read/write/map etc) have returned?
>
> Nope.
>
>> OR
>> (2) until the user has given up all references (descriptors) to the
>> files under the directory (i.e. until release() method has been
>> called)?
>
> Nope.
>
> There are some patches on the mailing list that I need to review that
> hopefully should resolve this problem, as it's been known for a very
> long time.
>
> In short, just don't remove debugfs files unless your module is
> unloading, and all should be good as modules are never auto-unloaded.
> If you remove debugfs files when a device is removed, be careful.
>
> thanks,
>
> greg k-h

Re: debugfs_remove_recursive() while a file is in use by userspace

[Greg Kroah-Hartman]

On Mon, Dec 28, 2015 at 12:51:32PM -0800, Rajat Jain wrote:
> Thanks Greg and Al for the quick turnaround.
> 
> Essentially I have a device that supports something called "contexts"
> that can be "created" and "destroyed" during the life of the device. I
> want to expose some debug files for the context when it is created,
> and destroy the files when the context is destroyed. However, I'm not
> sure how do I ensure that the user is not in the middle of reading /
> writing / mmaping to those files. Also how do I know that user is
> still not holding a reference to the file structure.

You don't.

> It seems like debugfs is currently not a good choice for this? Would
> you recommend me to any other fs or subsystem that I should use for
> this?

What exactly do you need to export to userspace and for what purpose?
For debugging-only stuff, sure, use debugfs, but don't rely on it for
any "real" tools, only your own debugging.

> Would hanging those files under the sysfs node for the device sound
> any better (by representing each "context" using an embedded kobject)?

That would ensure that things work properly.  But you don't need a whole
kobject, just use a named group and a subdir will be created properly
for you.

good luck,

greg k-h

Re: debugfs_remove_recursive() while a file is in use by userspace

[Rajat Jain]

On Mon, Dec 28, 2015 at 12:58 PM, Greg Kroah-Hartman <greg@kroah.com> wrote:
> On Mon, Dec 28, 2015 at 12:51:32PM -0800, Rajat Jain wrote:
>> Thanks Greg and Al for the quick turnaround.
>>
>> Essentially I have a device that supports something called "contexts"
>> that can be "created" and "destroyed" during the life of the device. I
>> want to expose some debug files for the context when it is created,
>> and destroy the files when the context is destroyed. However, I'm not
>> sure how do I ensure that the user is not in the middle of reading /
>> writing / mmaping to those files. Also how do I know that user is
>> still not holding a reference to the file structure.
>
> You don't.
>
>> It seems like debugfs is currently not a good choice for this? Would
>> you recommend me to any other fs or subsystem that I should use for
>> this?
>
> What exactly do you need to export to userspace and for what purpose?
> For debugging-only stuff, sure, use debugfs, but don't rely on it for
> any "real" tools, only your own debugging.

I'm actually writing a driver that would expose a "dummy device" to a
real driver. The dummy driver relies on user space to feed in the
device attributes (no of supported contexts etc). I am now thinking
that a character device interface to user space may actually be a
better choice. Question: Does cdev_del() ensure that all references to
the file are dropped before it returns?

Thanks,

Rajat

>
>> Would hanging those files under the sysfs node for the device sound
>> any better (by representing each "context" using an embedded kobject)?
>
> That would ensure that things work properly.  But you don't need a whole
> kobject, just use a named group and a subdir will be created properly
> for you.
>
> good luck,
>
> greg k-h

Re: debugfs_remove_recursive() while a file is in use by userspace

[Greg Kroah-Hartman]

On Mon, Dec 28, 2015 at 01:11:53PM -0800, Rajat Jain wrote:
> On Mon, Dec 28, 2015 at 12:58 PM, Greg Kroah-Hartman <greg@kroah.com> wrote:
> > On Mon, Dec 28, 2015 at 12:51:32PM -0800, Rajat Jain wrote:
> >> Thanks Greg and Al for the quick turnaround.
> >>
> >> Essentially I have a device that supports something called "contexts"
> >> that can be "created" and "destroyed" during the life of the device. I
> >> want to expose some debug files for the context when it is created,
> >> and destroy the files when the context is destroyed. However, I'm not
> >> sure how do I ensure that the user is not in the middle of reading /
> >> writing / mmaping to those files. Also how do I know that user is
> >> still not holding a reference to the file structure.
> >
> > You don't.
> >
> >> It seems like debugfs is currently not a good choice for this? Would
> >> you recommend me to any other fs or subsystem that I should use for
> >> this?
> >
> > What exactly do you need to export to userspace and for what purpose?
> > For debugging-only stuff, sure, use debugfs, but don't rely on it for
> > any "real" tools, only your own debugging.
> 
> I'm actually writing a driver that would expose a "dummy device" to a
> real driver. The dummy driver relies on user space to feed in the
> device attributes (no of supported contexts etc).

That sounds "odd".

Why not configfs?

> I am now thinking that a character device interface to user space may
> actually be a better choice.

Not really, you would now have to parse things in a character driver
write() callback, not very good or robust.

> Question: Does cdev_del() ensure that all references to the file are
> dropped before it returns?

No, but when the reference is dropped, everything cleans up properly.

thanks,

greg k-h

Re: debugfs_remove_recursive() while a file is in use by userspace

[Nicolai Stange]

Hi Rajat,

Rajat Jain <rajatxjain@gmail.com> writes:

> Hi,
>
> I wanted to understand the behavior taken when a module calls
> debugfs_remove_recursive() on a directory, while files under that
> directory may still be in use by the userspace (for instance an
> ongoing read / write operation).
>
> Does the function wait
>
> (1) until all the currently executing file operation methods
> (read/write/map etc) have returned?
> OR
> (2) until the user has given up all references (descriptors) to the
> files under the directory (i.e. until release() method has been
> called)?

The current state is that both question are to be answered with "no",
i.e. debugfs file removal is racy.

I've recently sent a patch addressing this issue:
  https://lkml.kernel.org/g/87y4dfukzy.fsf@gmail.com

Basically, it turns the answer to your first question into "yes".
Subsequent reads/writes will return a -EIO.

That patch (series) is still under review though.

Further reference can be found in the *sub*-thread rooted at
  http://thread.gmane.org/gmane.linux.kernel/1452470/focus=1467314

Best,

Nicolai

Re: debugfs_remove_recursive() while a file is in use by userspace

[Rajat Jain]

>
> The current state is that both question are to be answered with "no",
> i.e. debugfs file removal is racy.
>

Ouch! :-(. Thanks for confirming!

Somewhat related, is my understanding correct that the behaviour of
character device layer w.r.t. the two questions that I asked the
answer to my questions would be "yes"?

i.e. cdev_del() would wait

>> (1) until all the currently executing file operation methods
>> (read/write/map etc) have returned?

AND

>> (2) until the user has given up all references (descriptors) to the
>> files (i.e. until release() method has been
>> called)?

Thanks,

Rajat


On Mon, Dec 28, 2015 at 12:42 PM, Nicolai Stange <nicstange@gmail.com> wrote:
> Hi Rajat,
>
> Rajat Jain <rajatxjain@gmail.com> writes:
>
>> Hi,
>>
>> I wanted to understand the behavior taken when a module calls
>> debugfs_remove_recursive() on a directory, while files under that
>> directory may still be in use by the userspace (for instance an
>> ongoing read / write operation).
>>
>> Does the function wait
>>
>> (1) until all the currently executing file operation methods
>> (read/write/map etc) have returned?
>> OR
>> (2) until the user has given up all references (descriptors) to the
>> files under the directory (i.e. until release() method has been
>> called)?
>
> The current state is that both question are to be answered with "no",
> i.e. debugfs file removal is racy.
>
> I've recently sent a patch addressing this issue:
>   https://lkml.kernel.org/g/87y4dfukzy.fsf@gmail.com
>
> Basically, it turns the answer to your first question into "yes".
> Subsequent reads/writes will return a -EIO.
>
> That patch (series) is still under review though.
>
> Further reference can be found in the *sub*-thread rooted at
>   http://thread.gmane.org/gmane.linux.kernel/1452470/focus=1467314
>
> Best,
>
> Nicolai