Created attachment 103806 [details] tty-ask-password-agent-on-console.patch The tool systemd-tty-ask-password-agent used with the option --console does only ask the password on the last console configured in the kernel command line. That is that if e.g. console=ttyS0,115200 console=tty0 is used the passphrase for a e.g. crypted device will only be asked on /dev/tty1 at boot. To fix this I've ported my old multi console device support from sulogin to the tty-ask-password-agent source. Maybe it is worthy enough to integrate and/or port this solution into systemd. That is that the two functions collect_consoles() and free_consoles() as well as the struct console could become be part of src/share/util.c below resolve_dev_console()
Looks reasonable. I have some requests for changes in the implementation though: 1. Please make the list of 'consoles' simply an array. Then GREEDY_REALLOC will be enough in a loop will be enough to initially allocate and add items to the array, making everything a bit simpler. Also, having the console name appended to the end of 'struct console' makes things more complicated than need be. Please just strndup() the string. 2. 'consoles' global variable does not have to be global, and 'current_dev' probably too. We usually only use global variables for commandline arguments. 3. The whole thing with 'usemask' is not going to work, because there's no locking. And I don't see the need for it. Isn't it enough to a) in a loop: set index, launch child b) wait until no children are left c) quit ? 4. In collect consoles the block which adds a new 'struct console' entry is repeated twice. Can you make this a separate function? 5. For consistency with other code: switch(fork) should be turned into an two if's (e.g. http://cgit.freedesktop.org/systemd/systemd/tree/src/core/execute.c#n797) 6. When log_oom() happens, just fail, do not continue. 7. Killing and waiting for child should (I think) use the same signal sequence as wait_for_terminate(). If something different is needed, please add a comment.
I am not convinced this is really a good idea. I mean, we don't start gettys on all configured kernel consoles either, no rescue shell, no emergency shell. On Linux, /dev/console is a weird thing. It's output is dirtsibuted to multiple ttys, but it's input is not merged (and that'd even be difficult becuase it would be unclear what $TERM would be right for such a case). Just adding support for this to the text password prompt is just a work-around for one specific case...
(In reply to comment #2) > I am not convinced this is really a good idea. I mean, we don't start gettys > on all configured kernel consoles either, no rescue shell, no emergency > shell. What does this mean? > On Linux, /dev/console is a weird thing. It's output is dirtsibuted to > multiple ttys, but it's input is not merged (and that'd even be difficult > becuase it would be unclear what $TERM would be right for such a case). Just > adding support for this to the text password prompt is just a work-around > for one specific case... Hmmm ... I'm familiar with the kernel's system console. This patch is the port from my old blogd and sulogin[1] changes to systemd due to a bug report of a enterprise customer and a very helpful beta tester. As this customer does not use plymouth (which indeed would be useless), the systemd-tty-ask-password-agent should in his opinion able to ask the passphrase on all devices of the system console. And I do agree with him and furthermore I guess that redhat has also customers with the similar setups where the order of the console configuration used with the kernel parameters should not restrict the prompt for a encrypted device to the last mentioned console device. As with Murphy's law the superuser is always in front of the wrong display. Also such a list of console devices would allow that systemd writes out its boot messages to all those console devices, together with a small ring buffer this could also be used to relieve the kernel's message ring buffer. I guess that many kernel developers would appreciate such an extension. [1] Now also part of current util-linux
Created attachment 116907 [details] Enable tty-ask-password-agent to ask on all devices of the system console
(In reply to Zbigniew Jedrzejewski-Szmek from comment #1) Found some time to work on that > Looks reasonable. > > I have some requests for changes in the implementation though: > > 1. Please make the list of 'consoles' simply an array. Then GREEDY_REALLOC > will be enough in a loop will be enough to initially allocate and add items > to the array, making everything a bit simpler. Done ... even if I prever double linked lists. > > Also, having the console name appended to the end of 'struct console' makes > things more complicated than need be. Please just strndup() the string. I'd like to be able to free the structure in whole, with the attached patch this should work as I've tested. > 2. 'consoles' global variable does not have to be global, and 'current_dev' > probably too. We usually only use global variables for commandline arguments. Now 'consoles' is local. Nevertheless 'current_dev' remains global to be able to use this parse_password() > 3. The whole thing with 'usemask' is not going to work, because there's no > locking. And I don't see the need for it. Isn't it enough to > a) in a loop: set index, launch child > b) wait until no children are left > c) quit > ? Hmmm ... the 'usemask' is a shared memory area used to allow the worker childs asking for passwords to communicate with the main process. For this the id is used as bit. This allows to continue if one of the consoles had been used for a password as afterwards the remaining asking processes will be terminated. The wait_for_terminate() does not help for this use case. > 4. In collect consoles the block which adds a new 'struct console' entry is > repeated twice. Can you make this a separate function? done > 5. For consistency with other code: switch(fork) should be turned into an > two if's (e.g. > http://cgit.freedesktop.org/systemd/systemd/tree/src/core/execute.c#n797) done > 6. When log_oom() happens, just fail, do not continue. done > > 7. Killing and waiting for child should (I think) use the same signal > sequence as wait_for_terminate(). If something different is needed, please > add a comment. see above for the description of the usage of 'usemask' ... if one process has handled a password the other asking workers should die, that is we do not want to wait for termination.
This was implemented recently in current git (6af621248f2255f9ce50b0bafdde475305dc4e57), by you personally actually. Thanks. Closing.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.