* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-08 14:40 ` Paolo Bonzini
@ 2014-06-08 14:52 ` Michael S. Tsirkin
2014-06-08 14:52 ` Michael S. Tsirkin
2014-06-09 17:02 ` Bandan Das
2 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2014-06-08 14:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, bsd, qemu-devel, Andreas Färber,
Stefan Hajnoczi
On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >Tested-by: Michael S. Tsirkin <mst@redhat.com>
>
> You probably tested the reversal, actually. :)
I guess so, maybe patch asked me about it.
> Actually, there is a reason for it. "Unassembling" the device
> (unparent) should come after "powering it down" (unrealize).
>
> However, the bus is missing a recursive unrealization of the devices
> below it prior to calling bc->unrealize:
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e65a5aa..4282491 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool value,
> Error **errp)
> {
> BusState *bus = BUS(obj);
> BusClass *bc = BUS_GET_CLASS(bus);
> + BusChild *kid;
> Error *local_err = NULL;
>
> if (value && !bus->realized) {
> if (bc->realize) {
> bc->realize(bus, &local_err);
> -
> - if (local_err != NULL) {
> - goto error;
> - }
> -
> }
> +
> + /* TODO: recursive realization */
> } else if (!value && bus->realized) {
> - if (bc->unrealize) {
> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
> + DeviceState *dev = kid->child;
> + object_property_set_bool(OBJECT(dev), false, "realized",
> + &local_err);
> + if (local_err != NULL) {
> + break;
> + }
> + }
> + if (bc->unrealize && local_err == NULL) {
> bc->unrealize(bus, &local_err);
> -
> - if (local_err != NULL) {
> - goto error;
> - }
> }
> }
>
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> bus->realized = value;
> - return;
> -
> -error:
> - error_propagate(errp, local_err);
> }
>
> void qbus_create_inplace(void *bus, size_t size, const char *typename,
>
>
> This seems to fix the bug too.
>
> Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-08 14:40 ` Paolo Bonzini
2014-06-08 14:52 ` Michael S. Tsirkin
@ 2014-06-08 14:52 ` Michael S. Tsirkin
2014-06-09 7:51 ` Paolo Bonzini
2014-06-09 17:02 ` Bandan Das
2 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2014-06-08 14:52 UTC (permalink / raw)
To: Paolo Bonzini
Cc: peter.maydell, bsd, qemu-devel, Andreas Färber,
Stefan Hajnoczi
On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >Tested-by: Michael S. Tsirkin <mst@redhat.com>
>
> You probably tested the reversal, actually. :)
>
> Actually, there is a reason for it. "Unassembling" the device
> (unparent) should come after "powering it down" (unrealize).
>
> However, the bus is missing a recursive unrealization of the devices
> below it prior to calling bc->unrealize:
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e65a5aa..4282491 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool value,
> Error **errp)
> {
> BusState *bus = BUS(obj);
> BusClass *bc = BUS_GET_CLASS(bus);
> + BusChild *kid;
> Error *local_err = NULL;
>
> if (value && !bus->realized) {
> if (bc->realize) {
> bc->realize(bus, &local_err);
> -
> - if (local_err != NULL) {
> - goto error;
> - }
> -
> }
> +
> + /* TODO: recursive realization */
> } else if (!value && bus->realized) {
> - if (bc->unrealize) {
> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
> + DeviceState *dev = kid->child;
> + object_property_set_bool(OBJECT(dev), false, "realized",
> + &local_err);
> + if (local_err != NULL) {
> + break;
> + }
> + }
> + if (bc->unrealize && local_err == NULL) {
> bc->unrealize(bus, &local_err);
> -
> - if (local_err != NULL) {
> - goto error;
> - }
> }
> }
>
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> bus->realized = value;
> - return;
> -
> -error:
> - error_propagate(errp, local_err);
> }
>
> void qbus_create_inplace(void *bus, size_t size, const char *typename,
>
>
> This seems to fix the bug too.
>
> Paolo
Want to submit as a proper patch?
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-08 14:52 ` Michael S. Tsirkin
@ 2014-06-09 7:51 ` Paolo Bonzini
2014-06-09 8:15 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2014-06-09 7:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Stefan Hajnoczi, peter.maydell, bsd, qemu-devel,
Andreas Färber
Il 08/06/2014 16:52, Michael S. Tsirkin ha scritto:
> On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> You probably tested the reversal, actually. :)
>>
>> Actually, there is a reason for it. "Unassembling" the device
>> (unparent) should come after "powering it down" (unrealize).
>>
>> However, the bus is missing a recursive unrealization of the devices
>> below it prior to calling bc->unrealize:
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e65a5aa..4282491 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool value,
>> Error **errp)
>> {
>> BusState *bus = BUS(obj);
>> BusClass *bc = BUS_GET_CLASS(bus);
>> + BusChild *kid;
>> Error *local_err = NULL;
>>
>> if (value && !bus->realized) {
>> if (bc->realize) {
>> bc->realize(bus, &local_err);
>> -
>> - if (local_err != NULL) {
>> - goto error;
>> - }
>> -
>> }
>> +
>> + /* TODO: recursive realization */
>> } else if (!value && bus->realized) {
>> - if (bc->unrealize) {
>> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
>> + DeviceState *dev = kid->child;
>> + object_property_set_bool(OBJECT(dev), false, "realized",
>> + &local_err);
>> + if (local_err != NULL) {
>> + break;
>> + }
>> + }
>> + if (bc->unrealize && local_err == NULL) {
>> bc->unrealize(bus, &local_err);
>> -
>> - if (local_err != NULL) {
>> - goto error;
>> - }
>> }
>> }
>>
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> bus->realized = value;
>> - return;
>> -
>> -error:
>> - error_propagate(errp, local_err);
>> }
>>
>> void qbus_create_inplace(void *bus, size_t size, const char *typename,
>>
>>
>> This seems to fix the bug too.
>>
>> Paolo
>
> Want to submit as a proper patch?
I can, but I suppose Andreas will watch this when he's back and comment.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-09 7:51 ` Paolo Bonzini
@ 2014-06-09 8:15 ` Michael S. Tsirkin
0 siblings, 0 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2014-06-09 8:15 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, peter.maydell, bsd, qemu-devel,
Andreas Färber
On Mon, Jun 09, 2014 at 09:51:56AM +0200, Paolo Bonzini wrote:
> Il 08/06/2014 16:52, Michael S. Tsirkin ha scritto:
> >On Sun, Jun 08, 2014 at 04:40:56PM +0200, Paolo Bonzini wrote:
> >>Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
> >>>Tested-by: Michael S. Tsirkin <mst@redhat.com>
> >>
> >>You probably tested the reversal, actually. :)
> >>
> >>Actually, there is a reason for it. "Unassembling" the device
> >>(unparent) should come after "powering it down" (unrealize).
> >>
> >>However, the bus is missing a recursive unrealization of the devices
> >>below it prior to calling bc->unrealize:
> >>
> >>diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >>index e65a5aa..4282491 100644
> >>--- a/hw/core/qdev.c
> >>+++ b/hw/core/qdev.c
> >>@@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool value,
> >>Error **errp)
> >> {
> >> BusState *bus = BUS(obj);
> >> BusClass *bc = BUS_GET_CLASS(bus);
> >>+ BusChild *kid;
> >> Error *local_err = NULL;
> >>
> >> if (value && !bus->realized) {
> >> if (bc->realize) {
> >> bc->realize(bus, &local_err);
> >>-
> >>- if (local_err != NULL) {
> >>- goto error;
> >>- }
> >>-
> >> }
> >>+
> >>+ /* TODO: recursive realization */
> >> } else if (!value && bus->realized) {
> >>- if (bc->unrealize) {
> >>+ QTAILQ_FOREACH(kid, &bus->children, sibling) {
> >>+ DeviceState *dev = kid->child;
> >>+ object_property_set_bool(OBJECT(dev), false, "realized",
> >>+ &local_err);
> >>+ if (local_err != NULL) {
> >>+ break;
> >>+ }
> >>+ }
> >>+ if (bc->unrealize && local_err == NULL) {
> >> bc->unrealize(bus, &local_err);
> >>-
> >>- if (local_err != NULL) {
> >>- goto error;
> >>- }
> >> }
> >> }
> >>
> >>+ if (local_err != NULL) {
> >>+ error_propagate(errp, local_err);
> >>+ return;
> >>+ }
> >>+
> >> bus->realized = value;
> >>- return;
> >>-
> >>-error:
> >>- error_propagate(errp, local_err);
> >> }
> >>
> >> void qbus_create_inplace(void *bus, size_t size, const char *typename,
> >>
> >>
> >>This seems to fix the bug too.
> >>
> >>Paolo
> >
> >Want to submit as a proper patch?
>
> I can, but I suppose Andreas will watch this when he's back and comment.
>
> Paolo
I don't think we want to keep such serious memory corruptors out of
tree. We can always revert the fix if necessary, for now
it's impossible for people to test hotplug properly,
this blocks progress.
--
MST
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-08 14:40 ` Paolo Bonzini
2014-06-08 14:52 ` Michael S. Tsirkin
2014-06-08 14:52 ` Michael S. Tsirkin
@ 2014-06-09 17:02 ` Bandan Das
2014-06-11 12:03 ` Andreas Färber
2 siblings, 1 reply; 14+ messages in thread
From: Bandan Das @ 2014-06-09 17:02 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Stefan Hajnoczi, peter.maydell, qemu-devel, Andreas Färber,
Michael S. Tsirkin
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>
> You probably tested the reversal, actually. :)
>
> Actually, there is a reason for it. "Unassembling" the device
> (unparent) should come after "powering it down" (unrealize).
Yes, exactly. But to be honest, I was not sure if I was doing the
right thing and hence posted this series as a RFC.
> However, the bus is missing a recursive unrealization of the devices
> below it prior to calling bc->unrealize:
>
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index e65a5aa..4282491 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool
> value, Error **errp)
> {
> BusState *bus = BUS(obj);
> BusClass *bc = BUS_GET_CLASS(bus);
> + BusChild *kid;
> Error *local_err = NULL;
>
> if (value && !bus->realized) {
> if (bc->realize) {
> bc->realize(bus, &local_err);
> -
> - if (local_err != NULL) {
> - goto error;
> - }
> -
> }
> +
> + /* TODO: recursive realization */
> } else if (!value && bus->realized) {
> - if (bc->unrealize) {
> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
> + DeviceState *dev = kid->child;
> + object_property_set_bool(OBJECT(dev), false, "realized",
> + &local_err);
> + if (local_err != NULL) {
> + break;
> + }
> + }
> + if (bc->unrealize && local_err == NULL) {
> bc->unrealize(bus, &local_err);
It also seems that my original patch included unrealizing children, but
that didn't get in for some reason -
https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03204.html
Andreas might have had a reason not to include it however..
> -
> - if (local_err != NULL) {
> - goto error;
> - }
> }
> }
>
> + if (local_err != NULL) {
> + error_propagate(errp, local_err);
> + return;
> + }
> +
> bus->realized = value;
> - return;
> -
> -error:
> - error_propagate(errp, local_err);
> }
>
> void qbus_create_inplace(void *bus, size_t size, const char *typename,
>
>
> This seems to fix the bug too.
>
> Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-09 17:02 ` Bandan Das
@ 2014-06-11 12:03 ` Andreas Färber
2014-06-11 12:24 ` Paolo Bonzini
2014-06-11 15:51 ` Bandan Das
0 siblings, 2 replies; 14+ messages in thread
From: Andreas Färber @ 2014-06-11 12:03 UTC (permalink / raw)
To: Bandan Das, Paolo Bonzini
Cc: Stefan Hajnoczi, Michael S. Tsirkin, qemu-devel, peter.maydell
Am 09.06.2014 19:02, schrieb Bandan Das:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> You probably tested the reversal, actually. :)
>>
>> Actually, there is a reason for it. "Unassembling" the device
>> (unparent) should come after "powering it down" (unrealize).
>
> Yes, exactly. But to be honest, I was not sure if I was doing the
> right thing and hence posted this series as a RFC.
>
>> However, the bus is missing a recursive unrealization of the devices
>> below it prior to calling bc->unrealize:
>>
>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>> index e65a5aa..4282491 100644
>> --- a/hw/core/qdev.c
>> +++ b/hw/core/qdev.c
>> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool
>> value, Error **errp)
>> {
>> BusState *bus = BUS(obj);
>> BusClass *bc = BUS_GET_CLASS(bus);
>> + BusChild *kid;
>> Error *local_err = NULL;
>>
>> if (value && !bus->realized) {
>> if (bc->realize) {
>> bc->realize(bus, &local_err);
>> -
>> - if (local_err != NULL) {
>> - goto error;
>> - }
>> -
>> }
>> +
>> + /* TODO: recursive realization */
>> } else if (!value && bus->realized) {
>> - if (bc->unrealize) {
>> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
>> + DeviceState *dev = kid->child;
>> + object_property_set_bool(OBJECT(dev), false, "realized",
>> + &local_err);
>> + if (local_err != NULL) {
>> + break;
>> + }
>> + }
>> + if (bc->unrealize && local_err == NULL) {
>> bc->unrealize(bus, &local_err);
>
> It also seems that my original patch included unrealizing children, but
> that didn't get in for some reason -
> https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03204.html
> Andreas might have had a reason not to include it however..
Yes, we had talked about the future recursive (un)realization at KVM
Forum 2013, but your RFC patch implementing this for buses seemed to get
ahead of the game. I therefore took only the part of the patch that did
not contradict Paolo's objection to unchecked recursive realization -
and I was in a hurry to get it into 2.0 before the freeze, sorry.
Doing only recursive unrealization sounds like a good compromise to me,
since the concerns we had were all about recursive realization and
prereqs not being realized yet. Only suggestion would be to do the error
handling refactoring in a separate patch, but it'll better align with
the qdev.c code then.
Still, isn't this an indication that devices relied on the PCI bus bug
of not unrealizing its state all the time and we may need to go back as
far as ~1.7 (the initial finalize based fix) for resolving it?
Regards,
Andreas
>
>> -
>> - if (local_err != NULL) {
>> - goto error;
>> - }
>> }
>> }
>>
>> + if (local_err != NULL) {
>> + error_propagate(errp, local_err);
>> + return;
>> + }
>> +
>> bus->realized = value;
>> - return;
>> -
>> -error:
>> - error_propagate(errp, local_err);
>> }
>>
>> void qbus_create_inplace(void *bus, size_t size, const char *typename,
>>
>>
>> This seems to fix the bug too.
>>
>> Paolo
>
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-11 12:03 ` Andreas Färber
@ 2014-06-11 12:24 ` Paolo Bonzini
2014-06-11 15:51 ` Bandan Das
1 sibling, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-06-11 12:24 UTC (permalink / raw)
To: Andreas Färber, Bandan Das
Cc: Stefan Hajnoczi, Michael S. Tsirkin, qemu-devel, peter.maydell
Il 11/06/2014 14:03, Andreas Färber ha scritto:
> Still, isn't this an indication that devices relied on the PCI bus bug
> of not unrealizing its state all the time and we may need to go back as
> far as ~1.7 (the initial finalize based fix) for resolving it?
No, I don't think so. The devices rely on being unrealized at a time
when the PCI bus is still valid, but that's it. It's just an ordering
problem introduced by 2.0.
I will repost the patch as toplevel, split and with Cc to qemu-stable.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] Use-after-free during unrealize in system_reset
2014-06-11 12:03 ` Andreas Färber
2014-06-11 12:24 ` Paolo Bonzini
@ 2014-06-11 15:51 ` Bandan Das
1 sibling, 0 replies; 14+ messages in thread
From: Bandan Das @ 2014-06-11 15:51 UTC (permalink / raw)
To: Andreas Färber
Cc: peter.maydell, Paolo Bonzini, Michael S. Tsirkin, qemu-devel,
Stefan Hajnoczi
Andreas Färber <afaerber@suse.de> writes:
> Am 09.06.2014 19:02, schrieb Bandan Das:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>
>>> Il 08/06/2014 12:46, Michael S. Tsirkin ha scritto:
>>>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> You probably tested the reversal, actually. :)
>>>
>>> Actually, there is a reason for it. "Unassembling" the device
>>> (unparent) should come after "powering it down" (unrealize).
>>
>> Yes, exactly. But to be honest, I was not sure if I was doing the
>> right thing and hence posted this series as a RFC.
>>
>>> However, the bus is missing a recursive unrealization of the devices
>>> below it prior to calling bc->unrealize:
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index e65a5aa..4282491 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -567,32 +567,35 @@ static void bus_set_realized(Object *obj, bool
>>> value, Error **errp)
>>> {
>>> BusState *bus = BUS(obj);
>>> BusClass *bc = BUS_GET_CLASS(bus);
>>> + BusChild *kid;
>>> Error *local_err = NULL;
>>>
>>> if (value && !bus->realized) {
>>> if (bc->realize) {
>>> bc->realize(bus, &local_err);
>>> -
>>> - if (local_err != NULL) {
>>> - goto error;
>>> - }
>>> -
>>> }
>>> +
>>> + /* TODO: recursive realization */
>>> } else if (!value && bus->realized) {
>>> - if (bc->unrealize) {
>>> + QTAILQ_FOREACH(kid, &bus->children, sibling) {
>>> + DeviceState *dev = kid->child;
>>> + object_property_set_bool(OBJECT(dev), false, "realized",
>>> + &local_err);
>>> + if (local_err != NULL) {
>>> + break;
>>> + }
>>> + }
>>> + if (bc->unrealize && local_err == NULL) {
>>> bc->unrealize(bus, &local_err);
>>
>> It also seems that my original patch included unrealizing children, but
>> that didn't get in for some reason -
>> https://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03204.html
>> Andreas might have had a reason not to include it however..
>
> Yes, we had talked about the future recursive (un)realization at KVM
> Forum 2013, but your RFC patch implementing this for buses seemed to get
> ahead of the game. I therefore took only the part of the patch that did
> not contradict Paolo's objection to unchecked recursive realization -
> and I was in a hurry to get it into 2.0 before the freeze, sorry.
That's ok. I only regret that despite being posted as a RFC and way before
the 2.0 freeze deadline, the patches received absolutely no feedback. If
we had discussed Paolo's objections when the patches were posted, someone
might have pointed out what could go wrong.
Bandan
> Doing only recursive unrealization sounds like a good compromise to me,
> since the concerns we had were all about recursive realization and
> prereqs not being realized yet. Only suggestion would be to do the error
> handling refactoring in a separate patch, but it'll better align with
> the qdev.c code then.
>
> Still, isn't this an indication that devices relied on the PCI bus bug
> of not unrealizing its state all the time and we may need to go back as
> far as ~1.7 (the initial finalize based fix) for resolving it?
>
> Regards,
> Andreas
>
>>
>>> -
>>> - if (local_err != NULL) {
>>> - goto error;
>>> - }
>>> }
>>> }
>>>
>>> + if (local_err != NULL) {
>>> + error_propagate(errp, local_err);
>>> + return;
>>> + }
>>> +
>>> bus->realized = value;
>>> - return;
>>> -
>>> -error:
>>> - error_propagate(errp, local_err);
>>> }
>>>
>>> void qbus_create_inplace(void *bus, size_t size, const char *typename,
>>>
>>>
>>> This seems to fix the bug too.
>>>
>>> Paolo
>>
^ permalink raw reply [flat|nested] 14+ messages in thread