Re: [PATCH 4/5] linux: Optimize posix_spawn spurious sigaction calls

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 07/10/2019 15:25, Christian Brauner wrote:
> On Mon, Oct 07, 2019 at 02:50:56PM -0300, Adhemerval Zanella wrote:
>> Florian, do you still hold objection to this patch?
>>
>> On 02/09/2019 16:47, Adhemerval Zanella wrote:
>>>
>>>
>>> On 02/09/2019 10:14, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>> The problem is in fact false negatives, where posix_spawn will get a mask 
>>>>> *without* the bit set, but with a set signal disposition.
>>>>
>>>> Hmm.  Right.  Incidentally, the Go routine should be fine with that:
>>>>
>>>> | // When using cgo, call the C library for sigaction, so that we call into
>>>> | // any sanitizer interceptors. This supports using the memory
>>>> | // sanitizer with Go programs. The memory sanitizer only applies to
>>>> | // C/C++ code; this permits that code to see the Go runtime's existing signal
>>>> | // handlers when registering new signal handlers for the process.
>>>> | 
>>>> | //go:cgo_import_static x_cgo_sigaction
>>>> | //go:linkname x_cgo_sigaction x_cgo_sigaction
>>>> | //go:linkname _cgo_sigaction _cgo_sigaction
>>>> | var x_cgo_sigaction byte
>>>> | var _cgo_sigaction = &x_cgo_sigaction
>>>>
>>>> libjsig also keeps calling to glibc.
>>>>
>>>> Is there anything else we should check?
>>>
>>> No idea, my take on that is once you start to calling syscall directly
>>> where libbc provide a wrapper you are in your own. We had a similar
>>> discussing with clone usage by some container applications and their
>>> expectation regarding libc internal state afterwards. 
>>>
>>>>
>>>>> In fact I think due the syscall, even relaxed operations would work
>>>>> (since the syscall acts a strong memory barrier).
>>>>
>>>> Only as a signal fence, not a thread fence.  Some architectures can even
>>>> keep cache inconsistency across fork system calls.
>>>>
>>>> I find it a bit counter-intuitive that calling sigaction or signal
>>>> directly without the glibc wrappers could lead to data corruption, even
>>>> when done for standard signals such as SIGINT.  But that's what's going
>>>> to happen with this change, unfortunately.
>>>
>>> What is counter-intuitive imho is to rely on libc to keep its internal
>>> consistency by bypassing it. This might be even worse if glibc start to
>>> wrapper the signal handler as a way to implement BZ#19702, for instance.
>>>
>>> One thing we may do it to make it clean on manual that an application is
>>> *not* expect to call sigaction using syscall().
>>>
>>>>
>>>>>>>> I wonder if we can get kernel support for this in the new clone system
>>>>>>>> call with more flags.  Then we don't have to complicate the sigaction
>>>>>>>> implementation.
>>>>>>>
>>>>>>> Maybe a CLONE_RESET_SIGNALS where the cloned process sets its signal
>>>>>>> disposition to default SIG_IGN/SIG_DFL values may help us here.  However
>>>>>>> afaik clone now is out of space on 'flags' for newer ones (it already
>>>>>>> defines 24 flags plus it reserve 8 bits for signal to be sent at process
>>>>>>> exit) and it would take time to use this feature on glibc.
>>>>>>
>>>>>> Christian Brauner has been working on fixing this.
>>>>>
>>>>> Which strategy he is proposing? Even with proper kernel support, it would
>>>>> take time to enable glibc to use it.
>>>>
>>>> Lots of flag arguments, with the reset of the arguments located
>>>> indirectly via a pointer argument.
>>>>
>>>> For a pure optimization, I think it's not too bad to require kernel
>>>> backports of system calls.
> 
> So I just accidently caught wind of this discussion. :)
> I'm open to extending clone3() to support something like the above.
> My new clone3() version has been released with Linux 5.3. It takes a
> struct clone_args. The structure is versioned by size and thus - in
> theory - extensible indefinitely.
> 
> (I also sent a PR for v5.4-rc2 that got merged for the
> copy_struct_from_user() work from Aleksa. It adds a common helper for
> copying structure arguments version by size. This will guarantee that
> future syscalls will all use the same size-versioning logic (Yes, we
> need to be careful with unions.).)
> 
> [1]: fork: add clone3
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7f192e3cd316ba58c88dfa26796cf77789dd9872
> 
> [2]: lib: introduce copy_struct_from_user() helper
>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f5a1a536fa14895ccff4e94e6a5af90901ce86aa
> 
> Christian
> 

Yeah, I am aware of it and ideally if my patch eventually gets merged
I will probably add a code path to use clone3 when possible (not sure
if it worth to be enable only for --enable-kernel or a main path with
ENOSYS fallback to clone).

However, glibc supports older kernels as old as v3.2 and it will take
some years and releases to make v5.3 or new the minimum support kernel.
And I think it would be nice to have this optimization even for older
kernels.