odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
6.89k stars 606 forks source link

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

Open Feoramund opened 6 months ago

Feoramund commented 6 months ago

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. https://github.com/ziglang/zig/issues/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 commented 6 months ago

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 commented 6 months ago

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. https://github.com/golang/go/issues/55349

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

jasonKercher commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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