From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
To: Paul Durrant <paul@xen.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: Paul Durrant <pdurrant@amazon.com>,
Ian Jackson <iwj@xenproject.org>, Wei Liu <wl@xen.org>,
Anthony PERARD <anthony.perard@citrix.com>
Subject: Re: [PATCH v4 11/23] libxl: make sure callers of libxl_device_pci_list() free the list after use
Date: Tue, 1 Dec 2020 15:10:43 +0000 [thread overview]
Message-ID: <e6f0aad0-60bd-169e-1e70-f86d0517e551@epam.com> (raw)
In-Reply-To: <20201124080159.11912-12-paul@xen.org>
Hi, Paul!
On 11/24/20 10:01 AM, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> A previous patch introduced libxl_device_pci_list_free() which should be used
> by callers of libxl_device_pci_list() to properly dispose of the exported
> 'libxl_device_pci' types and the free the memory holding them. Whilst all
> current callers do ensure the memory is freed, only the code in xl's
> pcilist() function actually calls libxl_device_pci_dispose(). As it stands
> this laxity does not lead to any memory leaks, but the simple addition of
> .e.g. a 'string' into the idl definition of 'libxl_device_pci' would lead
> to leaks.
>
> This patch makes sure all callers of libxl_device_pci_list() can call
> libxl_device_pci_list_free() by keeping copies of 'libxl_device_pci'
> structures inline in 'pci_add_state' and 'pci_remove_state' (and also making
> sure these are properly disposed at the end of the operations) rather
> than keeping pointers to the structures returned by libxl_device_pci_list().
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Thank you,
Oleksandr
> ---
> Cc: Ian Jackson <iwj@xenproject.org>
> Cc: Wei Liu <wl@xen.org>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> ---
> tools/libs/light/libxl_pci.c | 68 ++++++++++++++++++++++++--------------------
> tools/xl/xl_pci.c | 3 +-
> 2 files changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/tools/libs/light/libxl_pci.c b/tools/libs/light/libxl_pci.c
> index d3c7a547c3..0f41939d1f 100644
> --- a/tools/libs/light/libxl_pci.c
> +++ b/tools/libs/light/libxl_pci.c
> @@ -1025,7 +1025,7 @@ typedef struct pci_add_state {
> libxl__xswait_state xswait;
> libxl__ev_qmp qmp;
> libxl__ev_time timeout;
> - libxl_device_pci *pci;
> + libxl_device_pci pci;
> libxl_domid pci_domid;
> } pci_add_state;
>
> @@ -1097,7 +1097,7 @@ static void pci_add_qemu_trad_watch_state_cb(libxl__egc *egc,
>
> /* Convenience aliases */
> libxl_domid domid = pas->domid;
> - libxl_device_pci *pci = pas->pci;
> + libxl_device_pci *pci = &pas->pci;
>
> rc = check_qemu_running(gc, domid, xswa, rc, state);
> if (rc == ERROR_NOT_READY)
> @@ -1118,7 +1118,7 @@ static void pci_add_qmp_device_add(libxl__egc *egc, pci_add_state *pas)
>
> /* Convenience aliases */
> libxl_domid domid = pas->domid;
> - libxl_device_pci *pci = pas->pci;
> + libxl_device_pci *pci = &pas->pci;
> libxl__ev_qmp *const qmp = &pas->qmp;
>
> rc = libxl__ev_time_register_rel(ao, &pas->timeout,
> @@ -1199,7 +1199,7 @@ static void pci_add_qmp_query_pci_cb(libxl__egc *egc,
> int dev_slot, dev_func;
>
> /* Convenience aliases */
> - libxl_device_pci *pci = pas->pci;
> + libxl_device_pci *pci = &pas->pci;
>
> if (rc) goto out;
>
> @@ -1300,7 +1300,7 @@ static void pci_add_dm_done(libxl__egc *egc,
>
> /* Convenience aliases */
> bool starting = pas->starting;
> - libxl_device_pci *pci = pas->pci;
> + libxl_device_pci *pci = &pas->pci;
> bool hvm = libxl__domain_type(gc, domid) == LIBXL_DOMAIN_TYPE_HVM;
>
> libxl__ev_qmp_dispose(gc, &pas->qmp);
> @@ -1516,7 +1516,10 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
> GCNEW(pas);
> pas->aodev = aodev;
> pas->domid = domid;
> - pas->pci = pci;
> +
> + libxl_device_pci_copy(CTX, &pas->pci, pci);
> + pci = &pas->pci;
> +
> pas->starting = starting;
> pas->callback = device_pci_add_stubdom_done;
>
> @@ -1555,12 +1558,6 @@ void libxl__device_pci_add(libxl__egc *egc, uint32_t domid,
>
> stubdomid = libxl_get_stubdom_id(ctx, domid);
> if (stubdomid != 0) {
> - libxl_device_pci *pci_s;
> -
> - GCNEW(pci_s);
> - libxl_device_pci_init(pci_s);
> - libxl_device_pci_copy(CTX, pci_s, pci);
> - pas->pci = pci_s;
> pas->callback = device_pci_add_stubdom_wait;
>
> do_pci_add(egc, stubdomid, pas); /* must be last */
> @@ -1619,7 +1616,7 @@ static void device_pci_add_stubdom_done(libxl__egc *egc,
>
> /* Convenience aliases */
> libxl_domid domid = pas->domid;
> - libxl_device_pci *pci = pas->pci;
> + libxl_device_pci *pci = &pas->pci;
>
> if (rc) goto out;
>
> @@ -1670,7 +1667,7 @@ static void device_pci_add_done(libxl__egc *egc,
> EGC_GC;
> libxl__ao_device *aodev = pas->aodev;
> libxl_domid domid = pas->domid;
> - libxl_device_pci *pci = pas->pci;
> + libxl_device_pci *pci = &pas->pci;
>
> if (rc) {
> LOGD(ERROR, domid,
> @@ -1680,6 +1677,7 @@ static void device_pci_add_done(libxl__egc *egc,
> rc);
> pci_info_xs_remove(gc, pci, "domid");
> }
> + libxl_device_pci_dispose(pci);
> aodev->rc = rc;
> aodev->callback(egc, aodev);
> }
> @@ -1770,7 +1768,7 @@ static int qemu_pci_remove_xenstore(libxl__gc *gc, uint32_t domid,
> typedef struct pci_remove_state {
> libxl__ao_device *aodev;
> libxl_domid domid;
> - libxl_device_pci *pci;
> + libxl_device_pci pci;
> bool force;
> bool hvm;
> unsigned int orig_vdev;
> @@ -1812,23 +1810,26 @@ static void do_pci_remove(libxl__egc *egc, pci_remove_state *prs)
> {
> STATE_AO_GC(prs->aodev->ao);
> libxl_ctx *ctx = libxl__gc_owner(gc);
> - libxl_device_pci *assigned;
> + libxl_device_pci *pcis;
> + bool attached;
> uint32_t domid = prs->domid;
> libxl_domain_type type = libxl__domain_type(gc, domid);
> - libxl_device_pci *pci = prs->pci;
> + libxl_device_pci *pci = &prs->pci;
> int rc, num;
> uint32_t domainid = domid;
>
> - assigned = libxl_device_pci_list(ctx, domid, &num);
> - if (assigned == NULL) {
> + pcis = libxl_device_pci_list(ctx, domid, &num);
> + if (!pcis) {
> rc = ERROR_FAIL;
> goto out_fail;
> }
> - libxl__ptr_add(gc, assigned);
> +
> + attached = is_pci_in_array(pcis, num, pci->domain,
> + pci->bus, pci->dev, pci->func);
> + libxl_device_pci_list_free(pcis, num);
>
> rc = ERROR_INVAL;
> - if ( !is_pci_in_array(assigned, num, pci->domain,
> - pci->bus, pci->dev, pci->func) ) {
> + if (!attached) {
> LOGD(ERROR, domainid, "PCI device not attached to this domain");
> goto out_fail;
> }
> @@ -1928,7 +1929,7 @@ static void pci_remove_qemu_trad_watch_state_cb(libxl__egc *egc,
>
> /* Convenience aliases */
> libxl_domid domid = prs->domid;
> - libxl_device_pci *const pci = prs->pci;
> + libxl_device_pci *const pci = &prs->pci;
>
> rc = check_qemu_running(gc, domid, xswa, rc, state);
> if (rc == ERROR_NOT_READY)
> @@ -1950,7 +1951,7 @@ static void pci_remove_qmp_device_del(libxl__egc *egc,
> int rc;
>
> /* Convenience aliases */
> - libxl_device_pci *const pci = prs->pci;
> + libxl_device_pci *const pci = &prs->pci;
>
> rc = libxl__ev_time_register_rel(ao, &prs->timeout,
> pci_remove_timeout,
> @@ -2020,7 +2021,7 @@ static void pci_remove_qmp_query_cb(libxl__egc *egc,
>
> /* Convenience aliases */
> libxl__ao *const ao = prs->aodev->ao;
> - libxl_device_pci *const pci = prs->pci;
> + libxl_device_pci *const pci = &prs->pci;
>
> if (rc) goto out;
>
> @@ -2075,7 +2076,7 @@ static void pci_remove_timeout(libxl__egc *egc, libxl__ev_time *ev,
> pci_remove_state *prs = CONTAINER_OF(ev, *prs, timeout);
>
> /* Convenience aliases */
> - libxl_device_pci *const pci = prs->pci;
> + libxl_device_pci *const pci = &prs->pci;
>
> LOGD(WARN, prs->domid, "timed out waiting for DM to remove "
> PCI_PT_QDEV_ID, pci->bus, pci->dev, pci->func);
> @@ -2096,7 +2097,7 @@ static void pci_remove_detached(libxl__egc *egc,
> bool isstubdom;
>
> /* Convenience aliases */
> - libxl_device_pci *const pci = prs->pci;
> + libxl_device_pci *const pci = &prs->pci;
> libxl_domid domid = prs->domid;
>
> /* Cleaning QMP states ASAP */
> @@ -2159,7 +2160,7 @@ static void pci_remove_done(libxl__egc *egc,
>
> if (rc) goto out;
>
> - libxl__device_pci_remove_xenstore(gc, prs->domid, prs->pci);
> + libxl__device_pci_remove_xenstore(gc, prs->domid, &prs->pci);
> out:
> device_pci_remove_common_next(egc, prs, rc);
> }
> @@ -2177,7 +2178,10 @@ static void libxl__device_pci_remove_common(libxl__egc *egc,
> GCNEW(prs);
> prs->aodev = aodev;
> prs->domid = domid;
> - prs->pci = pci;
> +
> + libxl_device_pci_copy(CTX, &prs->pci, pci);
> + pci = &prs->pci;
> +
> prs->force = force;
> libxl__xswait_init(&prs->xswait);
> libxl__ev_qmp_init(&prs->qmp);
> @@ -2212,7 +2216,7 @@ static void device_pci_remove_common_next(libxl__egc *egc,
> EGC_GC;
>
> /* Convenience aliases */
> - libxl_device_pci *const pci = prs->pci;
> + libxl_device_pci *const pci = &prs->pci;
> libxl__ao_device *const aodev = prs->aodev;
> const unsigned int pfunc_mask = prs->pfunc_mask;
> const unsigned int orig_vdev = prs->orig_vdev;
> @@ -2243,6 +2247,7 @@ out:
>
> if (!rc) pci_info_xs_remove(gc, pci, "domid");
>
> + libxl_device_pci_dispose(pci);
> aodev->rc = rc;
> aodev->callback(egc, aodev);
> }
> @@ -2357,7 +2362,6 @@ void libxl__device_pci_destroy_all(libxl__egc *egc, uint32_t domid,
> pcis = libxl_device_pci_list(CTX, domid, &num);
> if ( pcis == NULL )
> return;
> - libxl__ptr_add(gc, pcis);
>
> for (i = 0; i < num; i++) {
> /* Force remove on shutdown since, on HVM, qemu will not always
> @@ -2368,6 +2372,8 @@ void libxl__device_pci_destroy_all(libxl__egc *egc, uint32_t domid,
> libxl__device_pci_remove_common(egc, domid, pcis + i, true,
> aodev);
> }
> +
> + libxl_device_pci_list_free(pcis, num);
> }
>
> int libxl__grant_vga_iomem_permission(libxl__gc *gc, const uint32_t domid,
> diff --git a/tools/xl/xl_pci.c b/tools/xl/xl_pci.c
> index 34fcf5a4fa..7c0f102ac7 100644
> --- a/tools/xl/xl_pci.c
> +++ b/tools/xl/xl_pci.c
> @@ -35,9 +35,8 @@ static void pcilist(uint32_t domid)
> printf("%02x.%01x %04x:%02x:%02x.%01x\n",
> (pcis[i].vdevfn >> 3) & 0x1f, pcis[i].vdevfn & 0x7,
> pcis[i].domain, pcis[i].bus, pcis[i].dev, pcis[i].func);
> - libxl_device_pci_dispose(&pcis[i]);
> }
> - free(pcis);
> + libxl_device_pci_list_free(pcis, num);
> }
>
> int main_pcilist(int argc, char **argv)
next prev parent reply other threads:[~2020-12-01 15:11 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-24 8:01 [PATCH v4 00/23] xl / libxl: named PCI pass-through devices Paul Durrant
2020-11-24 8:01 ` [PATCH v4 01/23] xl / libxl: s/pcidev/pci and remove DEFINE_DEVICE_TYPE_STRUCT_X Paul Durrant
2020-12-01 12:32 ` Oleksandr Andrushchenko
2020-12-03 13:00 ` Paul Durrant
2020-11-24 8:01 ` [PATCH v4 02/23] libxl: make libxl__device_list() work correctly for LIBXL__DEVICE_KIND_PCI Paul Durrant
2020-12-01 12:50 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 03/23] libxl: Make sure devices added by pci-attach are reflected in the config Paul Durrant
2020-12-01 13:12 ` Oleksandr Andrushchenko
2020-12-03 13:17 ` Paul Durrant
2020-12-03 13:20 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 04/23] libxl: add/recover 'rdm_policy' to/from PCI backend in xenstore Paul Durrant
2020-12-01 13:13 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 05/23] libxl: s/detatched/detached in libxl_pci.c Paul Durrant
2020-12-01 13:15 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 06/23] libxl: remove extraneous arguments to do_pci_remove() " Paul Durrant
2020-12-01 13:41 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 07/23] libxl: stop using aodev->device_config in libxl__device_pci_add() Paul Durrant
2020-12-01 13:42 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 08/23] libxl: generalise 'driver_path' xenstore access functions in libxl_pci.c Paul Durrant
2020-12-01 13:48 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 09/23] libxl: remove unnecessary check from libxl__device_pci_add() Paul Durrant
2020-12-01 13:51 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 10/23] libxl: remove get_all_assigned_devices() from libxl_pci.c Paul Durrant
2020-12-01 14:18 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 11/23] libxl: make sure callers of libxl_device_pci_list() free the list after use Paul Durrant
2020-12-01 15:10 ` Oleksandr Andrushchenko [this message]
2020-11-24 8:01 ` [PATCH v4 12/23] libxl: add libxl_device_pci_assignable_list_free() Paul Durrant
2020-12-01 15:17 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 13/23] libxl: use COMPARE_PCI() macro is_pci_in_array() Paul Durrant
2020-12-01 15:20 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 14/23] docs/man: extract documentation of PCI_SPEC_STRING from the xl.cfg manpage Paul Durrant
2020-12-01 15:25 ` Oleksandr Andrushchenko
2020-11-24 8:01 ` [PATCH v4 15/23] docs/man: improve documentation of PCI_SPEC_STRING Paul Durrant
2020-11-24 8:01 ` [PATCH v4 16/23] docs/man: fix xl(1) documentation for 'pci' operations Paul Durrant
2020-11-24 8:01 ` [PATCH v4 17/23] libxl: introduce 'libxl_pci_bdf' in the idl Paul Durrant
2020-11-24 8:01 ` [PATCH v4 18/23] libxlu: introduce xlu_pci_parse_spec_string() Paul Durrant
2020-11-24 8:01 ` [PATCH v4 19/23] libxl: modify libxl_device_pci_assignable_add/remove/list/list_free() Paul Durrant
2020-11-24 8:01 ` [PATCH v4 20/23] docs/man: modify xl(1) in preparation for naming of assignable devices Paul Durrant
2020-11-24 8:01 ` [PATCH v4 21/23] xl / libxl: support " Paul Durrant
2020-11-24 8:01 ` [PATCH v4 22/23] docs/man: modify xl-pci-configuration(5) to add 'name' field to PCI_SPEC_STRING Paul Durrant
2020-11-24 8:01 ` [PATCH v4 23/23] xl / libxl: support 'xl pci-attach/detach' by name Paul Durrant
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e6f0aad0-60bd-169e-1e70-f86d0517e551@epam.com \
--to=oleksandr_andrushchenko@epam.com \
--cc=anthony.perard@citrix.com \
--cc=iwj@xenproject.org \
--cc=paul@xen.org \
--cc=pdurrant@amazon.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).