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=-7.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no 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 454CCC433E0 for ; Tue, 16 Mar 2021 15:27:03 +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 CAAB6650E1 for ; Tue, 16 Mar 2021 15:27:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CAAB6650E1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:36924 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lMBb3-00015n-JZ for qemu-devel@archiver.kernel.org; Tue, 16 Mar 2021 11:27:01 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:55642) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lMBZG-0000It-LF for qemu-devel@nongnu.org; Tue, 16 Mar 2021 11:25:10 -0400 Received: from mail-ed1-x52a.google.com ([2a00:1450:4864:20::52a]:40668) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lMBZD-0004yI-Uq for qemu-devel@nongnu.org; Tue, 16 Mar 2021 11:25:10 -0400 Received: by mail-ed1-x52a.google.com with SMTP id b16so9247696eds.7 for ; Tue, 16 Mar 2021 08:25:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pzorjPCWcLubXFiOy8kMAn2giJo5L2lx5aTYtcvsXSE=; b=O+uF1AHenwb5YY6b57qikFX344ozm1bI1kgfilVM8wHJbkq3mtU4wJgvFrxOL+9Hfm AqmKxRoDljnoJXrHtrrEoI9XYxZP/Dl4uH4KP7MCvq9h2mHPqm6l9HY3krDwGDnzzJ8H vVNdJc87ugEeUCfJnRouPKbp4Ef+Bp36MV9u6rRNYiXEpUQozJlewLEUzcbrr/YPMYDR /tDLFe2cY/Awb5XSIK54moGGStgkZEF1S7TLGWKFuMBMBxewZcra4UxU5napZ3fO+frr IUa9bvUNeVIJR9oYfxKEA90I2DMg9nTwrzyWimBmGBbOLLXP6UNouCvun4CxEBJ9PMQR GEyg== 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=pzorjPCWcLubXFiOy8kMAn2giJo5L2lx5aTYtcvsXSE=; b=osevIwd6T0dldp+wG0RS8FPzj+UW/TD3/G7gMOVYrWp+5zEJo+42m2gfLXenZrA9bZ DPt4O76Pa4Ubax+ZgeunrgReOwOow+6xM8145ua9P5PJ7N6uTJ1V+adh+Lx9hUaRO7wa kYVfgGKfaZNCUBWw3VPTlw4yRSEyw36G6kukJgLd6epBzQwnGs9RZ7CN5p2Av7Z05zL7 JuBKNpoxXRZPToXf9T8Kn2QmsX2++UUKWo7Z5HzxIK47/mLRdSbN5IbzS9L+Xq8cnIcQ ukZdr5ugg3qjLZU5CBKJjX7U4JVut2uYdZPKAoYlGKbunM23dxylRBXCaoZNjXDRFifc c64w== X-Gm-Message-State: AOAM533QYOf9aBnwBXEyv5OJTUvtuMKk7fHVfLhBKNcAaIqOXYYDYRUr 08Atjv+L9H9sNycB/NT/c09f9XSvNvdkCBEu9lk= X-Google-Smtp-Source: ABdhPJxjiTrvVGyHS2haq3gz1r6U5nV2yFaVrCKYUFdpKuy40CirNQOTSDfXYfMqRIHB37XTFuREufwKXNe7sUcT+z8= X-Received: by 2002:aa7:dbd3:: with SMTP id v19mr36346287edt.314.1615908306398; Tue, 16 Mar 2021 08:25:06 -0700 (PDT) MIME-Version: 1.0 References: <20210315170636.704201-1-zhlcindy@gmail.com> In-Reply-To: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Tue, 16 Mar 2021 19:24:54 +0400 Message-ID: Subject: Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance To: Li Zhang Content-Type: multipart/alternative; boundary="00000000000097af9305bda8f806" Received-SPF: pass client-ip=2a00:1450:4864:20::52a; envelope-from=marcandre.lureau@gmail.com; helo=mail-ed1-x52a.google.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-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: Lukas Straub , alexandr.iarygin@profitbricks.com, Markus Armbruster , QEMU , Pankaj Gupta , Li Zhang Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000097af9305bda8f806 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Tue, Mar 16, 2021 at 6:46 PM Li Zhang wrote: > Hi Marc-Andr=C3=A9, > > By looking into chardev and yank_register_function logic, this old > chardev is registered according to the chardev label. > So it's been in yank_instance_list. yank instance only has a chardev > label, and the new chardev's label is the same as the old chardev. > So it doesn't need to register it again when changing the chardev backend= . > Otherwise, it will report duplicated yank instances. > I think the chardev logic has no problems. And it works with yank > functions. > > The previous instance is being removed with yank_unregister_instance() during object_unparent(). The new instance is not registered. That scenario deserves tests. (it's a shame there are no tests for yank ...= ) > Thanks > Li > > On Mon, Mar 15, 2021 at 7:51 PM Marc-Andr=C3=A9 Lureau < > marcandre.lureau@gmail.com> wrote: > >> Hi >> >> On Mon, Mar 15, 2021 at 9:22 PM Li Zhang wrote: >> >>> From: Li Zhang >>> >>> When executing the QMP commands "chardev-change" to change the >>> backend device to socket, it will cause a segment fault because >>> it assumes chr->label as non-NULL in function yank_register_instance. >>> The function qmp_chardev_change calls chardev_new, which label >>> is NULL when creating a new chardev. The label will be passed to >>> yank_register_instance which causes a segment fault. The callchain >>> is as the following: >>> chardev_new -> >>> qemu_char_open -> >>> cc->open -> >>> qmp_chardev_open_socket -> >>> yank_register_instance >>> >>> Signed-off-by: Li Zhang >>> --- >>> chardev/char-socket.c | 8 +++++--- >>> 1 file changed, 5 insertions(+), 3 deletions(-) >>> >>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c >>> index c8bced76b7..26d5172682 100644 >>> --- a/chardev/char-socket.c >>> +++ b/chardev/char-socket.c >>> @@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *ch= r, >>> qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_FD_PASS); >>> } >>> >>> - if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), >>> errp)) { >>> - return; >>> + if (chr->label) { >>> + if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->label), >>> errp)) { >>> + return; >>> + } >>> + s->registered_yank =3D true; >>> } >>> - s->registered_yank =3D true; >>> >>> /* be isn't opened until we get a connection */ >>> *be_opened =3D false >>> >> >> Looks wrong to me, the new chardev will get the same label, and it shoul= d >> still be possible to call the yank functions then. The registration logi= c >> needs to be reworked during chardev-change. >> >> -- >> Marc-Andr=C3=A9 Lureau >> > --=20 Marc-Andr=C3=A9 Lureau --00000000000097af9305bda8f806 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Tue, Mar 16, 2021 at 6:46 PM Li = Zhang <li.zhang@cloud.ionos.= com> wrote:
Hi Marc-Andr=C3=A9,

By looking into = chardev and yank_register_function logic,=C2=A0 this old chardev is registe= red=C2=A0according to the chardev label.=C2=A0
So it's been i= n=C2=A0yank_instance_list. yank instance only has a chardev label, and the = new chardev's label is the same as the old chardev.
So it doe= sn't need to register it again when changing the chardev backend. Other= wise, it will report=C2=A0 duplicated yank instances.=C2=A0
I thi= nk the chardev logic has no problems. And it works with yank functions.=C2= =A0


The previous= instance is being removed with yank_unregister_instance() during object_un= parent(). The new instance is not registered.

That= scenario deserves tests. (it's a shame there are no tests for yank ...= )

=C2=A0
Thanks
Li=C2= =A0=C2=A0

On Mon, Mar 15, 2021 at 7:51 PM Marc-Andr=C3=A9 Lureau <= marcandre.l= ureau@gmail.com> wrote:
Hi

On Mon, Mar 15, 2021= at 9:22 PM Li Zhang <zhlcindy@gmail.com> wrote:
From: Li Zhang <li.zhang@cloud.ionos.com>

When executing the QMP commands "chardev-change" to change the backend device to socket, it will cause a segment fault because
it assumes chr->label as non-NULL in function yank_register_instance. The function qmp_chardev_change calls chardev_new, which label
is NULL when creating a new chardev. The label will be passed to
yank_register_instance which causes a segment fault. The callchain
is as the following:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 chardev_new ->
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_char_open ->
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 cc->open -> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qmp_chardev_open_so= cket ->
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 yank_= register_instance

Signed-off-by: Li Zhang <li.zhang@cloud.ionos.com>
---
=C2=A0chardev/char-socket.c | 8 +++++---
=C2=A01 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index c8bced76b7..26d5172682 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1421,10 +1421,12 @@ static void qmp_chardev_open_socket(Chardev *chr, =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qemu_chr_set_feature(chr, QEMU_CHAR_FEATU= RE_FD_PASS);
=C2=A0 =C2=A0 =C2=A0}

-=C2=A0 =C2=A0 if (!yank_register_instance(CHARDEV_YANK_INSTANCE(chr->la= bel), errp)) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
+=C2=A0 =C2=A0 if (chr->label) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!yank_register_instance(CHARDEV_YANK_INSTA= NCE(chr->label), errp)) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 s->registered_yank =3D true;
=C2=A0 =C2=A0 =C2=A0}
-=C2=A0 =C2=A0 s->registered_yank =3D true;

=C2=A0 =C2=A0 =C2=A0/* be isn't opened until we get a connection */
=C2=A0 =C2=A0 =C2=A0*be_opened =3D false

Looks wrong to me, the new chardev will get the same label,= and it should still be possible to call the yank functions then. The regis= tration logic needs to be reworked during chardev-change.
--
Marc-Andr=C3=A9 Lureau


--
Marc-Andr=C3=A9 Lureau
--00000000000097af9305bda8f806--