[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] tools/libs/light: numa placement: don't try to free a NULL list of vcpus


  • To: Dario Faggioli <dfaggioli@xxxxxxxx>
  • From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
  • Date: Mon, 17 Jan 2022 15:56:56 +0000
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none
  • Cc: Juergen Gross <JGross@xxxxxxxx>, Jim Fehlig <jfehlig@xxxxxxxx>, "wl@xxxxxxx" <wl@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 17 Jan 2022 15:57:50 +0000
  • Ironport-data: A9a23:0MqbBK32r1OPfW2yhfbD5SB2kn2cJEfYwER7XKvMYLTBsI5bpzADx zMZWG3TbPuNNDb9eI13Pdjl9kJXv8PUzNFnGlE/pC1hF35El5HIVI+TRqvS04J+DSFhoGZPt Zh2hgzodZhsJpPkS5PE3oHJ9RGQ74nRLlbHILOCanAZqTNMEn9700o6wrFh2+aEvPDia++zk YKqyyHgEAfNNw5cagr4PIra9XuDFNyr0N8plgRWicJj5TcypFFMZH4rHomjLmOQf2VhNrXSq 9Avbl2O1jixEx8FUrtJm1tgG6EAaua60QOm0hK6V0U+6/TrS+NbPqsTbZIhhUlrZzqhw9tdl 8gUmLGLWShxEbzVuccZfgVAOnQrVUFG0OevzXmXtMWSywvNcmf2wuUoB0YzVWEa0r8pWycUr 6VecW1TKEDY7w616OvTpu1Ej8I/LM7tLcUHt2tp1z3xBvc6W5HTBa7N4Le02R9u3J0QQaiOO aL1bxIwSjmaaA8VG20YUpQjzOD32HnvbyVX/Qf9Sa0fvDGIkV0ZPKLWGMXRUsyHQ4NShEnwj nLL+SH1Dw8XMPSbyCGZ6TS8i+nXhyT5VYkOUrqi+ZZCglOexmgSBzUNSEC25/K+jyaWV9xWJ EMV/S4vhbMv70HtRd74NzWaj2SDpQU0QMdLHqsx7wTl90bPy1/HXC5eFGcHMYF48p9tLdA36 rOXt4/iKWNy6ZC4cmKyrpiFtBKRPQUJNUZXMEfoUjA5y9XkpYgyiDfGQdBiDLO5g7XJJN3g/ 9yZhHNg3utO1Kbnw43+pAma2Gz0+vAlWyZsvl2PNl9J+D+Vc2JMi2aAzVHApchNI4+CJrVql ChVwpPOhAzi4HzkqcBsfAnvNOzzjxpmGGeF6bKKI3XH3273k5JEVdoBiAyS3G8zbq45lcbBO Sc/Qz956p5JJ2eNZqRqeY+3AMlC5fG+SY6+C6CPPoYSPMgZmOq7EMdGPxP4M4fFyhlErE3CE c3DLZbE4YgyVMyLMwZat89CiOR2l0jSNEvYRIzhzgTP7FZtTCX9dFvxC3PXNrpRxPrd+G39q o8DX+PXlUk3eLCgM0H/rN5CRXhXfCNTLc2n9KRqmhurf1AO9JcJUaGBmNvMuuVNwsxoqws/1 irsBR8Dlguu3C2vxMfjQikLVY4DlK1X9RoTVRHA9370s5T6SYrwvqoZabUterwrqL5qwfJuF qFXcMScGPVfDD/A/m1FP5X6qYVjcjWthB6PYHX5MGRuIcY4Slyb4MLgcyvu6DIKUni9u/whr uDyzQjcW5cCGVhvVZ6EdPK1wlqtlnEBg+YuDVDQK9xedRy0oohnIiD8lNEtJMQIJUmRzzeWz V/OUxwZufPMs8k+99yQ3fKIqIKgEu1fGEtGHjaEsebqZHeCpmf6mN1OSueFezzZRVjYwqT6a LUH1ez4Pd0GgE1O79h2HYF0wP9s/NDovbJbkFhpRS2Zc1SxB7p8CXCaxs0T5LZVz7pUtAbqC EKC/t5WZeeANM//SQNDIQMkaqKI1O0OmymU5vMweR2o6Chy9buBcENTIxjT13ANcOoraNsok bU7pcobyw2jkR57YN+Jgxdd+3mIMnFdAb4ssYsXAdOzhwcmor2YjUcw1sMiDEmzVuhx
  • Ironport-hdrordr: A9a23:vz0ytagkFCwaL0+mUCeIff9Vi3BQXtgji2hC6mlwRA09TySZ// rOoB0+726StN9xYgBFpTnuAsW9qB/nmqKdpLNhW4tKPzOW3VdATrsSjrcKqgeIc0aVm9K1l5 0QEZSWYOeAdGSS5vyb3ODXKbgd/OU=
  • Ironport-sdr: 0Hf78XvPPhLf3O19dekpPUNTV4aHrIdL/rU4KYM0rt9kctCutlnAUwHfniEKGmtBHiTSfyWOP/ IE3RK064kgIBT3slE3rnMk3PqdJQA+Mr1r5koY+pdWWrsL3Ie87pG9BwtALWwkIOhtlpJ0UFsr bYFDKFvYAjKtH5sA9+HNtPXdfjGky/TZ8BJZfQIat8zYTgJtfYMjliIyuPdSsKFeAkNiOIQ80m Dth0ES/kCMD80lREIHvCwyikzK8lnNE10Cq7dJxRNEAdF07ikStHjOW6SWawybL/pK68N8Q0uE M0ANvTSeZe8gb5QyDrSp1qRA
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On Fri, Jan 14, 2022 at 11:22:00PM +0000, Dario Faggioli wrote:
> On Thu, 2022-01-13 at 12:05 +0000, Anthony PERARD wrote:
> > On Wed, Jan 12, 2022 at 05:41:36PM +0100, Dario Faggioli wrote:
> > 
> > > 2) there should be nothing to free anyway
> > 
> > The issue here is that it doesn't appear to be true. Even if "info"
> > is
> > NULL, "nr" have an other value than 0, so libxl_vcpuinfo_list_free()
> > will try to access random addresses.
> > 
> My point here was that if vinfo is NULL (because libxl_list_vcpu()
> returned NULL), there aren't any list element or any list to free, so
> we can avoid calling libxl_vcpuinfo_list_free().
> 
> Then, sure, if we call it with a NULL vinfo and a non-zero
> nr_dom_vcpus, things explode, but this was being addressed in patch 2.
> 
> This first one was really only about not trying to free an empty list.
> And not to workaround the fact that it currently can make things
> explode, just because it feels pointless.
> 
> > > Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> > > Tested-by: James Fehlig <jfehlig@xxxxxxxx>
> > 
> > Can I suggest to make libxl_vcpuinfo_list_free() work a bit better in
> > case it's "nr" parameter is wrong? It will do nothing if "list" is
> > NULL.
> >
> I thought about that, and we certainly can do it.
> 
> However, I think it's unrelated to this patch so, if I do it, I'll do
> it in its own one.
> 
> Also, if we go that way, I guess we want to change
> libxl_cputopology_list_free(), libxl_pcitopology_list_free(),
> libxl_numainfo_list_free(), libxl_dominfo_list_free(),
> libxl_vminfo_list_free() and libxl_cpupoolinfo_list_free(), don't we?

I actually don't know if that would be useful. Those functions do work
as expected (I think) with the right parameters: they do nothing when
called with (NULL, 0). free(NULL) does nothing.

So checking for NULL before using `nr` would probably just be a
workaround for programming mistake in the function allocating the object
or some missing initialisation in the caller. So I don't think it is
important anymore.

> > Also I think it is better to keep the free in the exit path at the
> > end
> > of the loop.
> > 
> Can I ask why as, as I was trying to say above, if we are sent directly
> to next by one of the goto, we do know that vinfo is NULL and
> libxl_vcpuinfo_list_free() will be basically a NOP, however it is
> implemented.
> 
> Of course, you're the maintainer of this code, so I'll do like that if
> you ask... I'm just curious. :-)

Freeing resources should always been attempted, even if that mean to
check whether there's something to free before calling the associated
free function. Imagine someone adding some code that could fail after
the libxl_list_vcpu(), then when that new code fails it would `goto
next;` and the allocated data from libxl_list_vcpu() would never be
freed.

> Actually, if you really think that the call to
> libxl_vcpuinfo_list_free() should stay where it is, I can just drop
> this patch and go on with patch 2 only, which is enough for fixing the
> crash.

This patch is just a workaround a bug in libxl_list_vcpu(), so I think
it can be dropped.

Cheers,

-- 
Anthony PERARD



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.