Re: [PATCH] epoll: Add a flag, EPOLLWAKEUP, to prevent suspend while epoll events are ready

[Rafael J. Wysocki]

On Tuesday, July 17, 2012, Michael Kerrisk wrote:
> On Tue, Jul 17, 2012 at 12:04 AM, Arve Hjønnevåg <arve@xxxxxxxxxxx> wrote:
> > On Mon, Jul 16, 2012 at 4:00 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> >> On Monday, July 16, 2012, Michael Kerrisk wrote:
> >>> Arve, Rafael,
> >>>
> >>> On Tue, May 1, 2012 at 7:33 AM, Arve Hjønnevåg <arve@xxxxxxxxxxx> wrote:
> >>> > When an epoll_event, that has the EPOLLWAKEUP flag set, is ready, a
> >>> > wakeup_source will be active to prevent suspend. This can be used to
> >>> > handle wakeup events from a driver that support poll, e.g. input, if
> >>> > that driver wakes up the waitqueue passed to epoll before allowing
> >>> > suspend.
> >>>
> >>> It's late it the -rc series,
> >>
> >> Well, exactly. :-)
> 
> If someone had CCed linux-api@ along the way (as per
> Documentation/SubmitChecklist), it might have helped ;-)

Well, it still _is_ late.

> >>> but it strikes me that CAP_EPOLLWAKEUP is
> >>> a poor name for the capability that governs the use of EPOLLWAKEUP.
> >>> While on the one hand some capabilities are overloaded
> >>> (https://lwn.net/Articles/486306/), on the other hand we should avoid
> >>> adding individual capabilities for each new API feature (otherwise
> >>> capabilities become administratively unwieldy).
> >>>
> >>> This capability is not really about "EPOLL". It's about the ability to
> >>> block system suspend. Therefore, IMO, a better name would be something
> >>> like: CAP_BLOCK_SUSPEND. This name is better because there might be
> >>> some other API feature that is later added that also has the effect of
> >>> preventing system suspends, and we could reasonably govern that
> >>> feature with the same capability.
> >
> > We already have another api, "/sys/power/wake_lock", that allow
> > user-space to block suspend. Do we want to apply this capability that
> > api as well, or only to apis that do not have other ways to restrict
> > access?
> 
> Well, the question is: is there a governor on the use of
> /sys/power/wake_lock? It makes sense either they are both governed
> (preferably by the same mechanism, I would have thought), or neither
> is.
> 
> >>> Does that seem sensible to you? I can send a patch for the name change.
> >>
> >> I'm not sure what Arve thinks about that, but I'd be fine with that.
> >>
> >> Arve, what do you think?
> >>
> >
> > CAP_BLOCK_SUSPEND is fine with me, but if it does not apply to the
> > sysfs interface, then the comment should probably mention this.
> 
> I've sent a patch, but omitted mention of API details in the comments.
> Maybe that can be changed afterward, when a decision has been reached
> about governing /sys/power/wake_lock.

I'm going to push your patch for v3.5, but then I'm considering the following
one for v3.6.  I wouldn't like to make more changes in v3.5-rc at this point,
if possible.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@xxxxxxx>
Subject: PM / Sleep: Require CAP_BLOCK_SUSPEND to use wake_lock/wake_unlock

Require processes wanting to use the wake_lock/wake_unlock sysfs
files to have the CAP_BLOCK_SUSPEND capability, which also is
required for the eventpoll EPOLLWAKEUP flag to be effective, so that
all interfaces related to blocking autosleep depend on the same
capability.

Signed-off-by: Rafael J. Wysocki <rjw@xxxxxxx>
---
 kernel/power/wakelock.c |    7 +++++++
 1 file changed, 7 insertions(+)

Index: linux/kernel/power/wakelock.c
===================================================================
--- linux.orig/kernel/power/wakelock.c
+++ linux/kernel/power/wakelock.c
@@ -9,6 +9,7 @@
  * manipulate wakelocks on Android.
  */
 
+#include <linux/capability.h>
 #include <linux/ctype.h>
 #include <linux/device.h>
 #include <linux/err.h>
@@ -188,6 +189,9 @@ int pm_wake_lock(const char *buf)
 	size_t len;
 	int ret = 0;
 
+	if (!capable(CAP_BLOCK_SUSPEND))
+		return -EPERM;
+
 	while (*str && !isspace(*str))
 		str++;
 
@@ -231,6 +235,9 @@ int pm_wake_unlock(const char *buf)
 	size_t len;
 	int ret = 0;
 
+	if (!capable(CAP_BLOCK_SUSPEND))
+		return -EPERM;
+
 	len = strlen(buf);
 	if (!len)
 		return -EINVAL;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/