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

[Christian Brauner <christian dot brauner at ubuntu dot com>]

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