Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote:
In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
needed for things to work. However, we shouldn't reset it to root on
session_restore_vt() since it could have in fact already been set to
the user.
I don't follow here, can't parse this. Could you please elaborate?
---
src/login/logind-session.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index fdeacb1..905e73f 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -1070,8 +1070,6 @@ void session_restore_vt(Session *s) {
mode.mode = VT_AUTO;
ioctl(vt, VT_SETMODE, mode);
-fchown(vt, 0, -1);
-
s-vtfd = safe_close(s-vtfd);
}
Lennart
--
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
On 08/11/14 16:25, Lennart Poettering wrote:
On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote:
In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
needed for things to work. However, we shouldn't reset it to root on
session_restore_vt() since it could have in fact already been set to
the user.
I don't follow here, can't parse this. Could you please elaborate?
I meant, before the call to session_prepare_vt() the owner of /dev/ttyX
might not be root, but already set to the user. In which case setting it
back to root might not be expected/best.
E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user,
then if a process try a TakeControl() and it failed (or after it's done)
the ownership would be set to root, even though it wasn't actually root
to begin with.
---
src/login/logind-session.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index fdeacb1..905e73f 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -1070,8 +1070,6 @@ void session_restore_vt(Session *s) {
mode.mode = VT_AUTO;
ioctl(vt, VT_SETMODE, mode);
-fchown(vt, 0, -1);
-
s-vtfd = safe_close(s-vtfd);
}
Lennart
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
On Mon, 11.08.14 16:39, Olivier Brunel (j...@jjacky.com) wrote: On 08/11/14 16:25, Lennart Poettering wrote: On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote: In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is needed for things to work. However, we shouldn't reset it to root on session_restore_vt() since it could have in fact already been set to the user. I don't follow here, can't parse this. Could you please elaborate? I meant, before the call to session_prepare_vt() the owner of /dev/ttyX might not be root, but already set to the user. In which case setting it back to root might not be expected/best. But that sounds more as if session_restore_vt() should not be used as-is as cleanup path for session_prepare_vt(), no? E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user, then if a process try a TakeControl() and it failed (or after it's done) the ownership would be set to root, even though it wasn't actually root to begin with. Isn't this very theoretic? I mean, when does TakeControl() actually really fail for you IRL? Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
On 08/11/14 16:54, Lennart Poettering wrote: On Mon, 11.08.14 16:39, Olivier Brunel (j...@jjacky.com) wrote: On 08/11/14 16:25, Lennart Poettering wrote: On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote: In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is needed for things to work. However, we shouldn't reset it to root on session_restore_vt() since it could have in fact already been set to the user. I don't follow here, can't parse this. Could you please elaborate? I meant, before the call to session_prepare_vt() the owner of /dev/ttyX might not be root, but already set to the user. In which case setting it back to root might not be expected/best. But that sounds more as if session_restore_vt() should not be used as-is as cleanup path for session_prepare_vt(), no? E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user, then if a process try a TakeControl() and it failed (or after it's done) the ownership would be set to root, even though it wasn't actually root to begin with. Isn't this very theoretic? I mean, when does TakeControl() actually really fail for you IRL? Right, this was noticed when trying to start a second rootless X, since with systemd-215 things currently fail, and the ownership of /dev/ttyX would then be switched/changed to root. session_prepare_vt() could also check the owner first, and note what to reset to on session_restore_vt(), to effectively restore things as they were. Lennart ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
Hi On Mon, Aug 11, 2014 at 5:05 PM, Olivier Brunel j...@jjacky.com wrote: On 08/11/14 16:54, Lennart Poettering wrote: On Mon, 11.08.14 16:39, Olivier Brunel (j...@jjacky.com) wrote: On 08/11/14 16:25, Lennart Poettering wrote: On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote: In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is needed for things to work. However, we shouldn't reset it to root on session_restore_vt() since it could have in fact already been set to the user. I don't follow here, can't parse this. Could you please elaborate? I meant, before the call to session_prepare_vt() the owner of /dev/ttyX might not be root, but already set to the user. In which case setting it back to root might not be expected/best. But that sounds more as if session_restore_vt() should not be used as-is as cleanup path for session_prepare_vt(), no? E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user, then if a process try a TakeControl() and it failed (or after it's done) the ownership would be set to root, even though it wasn't actually root to begin with. Isn't this very theoretic? I mean, when does TakeControl() actually really fail for you IRL? Right, this was noticed when trying to start a second rootless X, since with systemd-215 things currently fail, and the ownership of /dev/ttyX would then be switched/changed to root. Wait, what? Can you please elaborate. Currently, only one process can be controller at a time, and session_prepare_vt() is called *after* the controller is set. Therefore, it is not called when you try starting a second controller. The only race I see is this: * start legacy Xserver (which doesn't use TakeControl()) which sets user-id on the TTY * start new Xserver which uses TakeControl() (and thus calls session_prepare_vt()) * stop new Xserver (thus drop Control and reset the TTY) In this case, the new xserver will try to start up, but fail as the old one is still running. Therefore, it *might* call ReleaseControl() and thus reset the TTY. However, you're not supposed to mix both and this is not a legitimate use-case. I mean, the old server is root and modifies the TTY by itself (without using systemd). Obviously, this is racy. From a systemd-logind perspective, this is the same as if you run sudo chown xyz /dev/tty manually. session_prepare_vt() could also check the owner first, and note what to reset to on session_restore_vt(), to effectively restore things as they were. Even though I don't see any problem here, I'd be fine with such a patch. Care to send one? Note that you probably need to store that information in the session-file (session_save() / session_load()) too. Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
On 08/11/14 17:12, David Herrmann wrote: Hi On Mon, Aug 11, 2014 at 5:05 PM, Olivier Brunel j...@jjacky.com wrote: On 08/11/14 16:54, Lennart Poettering wrote: On Mon, 11.08.14 16:39, Olivier Brunel (j...@jjacky.com) wrote: On 08/11/14 16:25, Lennart Poettering wrote: On Fri, 08.08.14 20:45, Olivier Brunel (j...@jjacky.com) wrote: In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is needed for things to work. However, we shouldn't reset it to root on session_restore_vt() since it could have in fact already been set to the user. I don't follow here, can't parse this. Could you please elaborate? I meant, before the call to session_prepare_vt() the owner of /dev/ttyX might not be root, but already set to the user. In which case setting it back to root might not be expected/best. But that sounds more as if session_restore_vt() should not be used as-is as cleanup path for session_prepare_vt(), no? E.g. if a log in on tty3 right now, /dev/tty3 will be owned by my user, then if a process try a TakeControl() and it failed (or after it's done) the ownership would be set to root, even though it wasn't actually root to begin with. Isn't this very theoretic? I mean, when does TakeControl() actually really fail for you IRL? Right, this was noticed when trying to start a second rootless X, since with systemd-215 things currently fail, and the ownership of /dev/ttyX would then be switched/changed to root. Wait, what? Can you please elaborate. Currently, only one process can Sorry, I meant e.g. having one rootless X on tt1 and starting another one on tty2. Currently this fails (see other patch/mail), and is how this was observed. be controller at a time, and session_prepare_vt() is called *after* the controller is set. Therefore, it is not called when you try starting a second controller. The only race I see is this: * start legacy Xserver (which doesn't use TakeControl()) which sets user-id on the TTY * start new Xserver which uses TakeControl() (and thus calls session_prepare_vt()) * stop new Xserver (thus drop Control and reset the TTY) In this case, the new xserver will try to start up, but fail as the old one is still running. Therefore, it *might* call ReleaseControl() and thus reset the TTY. However, you're not supposed to mix both and this is not a legitimate use-case. I mean, the old server is root and modifies the TTY by itself (without using systemd). Obviously, this is racy. From a systemd-logind perspective, this is the same as if you run sudo chown xyz /dev/tty manually. session_prepare_vt() could also check the owner first, and note what to reset to on session_restore_vt(), to effectively restore things as they were. Even though I don't see any problem here, I'd be fine with such a patch. Care to send one? Note that you probably need to store that information in the session-file (session_save() / session_load()) too. If you think the current behavior (setting to root on session_restore_vt()) is fine, I'm fine leaving it as is. Else, I could work on a patch as described. Thanks David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
Hi On Mon, Aug 11, 2014 at 5:37 PM, Olivier Brunel j...@jjacky.com wrote: On 08/11/14 17:12, David Herrmann wrote: Wait, what? Can you please elaborate. Currently, only one process can Sorry, I meant e.g. having one rootless X on tt1 and starting another one on tty2. Currently this fails (see other patch/mail), and is how this was observed. Ah, so it's triggered by the same SIG-handler bug. be controller at a time, and session_prepare_vt() is called *after* the controller is set. Therefore, it is not called when you try starting a second controller. The only race I see is this: * start legacy Xserver (which doesn't use TakeControl()) which sets user-id on the TTY * start new Xserver which uses TakeControl() (and thus calls session_prepare_vt()) * stop new Xserver (thus drop Control and reset the TTY) In this case, the new xserver will try to start up, but fail as the old one is still running. Therefore, it *might* call ReleaseControl() and thus reset the TTY. However, you're not supposed to mix both and this is not a legitimate use-case. I mean, the old server is root and modifies the TTY by itself (without using systemd). Obviously, this is racy. From a systemd-logind perspective, this is the same as if you run sudo chown xyz /dev/tty manually. session_prepare_vt() could also check the owner first, and note what to reset to on session_restore_vt(), to effectively restore things as they were. Even though I don't see any problem here, I'd be fine with such a patch. Care to send one? Note that you probably need to store that information in the session-file (session_save() / session_load()) too. If you think the current behavior (setting to root on session_restore_vt()) is fine, I'm fine leaving it as is. Else, I could work on a patch as described. I just wanted to understand why that happens. And indeed, by fixing the SIG-handler bug, this is no longer necessary. I added log_error() statements to prepare_vt() so the log should be verbose enough. If you want to fix the fchown() thing, feel free to provide a patch. But I doubt anyone can trigger it with my fix applied. Thanks a lot! David ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
[systemd-devel] [PATCH 1/3] logind: session: don't set /dev/ttyX owner to root on restore_vt
In session_prepare_vt() we set owner of /dev/ttyX to the user, as that is
needed for things to work. However, we shouldn't reset it to root on
session_restore_vt() since it could have in fact already been set to the user.
---
src/login/logind-session.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index fdeacb1..905e73f 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -1070,8 +1070,6 @@ void session_restore_vt(Session *s) {
mode.mode = VT_AUTO;
ioctl(vt, VT_SETMODE, mode);
-fchown(vt, 0, -1);
-
s-vtfd = safe_close(s-vtfd);
}
--
2.0.4
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel
