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

linux.ppoll always returns E_INVAL due to invalid sigsetsize #3493

Open
Feoramund opened this issue Apr 27, 2024 · 5 comments
Open

linux.ppoll always returns E_INVAL due to invalid sigsetsize #3493

Feoramund opened this issue Apr 27, 2024 · 5 comments

Comments

@Feoramund
Copy link
Contributor

Context

It seems the Linux kernel is expecting a specific value for sigsetsize (which glibc handles), despite what the man pages say about this syscall. In Odin, we pass size_of(Sig_Set), which is 128 bytes, compared to the expected 8 bytes (on a 64-bit system, at least).

   C library/kernel differences

       [...]

       The raw ppoll() system call has a  fifth  argument,  size_t  sigsetsize,
       which  specifies  the  size in bytes of the sigmask argument.  The glibc
       ppoll() wrapper function specifies this argument as a fixed value (equal
       to sizeof(kernel_sigset_t)).  See sigprocmask(2) for a discussion on the
       differences between the kernel and the libc notion of the sigset.

There's an interesting report over on the zig project related to this issue, including a link to the Linux kernel source where this is checked. ziglang/zig#12715

Using size_of(c.long) may fix it, but then I wonder what to do about the Sig_Set structure as far as the Odin user-facing API is concerned.

Odin:    dev-2024-04:8fd318ea7
OS:      Arch Linux, Linux 6.8.7-arch1-1
CPU:     12th Gen Intel(R) Core(TM) i7-12700K
RAM:     31916 MiB
Backend: LLVM 14.0.6
@flysand7
Copy link
Contributor

It appears that our definition of the Sig_Set type is wrong then. In the issue you have linked, someone pointed out that on 64-bit architectures the size of the structure is 64 bits in x86-64. I don't know how I found the number 128 for the size of this structure, but it felt off all this time.

As for the user-facing API i've been considering making sigset a bitset, there was just an issue where first signal would correspond to bit 0, so I didn't want to tie it to a different type. Maybe I'll reconsider this now that I know its not more than register size.

@Feoramund
Copy link
Contributor Author

I'm assuming our Sig_Set was derived from <bits/types/__sigset_t.h>, but it looks like the __NSIG_BYTES macro (also from glibc) is what the Linux kernel expects in this instance.

#define _SIGSET_NWORDS (1024 / (8 * sizeof (unsigned long int)))
typedef struct
{
  unsigned long int __val[_SIGSET_NWORDS];
} __sigset_t;

I did a little more digging, but I haven't been able to find what the extra memory is for, other than an assumed possible future expansion to keep the API stable until then. There was even an issue on the go project about this too. golang/go#55349

I suppose it's a small comfort that two other projects had run into this confusing issue too.

@jasonKercher
Copy link
Contributor

I actually have this fixed and working in a my branch. size_of(Sig_Set) should indeed be 8. I had to use this with rt_sigprocmask for timed process waiting. You can see it is actually hard-coded in the kernel to return EINVAL on anything else:

SYSCALL_DEFINE4(rt_sigprocmask, int, how, sigset_t __user *, nset,
                sigset_t __user *, oset, size_t, sigsetsize)
{
        sigset_t old_set, new_set;
        int error;

        /* XXX: Don't preclude handling different sized sigset_t's.  */
        if (sigsetsize != sizeof(sigset_t))
                return -EINVAL;

        ...

You will always get EINVAL on any number other than 8 as it currently stands.

We could use a bit_set here. It would just need to be made clear that the enum values that are used as flags for the bit_set are not the signal numbers because, as @flysand7 pointed out, they will be off by one. My usage worked, but without a bit_set, I ended up doing this:

mask: bit_set[0..=63]
mask += { int(linux.Signal.SIGCHLD) - 1 }

I guess that is what sigemptyset, sigaddset, sigdelset`, etc. are for.

@flysand7
Copy link
Contributor

flysand7 commented Apr 28, 2024

@jasonKercher Didn't realise you were doing core:os2/process work on linux, I was about to double that effort by making it part of my PR haha

@jasonKercher
Copy link
Contributor

@flysand7 Yup. Now, it runs on the much improved sys/linux =] I'm not attached to any of the code though. I'm just doing nuts and bolts work to get from point A to B.

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

No branches or pull requests

3 participants