Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

Arve Hjønnevåg
2009-05-09T00:38:28Z

2009/5/8 Rafael J. Wysocki <rjw@sisk.pl>:
> On Friday 08 May 2009, Arve Hjønnevåg wrote:
>> On Thu, May 7, 2009 at 3:32 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > On Wed 2009-05-06 18:42:59, Arve Hj?nnev?g wrote:
>> >> On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >> > Hi!
>> >> >
>> >> >> Add a misc device, "suspend_blocker", that allows user-space processes
>> >> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> >> >> and to block and unblock suspend. To delete the suspend_blocker, close
>> >> >> the device.
>> >> >
>> >> > Rafael proposed write() interface. I don't think you answered that.
>> >> >
>> >>
>> >> I think an ioctl interface makes more sense. With a write interface
>> >> you either have to do string parsing, or invent some other protocol
>> >> between the driver and user-space.
>> >
>> > Or perhaps just use "userspace/%d" % pid as a name?
>>
>> This would not be as useful. The point of naming the suspend blockers
>> is to that we can easily identify them in the stats and kernel logs.
>> Using the pid has two problems. First, the pid gives no hint about
>> what it is used for, you have to look up the process somewhere else.
>> Second, we use more than one suspend blocker from the same process.
>>
>> >
>> > 1) can't be faked that trivially
>>
>> I don't think this is a big problem. If you don't trust the apps that
>> you give suspend blocker access to use unique names, we can add a
>> prefix. This can be added in a later patch though.
>>
>> >
>> > 2) small / mostly fixed size
>> >
>> > 3) can use nicer write protocol?
>> >
>> I don't think a write protocol is nicer. "ioctl(suspend_block_fd,
>> SUSPEND_BLOCKER_IOCTL_BLOCK);" seems less error prone and more
>> readable to me than "write(suspend_block_fd, "1", 1);",
>> "write(suspend_block_fd, "1", 2);" or "suspend_block_val = 1;
>> write(suspend_block_fd, &suspend_block_val,
>> sizeof(suspend_block_val));".
>>
>> >> > kzalloc on user-supplied argument. Sounds like bad idea.
>> >> >
>> >> > Aha. And probably integer overflow in the same line. Ouch.
>> >> >
>> >>
>> >> The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
>> >> on the architecture. Do you want a lower limit on the name length?
>> >
>> > 16K of unswappable kernel memory for name is a bit too much, yes.
>>
>> What if I just truncate it to NAME_MAX.
>
> My main concern with the ioctl() interface is that you're adding a device
> file just in order to have the ioctl()s available.  Usually, however, a device
> file's purpose is to implement file operations (open, close, read, write),
> while ioctl()s are used for doing things that the file operations are not
> suitable for.

In this case, we are adding a device file to get close (release).

> So, if your device only implements open(), close() and ioctl(), this is an
> indication that there's something wrong with the interface, because it doesn't
> do what one would generally expect it to do (ie. smells like an abuse).  That's
> why I think it would be better to use read() and write() instead of ioctl().

It not not clear to me how putting a control interface on top of read
and write is any less abusive.

> Of course, there also is a problem that people generally don't like adding new
> ioctl()s without a _really_ good reason.

ioctls are better suited for issuing commands than read/write.

--
Arve Hjønnevåg