serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Kernel: Remove hostname-related syscalls

Open supercomputer7 opened this issue 2 years ago • 9 comments

The suggestion is rather simple - we could use what Linux does and have a kernel file to expose the hostname and allow modifying it with the ordinary tools we have in a Unix-like OS - using open, read and write. Removing syscalls for operations that could just work with files is preferable - it will allow for example to our users to just do a simple print with cat /sys/kernel/variables/hostname which is quite nice in my opinion.

As for what the standards say, the gethostname is mandated by this POSIX page, but error values are not specified. We so far did what Linux did, and the patches in this PR try to stick to the same behavior. Looking at the manual page for gethostname (here), we can also see an interesting behavior of glibc:

The GNU C library does not employ the gethostname() system call; instead, it implements gethostname() as a library function that calls uname(2) and copies up to len bytes from the returned nodename field into name. Having performed the copy, the function then checks if the length of the nodename was greater than or equal to len, and if it is, then the function returns -1 with errno set to ENAMETOOLONG; in this case, a terminating null byte is not included in the returned name.

Clearly, having multiple interfaces to acquire the hostname (gethostname syscall and uname syscall) as the situation currently is, is simply not ideal. By changing this to have sysfs file which is writable to change the hostname, we could just drop the gethostname syscall just like how glibc doesn't use it on Linux.

The sethostname function is not mandated by POSIX. It seems like Linux just allows the users to simply do a write on a procfs node - /proc/sys/kernel/hostname. Since our ProcFS is strictly for process information, we use the /sys/kernel path for constants and variables related to kernel configuration.

Hopefully all the reasoning above convinces the maintainers that these patches are worth having merged into the tree :)

supercomputer7 avatar Jul 20 '23 14:07 supercomputer7

This consistently fails TestTLSHandshake, both on CI and locally for me. (But the test passes without this.)

 FAIL   LibTLS/TestTLSHandshake (2.117323s)
         Test:   TestTLSHandshake (failed)
Running test 'test_TLS_hello_handshake'.
FAIL: Tests/LibTLS/TestTLSHandshake.cpp:61: name server returned permanent failure

Otherwise this change makes sense to me.

AtkinsSJ avatar Aug 09 '23 12:08 AtkinsSJ

