From fac3689e77d37aca9c85dafabf921a847b1c4e02 Mon Sep 17 00:00:00 2001 From: Andrew Jeffery Date: Fri, 17 May 2019 16:34:04 +0930 Subject: [PATCH] dbus: Unref replies to avoid leaks Testing showed that failing to free DBus message replies was leaking 800-900kiB per boot. After adding the appropriate unrefs, mboxd memory consumption stayed stable at 71920kiB across multiple reboots of the host. The root cause was identified using a DBus capture of the host's IPMI traffic during boot, then reducing the output to mboxd-specific messages and turning them into commands that could be run with ipmitool. Adding all of these commands to a script and running `pmap` between ipmitool invocations showed the growth in memory usage across the "boot" process, but this did not correlate with any particular set of commands to mboxd. The lack of correlation lead to the hypothesis that we might be able to reproduce by sending a lot of dbus messages, such as: ``` root@witherspoon:/tmp# for i in `seq 1 10000`; do \ busctl call xyz.openbmc_project.Hiomapd \ /xyz/openbmc_project/Hiomapd \ xyz.openbmc_project.Hiomapd.Protocol.V2 \ GetInfo y 2; \ done ``` Spamming the daemon in this way demonstrated the growth in memory seen during a regular boot process, confirming that just sending DBus messages was enough. Add the necessary unrefs for the replies at the end of each method handler to ensure the replies are appropriately freed. Testing and confirmation of the fix were performed on a Witherspoon system. Change-Id: If5fe7576aaca1be617181526bf64511ccee1dd9f Signed-off-by: Andrew Jeffery --- control_dbus.c | 8 ++++++-- control_legacy.c | 1 + transport_dbus.c | 36 +++++++++++++++++++++++++++--------- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/control_dbus.c b/control_dbus.c index 322c743..4fb9e33 100644 --- a/control_dbus.c +++ b/control_dbus.c @@ -43,7 +43,9 @@ static int control_dbus_directive(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int control_dbus_ping(sd_bus_message *m, void *userdata, @@ -190,7 +192,9 @@ static int control_dbus_set_backend(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int control_dbus_get_u8(sd_bus *bus, const char *path, diff --git a/control_legacy.c b/control_legacy.c index 3e9a6df..2a3461d 100644 --- a/control_legacy.c +++ b/control_legacy.c @@ -278,6 +278,7 @@ out: } rc = sd_bus_send(NULL, n, NULL); /* Send response */ + sd_bus_message_unref(n); if (rc < 0) MSG_ERR("sd_bus_send failed: %d\n", rc); diff --git a/transport_dbus.c b/transport_dbus.c index 42fb018..b6eb0e1 100644 --- a/transport_dbus.c +++ b/transport_dbus.c @@ -93,7 +93,9 @@ static int transport_dbus_reset(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_get_info(sd_bus_message *m, void *userdata, @@ -146,7 +148,9 @@ static int transport_dbus_get_info(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_get_flash_info(sd_bus_message *m, void *userdata, @@ -181,7 +185,9 @@ static int transport_dbus_get_flash_info(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_create_window(struct mbox_context *context, @@ -225,7 +231,9 @@ static int transport_dbus_create_window(struct mbox_context *context, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_create_read_window(sd_bus_message *m, void *userdata, @@ -274,7 +282,9 @@ static int transport_dbus_close_window(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } @@ -308,7 +318,9 @@ static int transport_dbus_mark_dirty(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_write_flush(sd_bus_message *m, void *userdata, @@ -334,7 +346,9 @@ static int transport_dbus_write_flush(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_ack(sd_bus_message *m, void *userdata, @@ -367,7 +381,9 @@ static int transport_dbus_ack(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_erase(sd_bus_message *m, void *userdata, @@ -400,7 +416,9 @@ static int transport_dbus_erase(sd_bus_message *m, void *userdata, return rc; } - return sd_bus_send(NULL, n, NULL); + rc = sd_bus_send(NULL, n, NULL); + sd_bus_message_unref(n); + return rc; } static int transport_dbus_get_property(sd_bus *bus, -- 2.25.0