On Fri, Dec 13, 2019 at 12:42:03PM +0100, Alex Gladkov wrote: > From: Alexey Gladkov > > All privileged operations moved to the daemon. Commands to the server > are sent through the unix domain socket. The credentials which the sender > specifies are checked by the kernel. The hasher-priv no longer SUID. I'm going to suggest some English literacy/typo improvements in a separate email. > > For each user server creates a separate session process that executes > commands only from the user who created it. The session process ends Since that new process will still be privileged, why the additional fork and the new listening socket inode? Is the strive for less complex daemon source code the only driver for that, albeit a perfectly viable one? > after a certain period of inactivity. No problem with that, but IMO we still should be careful about resource leaks. > > Signed-off-by: Alexey Gladkov > --- > hasher-priv/.gitignore | 1 + > hasher-priv/DESIGN | 281 ++++++++++++++++---------- I'd like to see a formal description of the client-server protocol in e. g. hasher-priv/IPC, that shows the intended message exchanges and meanings. The message contents can be rather easily inferred from the C headers, but the semantics cannot. This ends up as a source of programming errors, see the other emails. The relevant information is probably already present in the DESIGN file, but, as this is documentation for humans, we should wrap it into a form that's easier to comprehend without wasting much time. I'm also worried about the message format using plain C ABI structs. We're not going to use it over the network on machines with different architectures, and we're never going to support a client and server from different package builds, sure. But is this enough of a justification? > hasher-priv/Makefile | 28 ++- > hasher-priv/caller.c | 81 ++++---- After reading the following 2 new files... > hasher-priv/caller_server.c | 373 ++++++++++++++++++++++++++++++++++ > hasher-priv/caller_task.c | 214 ++++++++++++++++++++ ..., I'm not sure why do they have to be separate. They can even be merged with caller.c, perhaps. > hasher-priv/cmdline.c | 27 ++- > hasher-priv/communication.c | 392 ++++++++++++++++++++++++++++++++++++ > hasher-priv/communication.h | 77 +++++++ > hasher-priv/config.c | 143 ++++++++++++- > hasher-priv/epoll.c | 39 ++++ > hasher-priv/epoll.h | 18 ++ > hasher-priv/hasher-priv.c | 78 +++++++ > hasher-priv/hasher-privd.c | 375 ++++++++++++++++++++++++++++++++++ > hasher-priv/io_log.c | 2 +- > hasher-priv/io_x11.c | 2 +- > hasher-priv/killuid.c | 2 +- > hasher-priv/logging.c | 64 ++++++ > hasher-priv/logging.h | 55 +++++ > hasher-priv/main.c | 75 ------- > hasher-priv/pass.c | 117 ++++++++++- > hasher-priv/pidfile.c | 128 ++++++++++++ > hasher-priv/pidfile.h | 44 ++++ > hasher-priv/priv.h | 33 +-- > hasher-priv/server.conf | 13 ++ > hasher-priv/sockets.c | 183 +++++++++++++++++ > hasher-priv/sockets.h | 32 +++ > hasher-priv/x11.c | 1 + > 28 files changed, 2632 insertions(+), 246 deletions(-) > create mode 100644 hasher-priv/caller_server.c > create mode 100644 hasher-priv/caller_task.c > create mode 100644 hasher-priv/communication.c > create mode 100644 hasher-priv/communication.h > create mode 100644 hasher-priv/epoll.c > create mode 100644 hasher-priv/epoll.h > create mode 100644 hasher-priv/hasher-priv.c > create mode 100644 hasher-priv/hasher-privd.c > create mode 100644 hasher-priv/logging.c > create mode 100644 hasher-priv/logging.h > delete mode 100644 hasher-priv/main.c > create mode 100644 hasher-priv/pidfile.c > create mode 100644 hasher-priv/pidfile.h > create mode 100644 hasher-priv/server.conf > create mode 100644 hasher-priv/sockets.c > create mode 100644 hasher-priv/sockets.h Wow, this patch is rather big. That would be easier to review if there was a bunch of logically self-contained commits, which could later be squashed by the merging maintainer, perhaps, if there's a requirement that every commit has to build successfully and run well. I'm going to post reviews in separate messages, one answer per file.