Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logind: add SetBrightness() bus call as minimal API for setting "leds" and "backlight" kernel class device brightness #12424

Merged
merged 14 commits into from Jun 12, 2019

Conversation

@poettering
Copy link
Member

commented Apr 28, 2019

See #12413 for the discussion this is a result of

@lgtm-com

This comment has been minimized.

Copy link

commented Apr 28, 2019

This pull request introduces 1 alert when merging 158b23c into d897475 - view on LGTM.com

new alerts:

  • 1 for Missing header guard

Comment posted by LGTM.com

@poettering poettering force-pushed the poettering:logind-brightness branch from 158b23c to e563971 Apr 28, 2019
@poettering

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

Over at #12399 @benzea mentioned these patches worked for him when he backported them

@poettering poettering added this to the v243 milestone Apr 29, 2019
Copy link
Member

left a comment

The part that adds SetBrightness looks great. But I'm not convinced about the session guessing magic, please see comment inline.

src/login/71-seat.rules.in Outdated Show resolved Hide resolved
src/login/loginctl.c Outdated Show resolved Hide resolved
src/login/logind-dbus.c Outdated Show resolved Hide resolved
static int get_sender_session(
Manager *m,
sd_bus_message *message,
bool try_harder,

This comment has been minimized.

Copy link
@keszybz

keszybz Apr 30, 2019

Member

I don't like this at all. I understand that it conveniently solves the problem at hand, but it seems like too much magic. In particular, before there was consistency that "" would operate the same way in all calls. Now the "query" operations (GetSession, GetSessionByPID(0)) do one thing, and "action" operations (Activate, Lock, Kill, Terminate) do another. And this is scriptable API and people might use this in a way we don't know about. ("ReleaseSession" is yet another story, but since it's supposed to be internal, it doesn't matter that much).

What about introducing a new magic value ("(auto)" ?) that would explicitly do what the proposed patch does implicitly for ""? Then we'd still have full backwards compatibility, and gnome can switch to this where appropriate. If we later figure out that we want some different magic, we can add another value that slightly different semantics, again without breaking backwards compatibility.

This comment has been minimized.

Copy link
@poettering

poettering Apr 30, 2019

Author Member

So I thought about this a while, and even had an earlier version of the PR that did something very close to what you suggest, but then I figured that the complexity this brings in is a bit too much, given that this is not as magic as it might appear at first, given that the "display" sesison is pretty well defined.

I'll have another look, maybe we can find another way.

src/login/logind-brightness.c Outdated Show resolved Hide resolved
src/login/logind-brightness.c Outdated Show resolved Hide resolved
src/login/logind-brightness.c Outdated Show resolved Hide resolved
src/login/logind-brightness.c Outdated Show resolved Hide resolved
@poettering poettering force-pushed the poettering:logind-brightness branch from e563971 to b12ac33 Apr 30, 2019
@poettering

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Force pushed a new version now. It introduces the magic seat and session names "self" and "auto". The former is like the empty string, the latter introduces the new behaviour that looks for the display session if the caller is not part of a sesssion.

Why "self"? we already had a magic object path where the client session/seat was reachable via "self", this just extends the logic to also take the "self" string everywhere else.

Of course, this only works because real seats nor real sessions can be named "self" nor "auto". That's because we enforce that seat names start with "seat", and session names are generated by us, and either are a number or a number prefixed with "c". Hence we should be good.

I added some minor clean-up stuff on top. PTAL

@benzea

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Hm, just to make sure I get this right. This does mean together with systemd v243 and migrating to using the systemd user instance we need to change the parameter from an empty string to "auto" (or explicitly look up the ID).

@poettering

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Hm, just to make sure I get this right. This does mean together with systemd v243 and migrating to using the systemd user instance we need to change the parameter from an empty string to "auto" (or explicitly look up the ID).

yes, correct.

@boucman

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

would a more generic SetAttribute/GetAttribute (properly guarded by udev attributes) be a more long-term solution ? we don't know what other device will need mediation... backlights/leds brightness might be the tip of the iceberg, and the mechanism to mediate device-nodes is already a generic one, a generic one for sysfs could make sense...

@poettering

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@boucman the input and drm subsystems in the kernel are explicitly designed so that they can be passed to unprivleged clients safely, and that logind can arbitrate between clients in a fully safe way. i.e. because the kernel layer has a concept of "temporary ownership" of devices, and the folks doing those layers have understood the need for arbitraion in userspace we can safely implement session switching, taking off clients when their session is in the background and putting them back on when it comes back to the foreground. The backlights/leds subsystems don't have any of that, they are very naively written, without any concept of ownership, privileges or arbitration of access. Which is why this PR turns logind into a proxy in thise case, opening up only the most basic of operations, in a safe and focussed way. i.e. because the kernel won't do it for us we have to open this up safely on our own. But that's only something we can safely do when we actually understand the operation to proxy. We need a clear understanding what an operation entails if we want to open it up on our own. Hence I am very sure we should be conservative here: only expose what we understand and what real-life clients need. And if there's more to add later on, then that's great but we really need to think about it first then, and only open it up when we decided its safe to. Hence I think we should stick with SetBrightness() for now, and only when some other operation comes up and people can make a good case for add more, under new method calls.

Or to say this differently: when passing back an fd to a drm or input device, we know that the kernel will allow unpriv clients only do safe things, and when we tell the kernel to will freeze access to the devices (which we use during session switching). If we on the other hand give clients an fd of some form to make arbitrary changes to brightness devices, then it could do whatever it wants with that, because the kernel leds/backlight subsystems don't care about unpriv/priv clients, and there are no ioctls for freezing access during session switches.

@boucman

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@poettering Thx a lot, that was the kind of explanation I needed

@emersion

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I'd just like to point out that there are a few issues with this approach:

  1. The sysfs interface is ill-defined. Drivers do that they want, zero sometimes mean "black screen" and sometimes mean "minimum value with the screen still on", the paths are driver-specific, and so on.
  2. This should really be a per-connector knob. A DRM connector property would make a lot more sense.

There have been attempts in the past to fix this in the kernel side (e.g. https://lists.freedesktop.org/archives/dri-devel/2014-September/067988.html, https://lists.freedesktop.org/archives/dri-devel/2016-October/121872.html), but there were still some issues (we need to agree on a standard, the kernel code for backlight is a mess). I was planning to revive the efforts to properly fix this.

@benzea

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@emersion, I believe that this is exactly why the interface is that simple. All it does is punch a hole to write the (broken) kernel interface, it specifically does not try to abstract away the mentioned issues.

@emersion

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

I think we shouldn't encourage compositors to use the broken interface by exposing it like this.

In other words, adding this interface isn't a proper fix. I'd rather make compositors wait a little bit more and do the right thing.

@benzea

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@emersion, well, everyone is already using these interfaces (though I don't think anyone is doing so inside the compositor/display server, it is generally done separately). Really, all we are doing here is creating a common way to do access control on these interfaces.

@emersion

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Do we really want to expose a new interface, knowing it is broken by design?

What happens when we get a proper DRM API?

@ascent12

This comment has been minimized.

Copy link

commented May 3, 2019

A DRM property would definitely be the best solution to this, if it can be done. It removes the need for each display server to have a bunch of dodgy heuristics and hopefully removes the need to write any driver specific code.

Doing this through logind doesn't really provide a lot of value; logind has no way to actually enforce this like it can with DRM masters and EVIOCREVOKE, except for compositors that are playing along. It replaces the need for us to suid part, but not much else.

Also, any display server which doesn't have a hard dependency on systemd/logind (which is all of them except Gnome AFAIK) and going to need to implement a suid helper anyway, so it doesn't even really save us from that. This may not be the most compelling point to systemd developers, though.

To me it's "keep using old code to do it the old way" (suid helper) vs "write new code to do it the old way" (logind) vs "write new code to do it the new way" (DRM property). There isn't a compelling reason to use the middle option, unless a DRM backlight API doesn't ever end up panning out.

@boucman

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@ascent12 note that most brightness tool don't use a suid helper, but use udev rules to add the video group to brightness devices...

not sure if it's more or less ugly than a suid heper... that's a matter of taste I guess

@CameronNemo

This comment has been minimized.

Copy link

commented May 3, 2019

Whether it is suid or sgid to something like _brightness still lacks the capability of tracking ownership via sessions. My tool uses polkit for that, but even then it is not aware of seats.

The point above about hard dependencies on dbus is quite real. Most tools are either going to end up being very simple wrappers around logind's dbus API (e.g. a shell script around busctl...), or insist on accessing the sysfs API directly.

Hopefully there is some movement on the kernel side. They mention a full backlight character device is overkill but maybe that would be an easier implementation than linking the backlight devices to DRM devices. Would certainly be better than the status quo.

@poettering

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

I'd just like to point out that there are a few issues with this approach:

1. The sysfs interface is ill-defined. Drivers do that they want, zero sometimes mean "black screen" and sometimes mean "minimum value with the screen still on", the paths are driver-specific, and so on.

2. This should really be a per-connector knob. A DRM connector property would make a lot more sense.

There have been attempts in the past to fix this in the kernel side (e.g. https://lists.freedesktop.org/archives/dri-devel/2014-September/067988.html, https://lists.freedesktop.org/archives/dri-devel/2016-October/121872.html), but there were still some issues (we need to agree on a standard, the kernel code for backlight is a mess). I was planning to revive the efforts to properly fix this.

I remember a similar discussion a few years back, but nothing much happened in this regard. I think if backlight control moves to DRM that'd be more than excellent, but I am a bit conservative on hoping it happens next week. ;-)

However, I would see that as reason to not do a SetBrightness() call like this either. I mean, the backlight subsystem is unlikely to go away, and this API actually covers the leds subsystem too, which I figure is not going to be subsumed into DRM, is it? i.e. supporting SetBrightness() doesn't mean that it wasn't better solved in DRM, and I really hope that happens eventually, but I also think there's no fault in exposing any backlights or leds devices associated with a seat to the one owning the active session on the seat, given that these devices classes are not going away...

@poettering

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

And yeah, the sysfs iface and the fact that brightness 0 sometimes means off and sometimes doesn't sucks. iirc gnome simply never uses the lower 10% of the brightness range anyway, just to be sure... But either way, I have no intention to fix that, and I am not sure it's worth avoiding the iface because of that altogether..

@benzea

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

iirc gnome simply never uses the lower 10% of the brightness range anyway

We are limiting to 1% to be exact, the logic is MAX (1, brightness_max * 0.01) with a few more special cases. But yeah, we do try to prevent the "off" case with the reasoning that DPMS should be used instead.

@poettering poettering force-pushed the poettering:logind-brightness branch from b12ac33 to 6d55d35 May 10, 2019
gnomesysadmins pushed a commit to GNOME/mutter that referenced this pull request May 10, 2019
If mutter is running as a systemd user service, then we cannot use the
magic "self" session for the ID lookup. For now we need to lookup the ID
explicitly. Eventually we can change to use the magic "auto" paths for
both the session and seat, but that will require systemd v243.

See also systemd/systemd#12424 (comment)

https://gitlab.gnome.org/GNOME/mutter/merge_requests/571
gnomesysadmins pushed a commit to GNOME/mutter that referenced this pull request May 10, 2019
If mutter is running as a systemd user service, then we cannot use the
magic "self" session for the ID lookup. For now we need to lookup the ID
explicitly. Eventually we can change to use the magic "auto" paths for
both the session and seat, but that will require systemd v243.

See also systemd/systemd#12424 (comment)

https://gitlab.gnome.org/GNOME/mutter/merge_requests/571
gnomesysadmins pushed a commit to GNOME/mutter that referenced this pull request May 21, 2019
If mutter is running as a systemd user service, then we cannot use the
magic "self" session for the ID lookup. For now we need to lookup the ID
explicitly. Eventually we can change to use the magic "auto" paths for
both the session and seat, but that will require systemd v243.

See also systemd/systemd#12424 (comment)

https://gitlab.gnome.org/GNOME/mutter/merge_requests/571
poettering added 14 commits Apr 28, 2019
These devices do not become user-accessible this way, but they are
logically assigned to a seat, which makes a lot of sense, since they are
human-facing output devices, and such should belong to one.
…acklight devices associated with a seat

This augments the drm/input device management by adding a single method
call for setting the brightness of an "leds" or "backlight" kernel class
device.

This method call requires no privileges to call, but a caller can only
change the brightness on sessions that are currently active, and they
must own the session.

This does not do enumeration of such class devices, feature or range
probing, chnage notification; it doesn't help associating graphics or
input devices with their backlight or leds devices. For all that clients
should go directly to udev/sysfs. The SetBrightness() call is just for
executing the actual change operation, that is otherwise privileged.

Example line:

   busctl call org.freedesktop.login1 /org/freedesktop/login1/session/self org.freedesktop.login1.Session SetBrightness ssu "backlight" "intel_backlight" 200

The parameter the SetBrightness() call takes are the kernel subsystem
(i.e. "leds" or "backlight"), the device name, and the brightness
value.

On some hw setting the brightness is slow, and implementation and write
access to the sysfs knobs exposes this slowness. Due to this we'll fork
off a writer process in the background so that logind doesn't have to
block. Moreover, write requestes are coalesced: when a write request is
enqueued while one is already being executed it is queued. When another
write reques is then enqueued the earlier one is replaced by the newer
one, so that only one queued write request per device remains at any
time. Method replies are sent as soon as the first write request that
happens after the request was received is completed.

It is recommended that bus clients turn off the "expect_reply" flag on
the dbus messages they send though, that relieves logind from sending
completion notification and is particularly a good idea if clients
implement reactive UI sliders that send a quick secession of write
requests.

Replaces: #12413
… a user

Interestingly, elect_display_compare() already ordered "user" sessions
before "greeter" sessions, though nothing other than "user" sessions
where ever considered anyway.

Fixes: #12399
…+ sessions

Most of the operations one can do on sessions so far accepted an empty
session name as a shortcut for the caller's session. This is quite
useful traditionally, but much less useful than it used to be, since
most user code now (rightfully) runs in --user context, not in a
session.

With this change we tweak the logic a bit: we introduce the two special
session and seat names "self" and "auto". The former refers to the
session/seat the client is in, and is hence mostly equivalent to te
empty string "" as before. However, the latter refers to the
session/seat the client is in if that exists, with a fallback of the
user's display session if not. Clients can hence reference "auto"
instead of the empty string if they really don't want to think much
about sessions.

Why "self" btw? Previously, we'd already expose a special dbus object
with the path /org/freedesktop/login1/session/self (and similar for the
seat), matching what the empty string did for bus calls that took a
session name. With this scheme we reuse this identifier and introduce
"auto" in a similar way.

Of course this means real-life seats and sessions can never be named
"self" or "auto", but they aren't anyway: valid seat names have to start
with "seat" anyway, and sessions are generated server-side as either a
numeric value or "c" suffixed with a counter ID.

Fixes: #12399
The server side can do something similar, but better on its own, let's
hence rely on that.
Previously, logind's logind-session.h would define prototypes for
logind-session.c and logind-session-dbus.c. Split that out, so that
there's a separate logind-session-dbus.h for that. Similar for seats and
users as well as the manager itself.

This changes no code, just rearranges where protoypes are located.
@poettering poettering force-pushed the poettering:logind-brightness branch from 6d55d35 to 6ecda0f May 24, 2019
@keszybz

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

LGTM.

@keszybz keszybz merged commit 58cf79c into systemd:master Jun 12, 2019
10 of 12 checks passed
10 of 12 checks passed
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
CentOS CI (Arch in KVM with sanitizers) Build finished.
Details
CentOS CI (Arch in KVM) Build finished.
Details
CentOS CI (CentOS 7) Build finished.
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
bionic-s390x autopkgtest finished (success)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
systemd.systemd Build #20190524.24 succeeded
Details
systemd.systemd (ASan_UBSan) ASan_UBSan succeeded
Details
systemd.systemd (FuzzBuzz) FuzzBuzz succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.