Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rt_sigprocmask syscall implementation #112

Merged
merged 8 commits into from
Dec 7, 2021
Merged

rt_sigprocmask syscall implementation #112

merged 8 commits into from
Dec 7, 2021

Conversation

Lurk
Copy link
Contributor

@Lurk Lurk commented Dec 5, 2021

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
Copy link
Owner

nuta commented Dec 6, 2021

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 😃

Copy link
Owner

@nuta nuta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you have a look at my review?

libs/kerla_utils/bitmap.rs Outdated Show resolved Hide resolved
libs/kerla_utils/bitmap.rs Outdated Show resolved Hide resolved
libs/kerla_utils/bitmap.rs Outdated Show resolved Hide resolved
libs/kerla_utils/bitmap.rs Outdated Show resolved Hide resolved
kernel/process/process.rs Outdated Show resolved Hide resolved
kernel/process/process.rs Outdated Show resolved Hide resolved
kernel/process/process.rs Outdated Show resolved Hide resolved
kernel/process/process.rs Outdated Show resolved Hide resolved
@Lurk
Copy link
Contributor Author

Lurk commented Dec 6, 2021

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.

Copy link
Owner

@nuta nuta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me! Just leave some nitpicks.

libs/kerla_utils/bitmap.rs Outdated Show resolved Hide resolved
kernel/process/process.rs Outdated Show resolved Hide resolved
@nuta
Copy link
Owner

nuta commented Dec 7, 2021

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.

Serhiy Barhamon and others added 3 commits December 7, 2021 12:38
Co-authored-by: Seiya Nuta <nuta@seiya.me>
Co-authored-by: Seiya Nuta <nuta@seiya.me>
@Lurk
Copy link
Contributor Author

Lurk commented Dec 7, 2021

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
Copy link
Owner

nuta commented Dec 7, 2021

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

@nuta nuta merged commit a336e48 into nuta:main Dec 7, 2021
@nuta
Copy link
Owner

nuta commented Dec 7, 2021

Thanks a lot, @Lurk!

@Lurk
Copy link
Contributor Author

Lurk commented Dec 7, 2021

@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?

nuta pushed a commit that referenced this pull request Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants