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=-10.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,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 A7DE8C433E0 for ; Tue, 16 Mar 2021 14:50:30 +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 3225465087 for ; Tue, 16 Mar 2021 14:50:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3225465087 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cloud.ionos.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:60116 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lMB1f-0000zJ-JK for qemu-devel@archiver.kernel.org; Tue, 16 Mar 2021 10:50:27 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:46714) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lMAyP-0005tF-Dl for qemu-devel@nongnu.org; Tue, 16 Mar 2021 10:47:05 -0400 Received: from mail-ej1-x633.google.com ([2a00:1450:4864:20::633]:39830) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1lMAyK-0007Bh-LA for qemu-devel@nongnu.org; Tue, 16 Mar 2021 10:47:04 -0400 Received: by mail-ej1-x633.google.com with SMTP id p7so61286663eju.6 for ; Tue, 16 Mar 2021 07:46:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cloud.ionos.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5f9u944MQbSEWJBImNdcHfjetpII+YrdudOUKPpv/Kk=; b=I+j700pSIZCzAv/DfklDDPuLPMz3vJyYXe4FkKO1Dty9OgD4I8sOF4xPKhEEzNuISk FdlKt9euyGEwohfArM/+R7QKXf0CXouZzR1BITIbDB0cdYAZ2RRs9fEgSaYr7GEv2VFT 8HP1JylTAnJtkRKY1W0SUSYRaQ+IK9ubiVfdVbdq1rUJKYNKlMnxBNtF3ibnlILjlbtN cCLhDNBqfPgwhWMI0A/UzWnDipsmbXazUwWBAj3h5KG0VIZqpUM3GvrBY40vOASvOyvV 1m5RzHZq8/RmJeklbYhiFrIGPh6QVbyaNBak0RkfjH7J9KlCetQ5WZ2YRRsVblVYLq6X fwDQ== 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=5f9u944MQbSEWJBImNdcHfjetpII+YrdudOUKPpv/Kk=; b=F4FKTl8drsgB4jGS3GgVb9CohzZxE+f1I5p4rSVewAYOokX/7LLmLNqUj4NKsmgqx3 P5jNG0orMXcnNypW5b1BZEvJAagZp2pDrz83hCW1Uc7m/K2xXqffEj+iBWkUGGIg6yFB BGib7AEUTqax9iMLPvY3bQUIVGqdcT6bggKeSRJWlBdo2Q6WC/KB39HxDKn/YIMADLH7 xmH0h1M560do1Jg3Zj3DLvxnVdfPeob3Hzp2ArgOCqModHKu6y7YEN1AjI/pjitvhHTJ ApJHyDb7xCzFHgjdjY+r2oVVSXufjJI0ltsi59BUlZA9SwRz3w4+qR62WSNgmoZ2ZX8D BIbQ== X-Gm-Message-State: AOAM533Hpq2KQQLWYeoMvVKQLWbDazGaPaD01rmNRH0NaxS5LNoHCP4B 4Xc0bwJeIceN/PcYxKRpMVBrM132zG5qliX89oGuTw== X-Google-Smtp-Source: ABdhPJwnOwc81Bbv0X4Ca9DknOPXX250TR50tvz0RctvD61heQqV6ikBnqbVvDKRfAwlOvVHj/Z+VrNEwfwa/Uw6BvE= X-Received: by 2002:a17:906:fcb2:: with SMTP id qw18mr29276942ejb.434.1615906017567; Tue, 16 Mar 2021 07:46:57 -0700 (PDT) MIME-Version: 1.0 References: <20210315170636.704201-1-zhlcindy@gmail.com> In-Reply-To: From: Li Zhang Date: Tue, 16 Mar 2021 15:46:47 +0100 Message-ID: Subject: Re: [PATCH 1/2] Fix the segment fault when calling yank_register_instance To: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Content-Type: multipart/alternative; boundary="0000000000002afb7805bda870de" Received-SPF: pass client-ip=2a00:1450:4864:20::633; envelope-from=li.zhang@cloud.ionos.com; helo=mail-ej1-x633.google.com X-Spam_score_int: -19 X-Spam_score: -2.0 X-Spam_bar: -- X-Spam_report: (-2.0 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, 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" --0000000000002afb7805bda870de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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. 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 *chr= , >> 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 should > still be possible to call the yank functions then. The registration logic > needs to be reworked during chardev-change. > > -- > Marc-Andr=C3=A9 Lureau > --0000000000002afb7805bda870de Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Marc-Andr=C3=A9,

By looking into cha= rdev and yank_register_function logic,=C2=A0 this old chardev is registered= =C2=A0according to the chardev label.=C2=A0
So it's been in= =C2=A0yank_instance_list. yank instance only has a chardev label, and the n= ew chardev's label is the same as the old chardev.
So it does= n't need to register it again when changing the chardev backend. Otherw= ise, it will report=C2=A0 duplicated yank instances.=C2=A0
I thin= k the chardev logic has no problems. And it works with yank functions.=C2= =A0

Thanks
Li=C2=A0=C2=A0
On Mon, M= ar 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 <zhlcindy@gmail.com> wrote:
=
From: Li Zhang <= li.zhang@clou= d.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
--0000000000002afb7805bda870de--