nuta / kerla

A new operating system kernel with Linux binary compatibility written in Rust.
Other
3.33k stars 89 forks source link

rt_sigprocmask syscall implementation #112

Closed Lurk closed 2 years ago

Lurk commented 2 years ago

This pull request adds rt_sigprocmask syscall handler (https://man7.org/linux/man-pages/man2/sigprocmask.2.html).

Before adding signal filtering, I want to discuss the general approach.

Things that bother me:

I feel like the how argument should be an enum, but how to handle EINVAL in that case?

And the length argument doesn't make any sense to me. According to the manual, it is sizeof(kernel_sigset_t), and according to kernel sources, kernel_sigset_t is 1024 bits. So it always, according to POSIX, should be 128 bytes, unless we will run on something exotic like PDP-6/10 or DSP. If you can point me to some document or discussion to make sense of it, I would be grateful.

Also set and oldset are 1024 bits in length. I am using a slightly modified BitMap from kerla_utils, but it is suboptimal. It is [u8;128], which means we have 128 iterations on SIG_BLOCK and SIG_UNBLOCK. The solution can be generic over the type BitMap. Or create a new LongBitMap, which will use u128 under the hood. Or, maybe we can use or take inspiration from something like https://docs.rs/bitmaps/latest/bitmaps/. But I have a feeling that this is a task by itself, and should be discussed in a separate issue/PR.

nuta commented 2 years ago

Excellent! This is one of major missing features in Kerla 🎉

I feel like the how argument should be an enum, but how to handle EINVAL in that case?

How about using match in sys_rt_sigprocmask to do that, like you did in Process::rt_sigprocmask? The number of possible how looks small enough to me.

let how = match how {
    0 => SignalMask::Block,
     _ => return Err(Errno::EINVAL.into());
};

current_process().rt_sigprocmask(how, set, oldset, length)

And the length argument doesn't make any sense to me. According to the manual, it is sizeof(kernel_sigset_t), and according to kernel sources, kernel_sigset_t is 1024 bits. So it always, according to POSIX, should be 128 bytes, unless we will run on something exotic like PDP-6/10 or DSP. If you can point me to some document or discussion to make sense of it, I would be grateful.

I agree we can go with 1024 bits. Let's add debug_warn! if the length is not 1024 just in case.

Regarding the bitmap stuff, I think there're some possible optimizations too. The crate you mentioned looks neat and perhaps we'll replace our bitmap implementation with it. Let's discuss this topic in a separate issue later 😃

Lurk commented 2 years ago

I agree we can go with 1024 bits. Let's add debug_warn! if the length is not 1024 just in case.

I just checked, every time rt_sigprocmask is called during our integration tests, the length argument equals 8. I am lost :) It is like long long int in C is 128 bit, but google says it is 64 bits. Anyways, should I debug_warn if length is not 8?

And I added actual signal filtering in the last commit. Please take a look.

nuta commented 2 years ago

I just checked, every time rt_sigprocmask is called during our integration tests, the length argument equals 8. I am lost :) It is like long long int in C is 128 bit, but google says it is 64 bits. Anyways, should I debug_warn if length is not 8?

It seems I was confused with the difference between sigset_t and kernel_sigset_t. sigset_t is 1024 bits-sized type but apparently kernel_sigset_t is 8 bytes at lease in x64.

Please check if the length is 8 instead of 1024 bits in the system call handler.

Lurk commented 2 years ago

Please check if the length is 8 instead of 1024 bits in the system call handler.

Done

I will create a ticket for bitmap stuff later.

And maybe I should update https://github.com/nuta/kerla/blob/main/Documentation/compatibility.md ?

nuta commented 2 years ago

I'll update Documentation/compatibility.md by myself. Let me merge this PR as it is.

nuta commented 2 years ago

Thanks a lot, @Lurk!

Lurk commented 2 years ago

@nuta somehow I missed Clippy warning :(

error: redundant pattern matching, consider using `is_err()`
  --> kernel/syscalls/rt_sigprocmask.rs:27:16
   |
27 |         if let Err(_) = current_process().set_signal_mask(how, set, oldset, length) {
   |         -------^^^^^^-------------------------------------------------------------- help: try this: `if current_process().set_signal_mask(how, set, oldset, length).is_err()`
   |
   = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings`

Should I open a new pull request?