| Message ID | 20200727130225.32640-1-zhukeqian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bugfix: irq: Avoid covering object refcount of qemu_irq | expand |
Keqian Zhu <zhukeqian1@huawei.com> 于2020年7月27日周一 下午9:03写道： > > Avoid covering object refcount of qemu_irq, otherwise it may causes > memory leak. Any reproducer? Thanks, Li Qiang > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > hw/core/irq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/core/irq.c b/hw/core/irq.c > index fb3045b912..59af4dfc74 100644 > --- a/hw/core/irq.c > +++ b/hw/core/irq.c > @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) > int i; > qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); > for (i = 0; i < n; i++) { > - *old_irqs[i] = *gpio_in[i]; > + old_irqs[i]->handler = gpio_in[i]->handler; > + old_irqs[i]->opaque = gpio_in[i]->opaque; > + > gpio_in[i]->handler = handler; > gpio_in[i]->opaque = &old_irqs[i]; > } > -- > 2.19.1 >
On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <zhukeqian1@huawei.com> wrote: > > Avoid covering object refcount of qemu_irq, otherwise it may causes > memory leak. > > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > hw/core/irq.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/hw/core/irq.c b/hw/core/irq.c > index fb3045b912..59af4dfc74 100644 > --- a/hw/core/irq.c > +++ b/hw/core/irq.c > @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) > int i; > qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); > for (i = 0; i < n; i++) { > - *old_irqs[i] = *gpio_in[i]; > + old_irqs[i]->handler = gpio_in[i]->handler; > + old_irqs[i]->opaque = gpio_in[i]->opaque; > + > gpio_in[i]->handler = handler; > gpio_in[i]->opaque = &old_irqs[i]; > } This function is leaky by design, because it doesn't do anything with the old_irqs array and there's no function for un-intercepting the IRQs (which would need to free that memory). This is not ideal but OK because it's only used in the test suite. Is there a specific bug you're trying to fix here? thanks -- PMM
Hi Qiang, On 2020/7/27 22:37, Li Qiang wrote: > Keqian Zhu <zhukeqian1@huawei.com> 于2020年7月27日周一 下午9:03写道： >> >> Avoid covering object refcount of qemu_irq, otherwise it may causes >> memory leak. > > Any reproducer? > In mainline Qemu. this function is only used in qtest. One of our internal self-developed module also use this function. The memory leak is reported by ASAN. Thanks, Keqian > Thanks, > Li Qiang > >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> hw/core/irq.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/irq.c b/hw/core/irq.c >> index fb3045b912..59af4dfc74 100644 >> --- a/hw/core/irq.c >> +++ b/hw/core/irq.c >> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) >> int i; >> qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); >> for (i = 0; i < n; i++) { >> - *old_irqs[i] = *gpio_in[i]; >> + old_irqs[i]->handler = gpio_in[i]->handler; >> + old_irqs[i]->opaque = gpio_in[i]->opaque; >> + >> gpio_in[i]->handler = handler; >> gpio_in[i]->opaque = &old_irqs[i]; >> } >> -- >> 2.19.1 >> > . >
Hi Peter, On 2020/7/27 22:41, Peter Maydell wrote: > On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <zhukeqian1@huawei.com> wrote: >> >> Avoid covering object refcount of qemu_irq, otherwise it may causes >> memory leak. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> hw/core/irq.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/irq.c b/hw/core/irq.c >> index fb3045b912..59af4dfc74 100644 >> --- a/hw/core/irq.c >> +++ b/hw/core/irq.c >> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) >> int i; >> qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); >> for (i = 0; i < n; i++) { >> - *old_irqs[i] = *gpio_in[i]; >> + old_irqs[i]->handler = gpio_in[i]->handler; >> + old_irqs[i]->opaque = gpio_in[i]->opaque; >> + >> gpio_in[i]->handler = handler; >> gpio_in[i]->opaque = &old_irqs[i]; >> } > > This function is leaky by design, because it doesn't do anything > with the old_irqs array and there's no function for un-intercepting > the IRQs (which would need to free that memory). This is not ideal > but OK because it's only used in the test suite. One of our internal self-developed module also use this function, and we implemented a function to remove intercepting, so there is no memory leak after this bugfix. I suggest to merge this bugfix to prepare for future code which may invoke this function. > > Is there a specific bug you're trying to fix here? The memory leak is reported by ASAN. > Thanks, Keqian > thanks > -- PMM > . >
On 27/07/2020 16.41, Peter Maydell wrote: > On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <zhukeqian1@huawei.com> wrote: >> >> Avoid covering object refcount of qemu_irq, otherwise it may causes >> memory leak. >> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> hw/core/irq.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/hw/core/irq.c b/hw/core/irq.c >> index fb3045b912..59af4dfc74 100644 >> --- a/hw/core/irq.c >> +++ b/hw/core/irq.c >> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) >> int i; >> qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); >> for (i = 0; i < n; i++) { >> - *old_irqs[i] = *gpio_in[i]; >> + old_irqs[i]->handler = gpio_in[i]->handler; >> + old_irqs[i]->opaque = gpio_in[i]->opaque; >> + >> gpio_in[i]->handler = handler; >> gpio_in[i]->opaque = &old_irqs[i]; >> } > > This function is leaky by design, because it doesn't do anything > with the old_irqs array and there's no function for un-intercepting > the IRQs (which would need to free that memory). This is not ideal > but OK because it's only used in the test suite. I think this could better be done without calling qemu_allocate_irqs(): Simply call qemu_allocate_irq() (without "s" at the end) within the for-loop for each irq instead. What do you think? Thomas
Hi Thomas, On 2020/7/28 16:48, Thomas Huth wrote: > On 27/07/2020 16.41, Peter Maydell wrote: >> On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <zhukeqian1@huawei.com> wrote: >>> >>> Avoid covering object refcount of qemu_irq, otherwise it may causes >>> memory leak. >>> >>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >>> --- >>> hw/core/irq.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/core/irq.c b/hw/core/irq.c >>> index fb3045b912..59af4dfc74 100644 >>> --- a/hw/core/irq.c >>> +++ b/hw/core/irq.c >>> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) >>> int i; >>> qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); >>> for (i = 0; i < n; i++) { >>> - *old_irqs[i] = *gpio_in[i]; >>> + old_irqs[i]->handler = gpio_in[i]->handler; >>> + old_irqs[i]->opaque = gpio_in[i]->opaque; >>> + >>> gpio_in[i]->handler = handler; >>> gpio_in[i]->opaque = &old_irqs[i]; >>> } >> >> This function is leaky by design, because it doesn't do anything >> with the old_irqs array and there's no function for un-intercepting >> the IRQs (which would need to free that memory). This is not ideal >> but OK because it's only used in the test suite. > > I think this could better be done without calling qemu_allocate_irqs(): > Simply call qemu_allocate_irq() (without "s" at the end) within the > for-loop for each irq instead. What do you think? Yeah, this can save some memory. But I think it does not solve the refcount covering problem. > Thanks Keqian > Thomas > > >
On Tue, 28 Jul 2020 at 09:48, Thomas Huth <thuth@redhat.com> wrote: > > On 27/07/2020 16.41, Peter Maydell wrote: > > On Mon, 27 Jul 2020 at 14:03, Keqian Zhu <zhukeqian1@huawei.com> wrote: > >> > >> Avoid covering object refcount of qemu_irq, otherwise it may causes > >> memory leak. > >> > >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > >> --- > >> hw/core/irq.c | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/hw/core/irq.c b/hw/core/irq.c > >> index fb3045b912..59af4dfc74 100644 > >> --- a/hw/core/irq.c > >> +++ b/hw/core/irq.c > >> @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) > >> int i; > >> qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); > >> for (i = 0; i < n; i++) { > >> - *old_irqs[i] = *gpio_in[i]; > >> + old_irqs[i]->handler = gpio_in[i]->handler; > >> + old_irqs[i]->opaque = gpio_in[i]->opaque; > >> + > >> gpio_in[i]->handler = handler; > >> gpio_in[i]->opaque = &old_irqs[i]; > >> } > > > > This function is leaky by design, because it doesn't do anything > > with the old_irqs array and there's no function for un-intercepting > > the IRQs (which would need to free that memory). This is not ideal > > but OK because it's only used in the test suite. > > I think this could better be done without calling qemu_allocate_irqs(): > Simply call qemu_allocate_irq() (without "s" at the end) within the > for-loop for each irq instead. What do you think? Well, what are we trying to do with the function? I think that your suggestion still doesn't really fix the leak in the sense that there's no mechanism for undoing the operation and freeing the memory allocated by qemu_allocate_irq(). The whole concept of the function is pretty dubious because it's arbitrarily re-plugging IRQs, which in the whole of the rest of QEMU are assumed to be something that's wired up by board code or by an SoC device that "owns" the two devices it's connecting. That's fine for test-case purposes, and for the qtest code that is the only in-tree user of this function we could construct a (nominally) leak-free version of the function by having it return some kind of handle that could be passed to a qemu_irq_remove_intercept_in() to clean up [this would fix a Coverity nag, which would be the main benefit...]. I agree that copying the refcount here looks wrong, but in what situation is the refcount on any of the qemu_irqs involved changing anyway? For non-testing uses of this function it gets trickier (what if the device on one end or the other is destroyed while the intercept is in place, for instance?) So I'm a bit sceptical of ideas about extending the uses of this function to non-test-code, and I think I'd want to see what the intended non-test uses were... thanks -- PMM
diff --git a/hw/core/irq.c b/hw/core/irq.c index fb3045b912..59af4dfc74 100644 --- a/hw/core/irq.c +++ b/hw/core/irq.c @@ -125,7 +125,9 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n) int i; qemu_irq *old_irqs = qemu_allocate_irqs(NULL, NULL, n); for (i = 0; i < n; i++) { - *old_irqs[i] = *gpio_in[i]; + old_irqs[i]->handler = gpio_in[i]->handler; + old_irqs[i]->opaque = gpio_in[i]->opaque; + gpio_in[i]->handler = handler; gpio_in[i]->opaque = &old_irqs[i]; }
Avoid covering object refcount of qemu_irq, otherwise it may causes memory leak. Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- hw/core/irq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)