ToUserDevice+FromUserDevice unsafe across /click/config changes about click HOT 6 OPEN

I don't completely agree. ToUserDevice::cleanup() is trying to spin until no one is waiting for it (see the _exit boolean and the loop). That might be insufficient/incorrect, but I think something simpler is missing. cleanup() should walk over elem_map[] and set all entries that equal this to null instead. Then when a user program calls back into the device they'll exit immediately.

from click.

What if we're in the middle of ToUserDevice::dev_read when /dev/config is written?

I guess it doesn't matter why the Element is being destroyed (so /dev/config is incidental to the problem) but _lock is not held around all places where elem should be valid and since it is a member we get into trouble if elem is destroyed in the meantime.

Since _proc_queue and _lock are members, using them is problematic to begin with. Perhaps all that is needed here is a global lock that controls access to elem_map and an atomic ref count on TUDs that indicates whether cleanup can go forward.

SMP version of the issue:

CPU0
ToUserDevice::dev_read
  ToUserDevice *elem = GETELEM(filp); // valid here
  // _lock is not held and we are not in _proc_queue

CPU1
ToUserDevice::cleanup
  elem_map[_dev_minor] = 0;
  ...
~ToUserDevice

CPU0
  elem->_read_count++; // crash!

from click.

If we;'re in the middle of dev_read then we should have the lock. That's
why the destructor for ToUD should acquire the lock...

On Wed, Apr 30, 2014 at 12:19 PM, pallas [email protected] wrote:

What if we're in the middle of ToUserDevice::dev_read when /dev/config is
written?

I guess it doesn't matter why the Element is being destroyed (so
/dev/config is incidental to the problem) but _lock is not held around
all places where elem should be valid and since it is a member we get
into trouble if elem is destroyed in the meantime.

Since _proc_queue and _lock are members, using them is problematic to
begin with. Perhaps all that is needed here is a global lock that controls
access to elem_map and an atomic ref count on TUDs that indicates whether
cleanup can go forward.

SMP version of the issue:

CPU0
ToUserDevice::dev_read
ToUserDevice *elem = GETELEM(filp); // valid here
// _lock is not held and we are not in _proc_queue

CPU1
ToUserDevice::cleanup
elem_map[_dev_minor] = 0;
...
~ToUserDevice

CPU0
elem->_read_count++; // crash!

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/123#issuecomment-41816870
.

from click.

Sorry, I ahve a deadline so take my effluvia as weak. But there is
definitely a way to solve this problem with locks, perhaps involving
rw-locks around the device map (GETELEM).

On Wed, Apr 30, 2014 at 12:20 PM, Eddie Kohler [email protected] wrote:

If we;'re in the middle of dev_read then we should have the lock. That's
why the destructor for ToUD should acquire the lock...

On Wed, Apr 30, 2014 at 12:19 PM, pallas [email protected] wrote:

What if we're in the middle of ToUserDevice::dev_read when /dev/config is
written?

I guess it doesn't matter why the Element is being destroyed (so
/dev/config is incidental to the problem) but _lock is not held around
all places where elem should be valid and since it is a member we get
into trouble if elem is destroyed in the meantime.

Since _proc_queue and _lock are members, using them is problematic to
begin with. Perhaps all that is needed here is a global lock that controls
access to elem_map and an atomic ref count on TUDs that indicates
whether cleanup can go forward.

SMP version of the issue:

CPU0
ToUserDevice::dev_read
ToUserDevice *elem = GETELEM(filp); // valid here
// _lock is not held and we are not in _proc_queue

CPU1
ToUserDevice::cleanup
elem_map[_dev_minor] = 0;
...
~ToUserDevice

CPU0
elem->_read_count++; // crash!

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/123#issuecomment-41816870
.

from click.

So I have good newses and bad news. The bad news is that I verified that this is actually a problem. The good news is that it is very rare and I have a patch, which I'll prepare for pull soon. The other good news is that the more frequent problem was a local change to replace register_chrdev & friends with cdev_init & friends, since the former is deprecated, and I can turn that into a mainline pull as well if you're interested.

from click.

I would love to see cdev_init as a patch, fwiw.

from click.