515.618 [#0 LookupServer(9:9)]: Rejecting path '/sys/kernel/variables/hostname' because it hasn't been unveiled
515.622 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a76a04  Kernel::VirtualFileSystem::validate_path_against_process_veil(Kernel::Process const&, AK::StringView, int) [clone .localalias] +0x684
515.641 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a79021  Kernel::VirtualFileSystem::validate_path_against_process_veil(Kernel::Process const&, Kernel::Custody const&, int) [clone .localalias] +0x391
515.641 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a806e5  Kernel::VirtualFileSystem::resolve_path(Kernel::Process const&, Kernel::Credentials const&, AK::StringView, AK::NonnullRefPtr<Kernel::Custody>, AK::RefPtr<Kernel::Custody>*, int, int) [clone .localalias] +0x265
515.645 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a9bebc  Kernel::VirtualFileSystem::open(Kernel::Process const&, Kernel::Credentials const&, AK::StringView, int, unsigned short, Kernel::Custody&, AK::Optional<Kernel::UidAndGid>) [clone .localalias] +0x18c
515.645 [#0 LookupServer(9:9)]: Kernel + 0x0000000000aa043d  Kernel::VirtualFileSystem::open(Kernel::Credentials const&, AK::StringView, int, unsigned short, Kernel::Custody&, AK::Optional<Kernel::UidAndGid>) +0x18d
515.649 [#0 LookupServer(9:9)]: Kernel + 0x0000000000fc7833  Kernel::Process::sys$open(AK::Userspace<Kernel::Syscall::SC_open_params const*>) +0xb03
515.649 [#0 LookupServer(9:9)]: Kernel + 0x00000000010f76e9  Kernel::Syscall::handle(Kernel::RegisterState&, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long) [clone .localalias] +0xa99
515.649 [#0 LookupServer(9:9)]: Kernel + 0x00000000010f9f9a  syscall_handler +0x94a
515.649 [#0 LookupServer(9:9)]: Kernel + 0x0000000001455461  syscall_entry +0x51
515.652 LookupServer(9:9): ASSERTION FAILED: gethostname(buffer, sizeof(buffer)) == 0
./Userland/Services/LookupServer/LookupServer.cpp:149
515.672 [#0 LookupServer(9:9)]: Terminating LookupServer(9) due to signal 6

Looks like we'll need to make sure to unveil the SysFS variable in processes that want to interact with hostnames.

Alternatively, I would be fine with making unprivileged, read-only access to SysFS variables exempt from the unveil mechanism; especially if there are more plans to replace dedicated syscalls with that.

BertalanD avatar Aug 09 '23 12:08 BertalanD

515.618 [#0 LookupServer(9:9)]: Rejecting path '/sys/kernel/variables/hostname' because it hasn't been unveiled
515.622 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a76a04  Kernel::VirtualFileSystem::validate_path_against_process_veil(Kernel::Process const&, AK::StringView, int) [clone .localalias] +0x684
515.641 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a79021  Kernel::VirtualFileSystem::validate_path_against_process_veil(Kernel::Process const&, Kernel::Custody const&, int) [clone .localalias] +0x391
515.641 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a806e5  Kernel::VirtualFileSystem::resolve_path(Kernel::Process const&, Kernel::Credentials const&, AK::StringView, AK::NonnullRefPtr<Kernel::Custody>, AK::RefPtr<Kernel::Custody>*, int, int) [clone .localalias] +0x265
515.645 [#0 LookupServer(9:9)]: Kernel + 0x0000000000a9bebc  Kernel::VirtualFileSystem::open(Kernel::Process const&, Kernel::Credentials const&, AK::StringView, int, unsigned short, Kernel::Custody&, AK::Optional<Kernel::UidAndGid>) [clone .localalias] +0x18c
515.645 [#0 LookupServer(9:9)]: Kernel + 0x0000000000aa043d  Kernel::VirtualFileSystem::open(Kernel::Credentials const&, AK::StringView, int, unsigned short, Kernel::Custody&, AK::Optional<Kernel::UidAndGid>) +0x18d
515.649 [#0 LookupServer(9:9)]: Kernel + 0x0000000000fc7833  Kernel::Process::sys$open(AK::Userspace<Kernel::Syscall::SC_open_params const*>) +0xb03
515.649 [#0 LookupServer(9:9)]: Kernel + 0x00000000010f76e9  Kernel::Syscall::handle(Kernel::RegisterState&, unsigned long, unsigned long, unsigned long, unsigned long, unsigned long) [clone .localalias] +0xa99
515.649 [#0 LookupServer(9:9)]: Kernel + 0x00000000010f9f9a  syscall_handler +0x94a
515.649 [#0 LookupServer(9:9)]: Kernel + 0x0000000001455461  syscall_entry +0x51
515.652 LookupServer(9:9): ASSERTION FAILED: gethostname(buffer, sizeof(buffer)) == 0
./Userland/Services/LookupServer/LookupServer.cpp:149
515.672 [#0 LookupServer(9:9)]: Terminating LookupServer(9) due to signal 6

Looks like we'll need to make sure to unveil the SysFS variable in processes that want to interact with hostnames.

This will be fixed soon :)

Alternatively, I would be fine with making unprivileged, read-only access to SysFS variables exempt from the unveil mechanism; especially if there are more plans to replace dedicated syscalls with that.

This is not possible without hardcoding the path in the kernel code. Also, it doesn't make sense imho, because:

  1. We are not going to deprecate more syscalls (that will need a SysFS file) as far as I can tell...
  2. We should allow only certain programs to access the file. There's no easy way to tell which program will need, besides just doing the tedious work of determining it manually :)

supercomputer7 avatar Aug 10 '23 19:08 supercomputer7

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Oct 04 '23 01:10 stale[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Oct 29 '23 12:10 stale[bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Nov 24 '23 20:11 stale[bot]

@ADKaster in regard to this comment in the discord server:

Yes, this is not performance critical-wise thing to do. But realistically, this was never an issue for any application, was it? no application actually requires to read and write the hostname so frequently. As far as I can tell, LookupServer could easily keep a file descriptor to the sysfs file so it could read it over and over again, just like how it happens with the original syscall. Other programs usually don't read or write so often to the hostname variable as they clearly have short-time lifetime.

Similarly to the remove of the beep syscall, the same principle applies here - the idea is to add a way to allow other users to change the hostname, even if they're not root. Now you might ask - why should I allow it? the answer is flexibility, and once again, also for Jails usage. I'd like to add a mechanism to remap some kernel knob inodes in the sysfs as non-root modifiable inside a Jail. Similarly to how Linux containers allow users to change the hostname, I think we can find useful use-cases for such functionality.

Regardless, I don't think we have this syscall for performance reasons in the first place, it probably was just the route due to what Andreas chose at the time, but this doesn't mean we have to keep 2 special syscalls that serve such a specific purpose when it can be done with a sysfs file.

supercomputer7 avatar Dec 16 '23 09:12 supercomputer7

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

stale[bot] avatar Jan 06 '24 17:01 stale[bot]

I will try to implement a mechanism for allowing bind-mounting in jails to remap permissions, so basically with little trickery here we could allow each process inside a Jail to change the hostname as it sees fit (without changing the original host hostname anyway). I think such functionality will justify removing these syscalls and moving them to a SysFS file.

supercomputer7 avatar Jan 20 '24 18:01 supercomputer7

#22968 will introduce a way to separate hostname contexts, so we will probably need this set of syscalls. Let's close this for now.

supercomputer7 avatar Feb 02 '24 15:02 supercomputer7