Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1562843 > unrolled thread

debugfs vs. device removal

Back to article view | Back to linux.kernel

Contents

  debugfs vs. device removal Omar Sandoval <osandov@osandov.com> - 2017-01-19 16:50 +0100
    Re: debugfs vs. device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-01-19 17:00 +0100
      Re: debugfs vs. device removal Jiri Kosina <jikos@kernel.org> - 2017-01-19 17:10 +0100
        Re: debugfs vs. device removal Omar Sandoval <osandov@osandov.com> - 2017-01-19 18:40 +0100
          Re: debugfs vs. device removal Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-01-19 19:30 +0100
            Re: debugfs vs. device removal Omar Sandoval <osandov@osandov.com> - 2017-01-19 20:50 +0100

#1562843 — debugfs vs. device removal

[Multipart message — attachments visible in raw view] — view raw

Hi,

In the block layer, we abuse sysfs to export some per-device debugging
information. I was looking into moving this to debugfs, but I realized
that debugfs doesn't have a mechanism to ensure that a file associated
with a device is safe to use when the device is removed. 

At a quick glance, HID has some per-device information in debugfs.
However, I don't see any sort of protection against a device being
removed. I was easily able to trigger an oops by reading from
/sys/kernel/debug/hid/*/rdesc in a loop and removing the USB device
(trace attached).

How can I safely export per-device debugging information to debugfs?

Thanks,
Omar

[toc] | [next] | [standalone]

#1562860

On Thu, Jan 19, 2017 at 07:48:41AM -0800, Omar Sandoval wrote:
> Hi,
> 
> In the block layer, we abuse sysfs to export some per-device debugging
> information. I was looking into moving this to debugfs, but I realized
> that debugfs doesn't have a mechanism to ensure that a file associated
> with a device is safe to use when the device is removed. 

What do you mean by "safe"?  The race conditions where you remove a file
and still have it open should all now be resolved in 4.8 and 4.9, di dwe
miss something?

> At a quick glance, HID has some per-device information in debugfs.
> However, I don't see any sort of protection against a device being
> removed. I was easily able to trigger an oops by reading from
> /sys/kernel/debug/hid/*/rdesc in a loop and removing the USB device
> (trace attached).
> 
> How can I safely export per-device debugging information to debugfs?

Try 4.9 and see if you can still reproduce this, it should be fixed.  If
not, we might have forgotten to include the .owner field for the hid
debugfs file...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1562861

On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:

> > In the block layer, we abuse sysfs to export some per-device debugging
> > information. I was looking into moving this to debugfs, but I realized
> > that debugfs doesn't have a mechanism to ensure that a file associated
> > with a device is safe to use when the device is removed. 
> 
> What do you mean by "safe"?  The race conditions where you remove a file
> and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> miss something?

This is something else -- Omar is right, hid-debugfs interface is buggy. 
It basically doesn't synchronize the data dumping with device removal, so 
if device is removed and deallocated and the race is hit, it tries to 
dereference struct hid_device which has already been freed.

I'll look into fixing this later today or tomorrow. Basically we'd need to 
synchronize between hid_remove_device() and anything in hid-debug and 
whenever removal is pending, not to try to get any data out of it any more 
and bail immediately. Something like rwlock (debugfs being the reader and 
device removal being the writer) should work.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1562969

On Thu, Jan 19, 2017 at 05:03:48PM +0100, Jiri Kosina wrote:
> On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:
> 
> > > In the block layer, we abuse sysfs to export some per-device debugging
> > > information. I was looking into moving this to debugfs, but I realized
> > > that debugfs doesn't have a mechanism to ensure that a file associated
> > > with a device is safe to use when the device is removed. 
> > 
> > What do you mean by "safe"?  The race conditions where you remove a file
> > and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> > miss something?
> 
> This is something else -- Omar is right, hid-debugfs interface is buggy. 
> It basically doesn't synchronize the data dumping with device removal, so 
> if device is removed and deallocated and the race is hit, it tries to 
> dereference struct hid_device which has already been freed.

Yup, I'm talking about the case where I create a debugfs file and the
data pointer is, say, a struct request_queue. If userspace calls open()
on a debugfs file, then the device goes away, the struct request_queue
is going to get freed and read() will blow up.

