From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:37245) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h2Dgy-0002La-30 for qemu-devel@nongnu.org; Fri, 08 Mar 2019 06:29:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h2Dgw-000064-1b for qemu-devel@nongnu.org; Fri, 08 Mar 2019 06:29:31 -0500 Received: from mail-wm1-f65.google.com ([209.85.128.65]:35341) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1h2Dgv-0008Sm-Rb for qemu-devel@nongnu.org; Fri, 08 Mar 2019 06:29:29 -0500 Received: by mail-wm1-f65.google.com with SMTP id y15so12150118wma.0 for ; Fri, 08 Mar 2019 03:29:29 -0800 (PST) From: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20190308013222.12524-1-philmd@redhat.com> <20190308013222.12524-11-philmd@redhat.com> <40a49a86-3010-da90-41f3-0f4a1705bca0@redhat.com> Message-ID: <9dc406df-6c19-efd9-f74c-9a1263dc78dc@redhat.com> Date: Fri, 8 Mar 2019 12:29:26 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 10/18] hw/nvram/fw_cfg: Add reboot_timeout to FWCfgState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek , Gerd Hoffmann , "Michael S. Tsirkin" , qemu-devel@nongnu.org, =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: Marcel Apfelbaum , Eduardo Habkost , Paolo Bonzini , Richard Henderson , Artyom Tarasenko , "Dr. David Alan Gilbert" , Peter Maydell , David Gibson , Igor Mammedov , Eric Blake , qemu-ppc@nongnu.org, qemu-arm@nongnu.org, Markus Armbruster , Mark Cave-Ayland , Thomas Huth , "Daniel P . Berrange" On 3/8/19 12:22 PM, Philippe Mathieu-Daudé wrote: > On 3/8/19 12:04 PM, Laszlo Ersek wrote: >> Hi Phil, >> >> On 03/08/19 02:32, Philippe Mathieu-Daudé wrote: >>> Due to the contract interface of fw_cfg_add_file(), the >>> 'reboot_timeout' data has to be valid for the lifetime of the >>> FwCfg object. For this reason it is copied on the heap with >>> memdup(). >>> >>> The object state, 'FWCfgState', is also meant to be valid during the >>> lifetime of the object. >>> Move the 'reboot_timeout' in FWCfgState to achieve the same purpose. >>> Doing so we avoid a memory leak. >>> >>> Signed-off-by: Philippe Mathieu-Daudé >>> --- >>> hw/nvram/fw_cfg.c | 4 +++- >>> include/hw/nvram/fw_cfg.h | 2 ++ >>> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> Currently, there is no memory leak. Right now, the leak is theoretical, >> and it would depend on the fw_cfg object being actually destroyed. > > Actually my first motivation came while using valgrind, there are a > bunch of warnings related to the fw_cfg device. > This device is not hotpluggable however, and we don't test it in the > device-introspect-test. IOW this device makes finding memory leaks in other introspectable devices harder. >> I think armoring the fw_cfg implementation for such lifetime actions is >> valuable. But, that definitely belongs to its own series, in my opinion. >> >> In the "hw/nvram/fw_cfg.c" file, I count: >> >> (a) two "specific purpose" g_memdup() calls, namely in >> fw_cfg_bootsplash() and in fw_cfg_reboot(); >> >> (b) one "generic purpose" g_memdup() call, namely in fw_cfg_add_string(); >> >> (c) two "generic purpose" g_malloc() calls, namely in fw_cfg_add_i16(), >> fw_cfg_add_i32(), and fw_cfg_add_i64(). (The one in fw_cfg_modify_i16() >> does not matter here because the previous blob is freed in that function.) >> >> Your series deals with (a), namely with fw_cfg_reboot() in this patch, >> and with fw_cfg_bootsplash() in the next one. >> >> Your series deals with neither (b) nor (c). The > > I did a PoC of (b) and (c) but it is a more invasive patchset indeed. > >> fw_cfg_add_(string|i16|i32|i64) functions are called from a bunch of >> places however, so if we really intend *not* to leak those copies upon >> fw_cfg destruction, then we'll have to track all of them dynamically, in >> a list for example. > > I haven't think of using a list. > >> (And that necessitates a separate series for this topic even more.) > > OK. > >> In turn, once we add dynamic tracking, for those blobs that the >> fw_cfg_add_(string|i16|i32|i64) functions allocate internally -- as they >> are advertized to do --, then we might as well use the same tracking >> infrastructure for (a). In other words, it should not be necessary to >> add the specific fields "reboot_timeout" and "boot_splash" to FWCfgState. > > OK, I'll drop these patches from this series. > >> >> Thanks, >> Laszlo >> >>> >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >>> index b73a591eff..182d27f59a 100644 >>> --- a/hw/nvram/fw_cfg.c >>> +++ b/hw/nvram/fw_cfg.c >>> @@ -250,7 +250,9 @@ static void fw_cfg_reboot(FWCfgState *s) >>> } >>> } >>> >>> - fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&rt_val, 4), 4); >>> + s->reboot_timeout = rt_val; >>> + fw_cfg_add_file(s, "etc/boot-fail-wait", >>> + &s->reboot_timeout, sizeof(s->reboot_timeout)); >>> } >>> >>> static void fw_cfg_write(FWCfgState *s, uint8_t value) >>> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h >>> index 828ad9dedc..99f6fafcaa 100644 >>> --- a/include/hw/nvram/fw_cfg.h >>> +++ b/include/hw/nvram/fw_cfg.h >>> @@ -53,6 +53,8 @@ struct FWCfgState { >>> dma_addr_t dma_addr; >>> AddressSpace *dma_as; >>> MemoryRegion dma_iomem; >>> + >>> + uint32_t reboot_timeout; >>> }; >>> >>> struct FWCfgIoState { >>> >>