| Message ID | 20190308013222.12524-11-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fw_cfg: reduce memleaks, add QMP/HMP info + edk2_add_host_crypto_policy | expand |
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é <philmd@redhat.com> > --- > 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. 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 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. (And that necessitates a separate series for this topic even more.) 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. 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 { >
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é <philmd@redhat.com> >> --- >> 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. > 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 { >> >
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é <philmd@redhat.com> >>> --- >>> 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 { >>> >>
On Fri, Mar 08, 2019 at 02:32:14AM +0100, 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é <philmd@redhat.com> I don't like adding random per-file state to fw cfg at all. I don't see a leak. Please add more explanation about this. And maybe split from this series. > --- > hw/nvram/fw_cfg.c | 4 +++- > include/hw/nvram/fw_cfg.h | 2 ++ > 2 files changed, 5 insertions(+), 1 deletion(-) > > 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 { > -- > 2.20.1
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 {
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é <philmd@redhat.com> --- hw/nvram/fw_cfg.c | 4 +++- include/hw/nvram/fw_cfg.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-)