If we're talking about objects with a struct kobject (like struct
request_queue), can we just grab an extra reference in open() and drop
it in release()? This allows userspace to keep stuff pinned
indefinitely, but debugfs is root-only and the use-case is usually just
`cat`.

> I'll look into fixing this later today or tomorrow. Basically we'd need to 
> synchronize between hid_remove_device() and anything in hid-debug and 
> whenever removal is pending, not to try to get any data out of it any more 
> and bail immediately. Something like rwlock (debugfs being the reader and 
> device removal being the writer) should work.
> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

[toc] | [prev] | [next] | [standalone]

#1562991

On Thu, Jan 19, 2017 at 09:33:50AM -0800, Omar Sandoval wrote:
> On Thu, Jan 19, 2017 at 05:03:48PM +0100, Jiri Kosina wrote:
> > On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:
> > 
> > > > In the block layer, we abuse sysfs to export some per-device debugging
> > > > information. I was looking into moving this to debugfs, but I realized
> > > > that debugfs doesn't have a mechanism to ensure that a file associated
> > > > with a device is safe to use when the device is removed. 
> > > 
> > > What do you mean by "safe"?  The race conditions where you remove a file
> > > and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> > > miss something?
> > 
> > This is something else -- Omar is right, hid-debugfs interface is buggy. 
> > It basically doesn't synchronize the data dumping with device removal, so 
> > if device is removed and deallocated and the race is hit, it tries to 
> > dereference struct hid_device which has already been freed.
> 
> Yup, I'm talking about the case where I create a debugfs file and the
> data pointer is, say, a struct request_queue. If userspace calls open()
> on a debugfs file, then the device goes away, the struct request_queue
> is going to get freed and read() will blow up.
> 
> If we're talking about objects with a struct kobject (like struct
> request_queue), can we just grab an extra reference in open() and drop
> it in release()? This allows userspace to keep stuff pinned
> indefinitely, but debugfs is root-only and the use-case is usually just
> `cat`.

Again, debugfs got a bunch of changes in the 4.8 and 4.9 timeframe to
resolve this issue.  Try it and see with just a "normal" debugfs file
and see how it works.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1563041

On Thu, Jan 19, 2017 at 07:03:52PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 19, 2017 at 09:33:50AM -0800, Omar Sandoval wrote:
> > On Thu, Jan 19, 2017 at 05:03:48PM +0100, Jiri Kosina wrote:
> > > On Thu, 19 Jan 2017, Greg Kroah-Hartman wrote:
> > > 
> > > > > In the block layer, we abuse sysfs to export some per-device debugging
> > > > > information. I was looking into moving this to debugfs, but I realized
> > > > > that debugfs doesn't have a mechanism to ensure that a file associated
> > > > > with a device is safe to use when the device is removed. 
> > > > 
> > > > What do you mean by "safe"?  The race conditions where you remove a file
> > > > and still have it open should all now be resolved in 4.8 and 4.9, di dwe
> > > > miss something?
> > > 
> > > This is something else -- Omar is right, hid-debugfs interface is buggy. 
> > > It basically doesn't synchronize the data dumping with device removal, so 
> > > if device is removed and deallocated and the race is hit, it tries to 
> > > dereference struct hid_device which has already been freed.
> > 
> > Yup, I'm talking about the case where I create a debugfs file and the
> > data pointer is, say, a struct request_queue. If userspace calls open()
> > on a debugfs file, then the device goes away, the struct request_queue
> > is going to get freed and read() will blow up.
> > 
> > If we're talking about objects with a struct kobject (like struct
> > request_queue), can we just grab an extra reference in open() and drop
> > it in release()? This allows userspace to keep stuff pinned
> > indefinitely, but debugfs is root-only and the use-case is usually just
> > `cat`.
> 
> Again, debugfs got a bunch of changes in the 4.8 and 4.9 timeframe to
> resolve this issue.  Try it and see with just a "normal" debugfs file
> and see how it works.

The change in this area that I see is 49d200deaa68 ("debugfs: prevent
access to removed files' private data"). That went in for 4.7. I'm
pretty confused now since I can't reproduce the oops anymore on either
4.8 or 4.10-rc4. If I see it again I'll be sure to report it, but it
seems like debugfs should just work for what I need. Thanks for the
help, Greg.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web