From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0145EC433B4 for ; Thu, 6 May 2021 15:09:41 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 39D2D61166 for ; Thu, 6 May 2021 15:09:40 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 39D2D61166 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=bsdimp.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:48606 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lefdD-0008P8-3A for qemu-devel@archiver.kernel.org; Thu, 06 May 2021 11:09:39 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:34908) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lefbg-0007Gr-VR for qemu-devel@nongnu.org; Thu, 06 May 2021 11:08:05 -0400 Received: from mail-qk1-x72c.google.com ([2607:f8b0:4864:20::72c]:41728) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lefbe-0002nI-LO for qemu-devel@nongnu.org; Thu, 06 May 2021 11:08:04 -0400 Received: by mail-qk1-x72c.google.com with SMTP id l129so5195790qke.8 for ; Thu, 06 May 2021 08:08:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7egZ91lm04uZj+IsPG0NVHnVuWyoWKPFXJjtyiUoCkw=; b=diJS2U3AJG0Q2IcHHENe9FqrMqQvkwbQnX5kO1LuK7ZQ4PCSTYl54KAXxXZdNIDPif uxif6knAaYxHBI6vQcCv46KBLpCciMoy+/QGZfkVy1l/9f0xWgtca8Le3r+cb52ZCx57 4ErURmr6d6CqGxoTEQ46wnlUMXdj0KBBRkg5JD+RSrUQbXWrv8DrkVHHBNGMfXOVUu2O 5wftvMADcf/YLBncthMo/XclxJftLmwaChGCxScxyHsqKR6zJcrmvlfoxC5DsldODqSr GZUh1i/K9mQBKOVN4W8E7cyLnuXxra06zruH/DKcZtQHFHNPr534S0ANIhyXNa/VQZvj suSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=7egZ91lm04uZj+IsPG0NVHnVuWyoWKPFXJjtyiUoCkw=; b=hYeAvKn2eZBU1kRmiKOyndIcAmA5rfrxmOTTb3gHBAEPwvTvb98FQWtzLb82BBLlbc dSqZ7Fd8SMUex/EUwQrX5fSOpP4e9qL3uFXSAQp9xKzsxtPIS6UnOau5F1+pL9VX3kdm cNobYUh9U0G+LKwOePuk+OvVTQuAhAqiGzbJuGECmV23jbmX+oRIarIg4dXCTTOVwxW6 Ynu3jxo4PLbwbWRmkwkZZz9MUYwhkOFkz09VXlAu7O72zZ84Wqb3PJMUn/cA8+kAWlez w3KL5uLwBCGnlVk/bCqAjwxGAlVZIECQLPDWqDxYHRXBIDxPsCkSq8BXq0m9NuS2mqlH bjmQ== X-Gm-Message-State: AOAM533S1jQR+r+PQpquZqZlbHcktIKZnivk4E1rzftHm4Pl24eFrNhx FGEyutLgiNPeSLuBpSvlfN30TnA63dp+vQwIIrbBpg== X-Google-Smtp-Source: ABdhPJwMxr0rx6Wt17J390oM45FL+kykdz1SOx1ArlIbbbnxUKQ/l+ep0t0SQUDWMy+WX0OoNkoOFpadqrJl4ML8kQA= X-Received: by 2002:a37:f512:: with SMTP id l18mr4338093qkk.89.1620313680338; Thu, 06 May 2021 08:08:00 -0700 (PDT) MIME-Version: 1.0 References: <20210506133758.1749233-1-philmd@redhat.com> <20210506133758.1749233-5-philmd@redhat.com> <994028a8-8d62-0355-0343-f721d6f256f6@redhat.com> In-Reply-To: <994028a8-8d62-0355-0343-f721d6f256f6@redhat.com> From: Warner Losh Date: Thu, 6 May 2021 09:07:49 -0600 Message-ID: Subject: Re: [PATCH v2 4/9] bsd-user/syscall: Replace alloca() by g_new() To: =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Content-Type: multipart/alternative; boundary="0000000000005775d705c1aaad51" Received-SPF: none client-ip=2607:f8b0:4864:20::72c; envelope-from=wlosh@bsdimp.com; helo=mail-qk1-x72c.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , kvm-devel , Kyle Evans , QEMU Developers , qemu-arm , qemu-ppc , Gerd Hoffmann , Paolo Bonzini Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000005775d705c1aaad51 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, May 6, 2021 at 8:57 AM Philippe Mathieu-Daud=C3=A9 wrote: > On 5/6/21 4:48 PM, Warner Losh wrote: > > > > > > On Thu, May 6, 2021 at 8:21 AM Peter Maydell > > wrote: > > > > On Thu, 6 May 2021 at 15:17, Warner Losh > > wrote: > > > > > > > > > > > > On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-Daud=C3=A9 > > > wrote: > > >> > > >> The ALLOCA(3) man-page mentions its "use is discouraged". > > >> > > >> Replace it by a g_new() call. > > >> > > >> Signed-off-by: Philippe Mathieu-Daud=C3=A9 > > > > >> --- > > >> bsd-user/syscall.c | 3 +-- > > >> 1 file changed, 1 insertion(+), 2 deletions(-) > > >> > > >> diff --git a/bsd-user/syscall.c b/bsd-user/syscall.c > > >> index 4abff796c76..dbee0385ceb 100644 > > >> --- a/bsd-user/syscall.c > > >> +++ b/bsd-user/syscall.c > > >> @@ -355,9 +355,8 @@ abi_long do_freebsd_syscall(void *cpu_env, > > int num, abi_long arg1, > > >> case TARGET_FREEBSD_NR_writev: > > >> { > > >> int count =3D arg3; > > >> - struct iovec *vec; > > >> + g_autofree struct iovec *vec =3D g_new(struct iovec= , > > count); > > > > > > > > > Where is this freed? > > > > g_autofree, so it gets freed when it goes out of scope. > > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-= autofree > > < > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-= autofree > > > > > > > > Ah. I'd missed that feature and annotation... Too many years seeing > > patches like > > this in other projects where a feature like this wasn't there to save > > the day... > > > > This means you can ignore my other message as equally misinformed. > > This also shows my commit description is poor. > > > > Also, alloca just moves a stack pointer, where malloc has complex > > interactions. Are you sure that's a safe change here? > > > > alloca()ing something with size determined by the guest is > > definitely not safe :-) malloc as part of "handle this syscall" > > is pretty common, at least in linux-user. > > > > > > Well, since this is userland emulation, the normal process boundaries > > will fix that. allocating from > > the heap is little different..., so while unsafe, it's the domain of > > $SOMEONE_ELSE to enforce > > the safety. linux-user has many similar usages for both malloc and > > alloca, and it's ok for the > > same reason. > > > > Doing a quick grep on my blitz[*] branch in the bsd-user fork shows tha= t > > alloca is used almost > > exclusively there. There's 24 calls to alloca in the bsd-user code. > > There's almost no malloc > > calls (7) in that at all outside the image loader: all but one of them > > are confined to sysctl > > emulation with the last one used to keep track of thread state in new > > threads... > > linux-user has a similar profile (20 alloca's and 14 mallocs outside th= e > > loader), > > but with more mallocs in other paths than just sysctl (which isn't > present). > > > > I had no plans on migrating to anything else... > > I considered excluding user emulation (both Linux/BSD) by enabling > CFLAGS=3D-Walloca for everything except user-mode, but Meson doesn't > support adding flags to a specific source set: > https://github.com/mesonbuild/meson/issues/1367#issuecomment-277929767 > > Q: Is it possible to add a flag to a specific file? I have some > generated code that's freaking the compiler out and I don't > feel like mucking with the generator. > > A: We don't support per-file compiler flags by design. It interacts > very poorly with other parts, especially precompiled headers. > The real solution is to fix the generator to not produce garbage. > Obviously this is often not possible so the solution to that is, > as mentioned above, build a static library with the specific > compiler args. This has the added benefit that it makes this > workaround explicit and visible rather than hiding things in > random locations. > > Then Paolo suggested to convert all alloca() calls instead. > I'm having trouble understanding how the emulated system calls can still be async safe if that's done. I might be missing something in the CPU emulation that would clear up the concern, though. How threads interact with each-other in emulation is an area I'm still learning. Warner P.S. Of course, no matter what you do, the current in-tree bsd-user dumps core right away, so it's hard to break it worse than it already is broken :)... --0000000000005775d705c1aaad51 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, May 6, 2021 at 8:57 AM Philip= pe Mathieu-Daud=C3=A9 <philmd@redha= t.com> wrote:
On 5/6/21 4:48 PM, Warner Losh wrote:
>
>
> On Thu, May 6, 2021 at 8:21 AM Peter Maydell <peter.maydell@linaro.org
> <mailto:peter.maydell@linaro.org>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0On Thu, 6 May 2021 at 15:17, Warner Losh <imp@bsdimp.com
>=C2=A0 =C2=A0 =C2=A0<mailto:imp@bsdimp.com>> wrote:
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> On Thu, May 6, 2021, 7:38 AM Philippe Mathieu-= Daud=C3=A9
>=C2=A0 =C2=A0 =C2=A0<philmd@redhat.com <mailto:philmd@redhat.com>> wrote:
>=C2=A0 =C2=A0 =C2=A0>>
>=C2=A0 =C2=A0 =C2=A0>> The ALLOCA(3) man-page mentions its "= use is discouraged".
>=C2=A0 =C2=A0 =C2=A0>>
>=C2=A0 =C2=A0 =C2=A0>> Replace it by a g_new() call.
>=C2=A0 =C2=A0 =C2=A0>>
>=C2=A0 =C2=A0 =C2=A0>> Signed-off-by: Philippe Mathieu-Daud=C3=A9= <philmd@redhat.c= om
>=C2=A0 =C2=A0 =C2=A0<mailto:philmd@redhat.com>>
>=C2=A0 =C2=A0 =C2=A0>> ---
>=C2=A0 =C2=A0 =C2=A0>>=C2=A0 bsd-user/syscall.c | 3 +--
>=C2=A0 =C2=A0 =C2=A0>>=C2=A0 1 file changed, 1 insertion(+), 2 de= letions(-)
>=C2=A0 =C2=A0 =C2=A0>>
>=C2=A0 =C2=A0 =C2=A0>> diff --git a/bsd-user/syscall.c b/bsd-user= /syscall.c
>=C2=A0 =C2=A0 =C2=A0>> index 4abff796c76..dbee0385ceb 100644
>=C2=A0 =C2=A0 =C2=A0>> --- a/bsd-user/syscall.c
>=C2=A0 =C2=A0 =C2=A0>> +++ b/bsd-user/syscall.c
>=C2=A0 =C2=A0 =C2=A0>> @@ -355,9 +355,8 @@ abi_long do_freebsd_sy= scall(void *cpu_env,
>=C2=A0 =C2=A0 =C2=A0int num, abi_long arg1,
>=C2=A0 =C2=A0 =C2=A0>>=C2=A0 =C2=A0 =C2=A0 case TARGET_FREEBSD_NR= _writev:
>=C2=A0 =C2=A0 =C2=A0>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 int count =3D arg3;
>=C2=A0 =C2=A0 =C2=A0>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= struct iovec *vec;
>=C2=A0 =C2=A0 =C2=A0>> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= g_autofree struct iovec *vec =3D g_new(struct iovec,
>=C2=A0 =C2=A0 =C2=A0count);
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0>
>=C2=A0 =C2=A0 =C2=A0> Where is this freed?
>
>=C2=A0 =C2=A0 =C2=A0g_autofree, so it gets freed when it goes out of sc= ope.
>=C2=A0 =C2=A0 =C2=A0https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html= #g-autofree
>=C2=A0 =C2=A0 =C2=A0<https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.= html#g-autofree>
>
>
> Ah. I'd missed that feature and annotation...=C2=A0 Too many years= seeing
> patches like
> this in other projects where a feature like this wasn't there to s= ave
> the day...
>
> This means you can ignore my other message as equally misinformed.

This also shows my commit description is poor.

>=C2=A0 =C2=A0 =C2=A0> Also, alloca just moves a stack pointer, where= malloc has complex
>=C2=A0 =C2=A0 =C2=A0interactions. Are you sure that's a safe change= here?
>
>=C2=A0 =C2=A0 =C2=A0alloca()ing something with size determined by the g= uest is
>=C2=A0 =C2=A0 =C2=A0definitely not safe :-) malloc as part of "han= dle this syscall"
>=C2=A0 =C2=A0 =C2=A0is pretty common, at least in linux-user.
>
>
> Well, since this is userland emulation, the normal process boundaries<= br> > will fix that. allocating from
> the heap is little different..., so while unsafe, it's the domain = of
> $SOMEONE_ELSE to enforce
> the safety. linux-user has many similar usages for both malloc and
> alloca, and it's ok for the
> same reason.
>
> Doing a quick grep on my blitz[*] branch in the bsd-user fork shows th= at
> alloca is used almost
> exclusively there. There's 24 calls to alloca in the bsd-user code= .
> There's almost no malloc
> calls (7) in that at all outside the image loader: all but one of them=
> are confined to sysctl
> emulation with the last one used to keep track of thread state in new<= br> > threads...
> linux-user has a similar profile (20 alloca's and 14 mallocs outsi= de the
> loader),
> but with more mallocs in other paths than just sysctl (which isn't= present).
>
> I had no plans on migrating to anything else...

I considered excluding user emulation (both Linux/BSD) by enabling
CFLAGS=3D-Walloca for everything except user-mode, but Meson doesn't support adding flags to a specific source set:
https://github.com/mesonbuild/= meson/issues/1367#issuecomment-277929767

=C2=A0 Q: Is it possible to add a flag to a specific file? I have some
=C2=A0 =C2=A0 =C2=A0generated code that's freaking the compiler out and= I don't
=C2=A0 =C2=A0 =C2=A0feel like mucking with the generator.

=C2=A0 A: We don't support per-file compiler flags by design. It intera= cts
=C2=A0 =C2=A0 =C2=A0very poorly with other parts, especially precompiled he= aders.
=C2=A0 =C2=A0 =C2=A0The real solution is to fix the generator to not produc= e garbage.
=C2=A0 =C2=A0 =C2=A0Obviously this is often not possible so the solution to= that is,
=C2=A0 =C2=A0 =C2=A0as mentioned above, build a static library with the spe= cific
=C2=A0 =C2=A0 =C2=A0compiler args. This has the added benefit that it makes= this
=C2=A0 =C2=A0 =C2=A0workaround explicit and visible rather than hiding thin= gs in
=C2=A0 =C2=A0 =C2=A0random locations.

Then Paolo suggested to convert all alloca() calls instead.

I'm having trouble understanding how the emulated = system calls
can still be async safe if that's done. I might = be missing something
in the CPU emulation that would clear up the= concern, though. How
threads interact with each-other in emulati= on is an area I'm still
learning.

Wa= rner

P.S. Of course, no matter what you do, the cu= rrent in-tree
bsd-user dumps core right away, so it's hard to= break it worse
than it already is broken :)...
--0000000000005775d705c1aaad51--