* [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()
@ 2021-09-10 8:56 Stefano Garzarella
2021-09-10 10:37 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Garzarella @ 2021-09-10 8:56 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Hanna Reitz, Vladimir Sementsov-Ogievskiy, John Snow,
qemu-block
In mirror_iteration() we call mirror_wait_on_conflicts() with
`self` parameter set to NULL.
Starting from commit d44dae1a7c we dereference `self` pointer in
mirror_wait_on_conflicts() without checks if it is not NULL.
Backtrace:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
at ../block/mirror.c:172
172 self->waiting_for_op = op;
[Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]
(gdb) bt
#0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
at ../block/mirror.c:172
#1 0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized out>) at ../block/mirror.c:491
#2 0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at ../job.c:917
#3 0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>)
at ../util/coroutine-ucontext.c:173
#4 0x00007f0909975820 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91
from /usr/lib64/libc.so.6
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404
Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in mirror_wait_on_conflicts")
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
I'm not familiar with this code so maybe we can fix the bug differently.
Running iotests and the test in bugzilla, everything seems okay.
Thanks,
Stefano
---
block/mirror.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 98fc66eabf..6c834d6a7b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -169,9 +169,16 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
continue;
}
- self->waiting_for_op = op;
+ if (self) {
+ self->waiting_for_op = op;
+ }
+
qemu_co_queue_wait(&op->waiting_requests, NULL);
- self->waiting_for_op = NULL;
+
+ if (self) {
+ self->waiting_for_op = NULL;
+ }
+
break;
}
}
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()
2021-09-10 8:56 [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts() Stefano Garzarella
@ 2021-09-10 10:37 ` Vladimir Sementsov-Ogievskiy
2021-09-10 11:42 ` Stefano Garzarella
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-10 10:37 UTC (permalink / raw)
To: Stefano Garzarella, qemu-devel
Cc: John Snow, Hanna Reitz, Kevin Wolf, qemu-block
10.09.2021 11:56, Stefano Garzarella wrote:
> In mirror_iteration() we call mirror_wait_on_conflicts() with
> `self` parameter set to NULL.
>
> Starting from commit d44dae1a7c we dereference `self` pointer in
> mirror_wait_on_conflicts() without checks if it is not NULL.
>
> Backtrace:
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
> at ../block/mirror.c:172
> 172 self->waiting_for_op = op;
> [Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]
> (gdb) bt
> #0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
> at ../block/mirror.c:172
> #1 0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized out>) at ../block/mirror.c:491
> #2 0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at ../job.c:917
> #3 0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>)
> at ../util/coroutine-ucontext.c:173
> #4 0x00007f0909975820 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91
> from /usr/lib64/libc.so.6
>
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404
> Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in mirror_wait_on_conflicts")
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> I'm not familiar with this code so maybe we can fix the bug differently.
>
> Running iotests and the test in bugzilla, everything seems okay.
>
> Thanks,
> Stefano
> ---
> block/mirror.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 98fc66eabf..6c834d6a7b 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -169,9 +169,16 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
> continue;
> }
>
> - self->waiting_for_op = op;
> + if (self) {
> + self->waiting_for_op = op;
> + }
> +
> qemu_co_queue_wait(&op->waiting_requests, NULL);
> - self->waiting_for_op = NULL;
> +
> + if (self) {
> + self->waiting_for_op = NULL;
> + }
> +
> break;
> }
> }
>
Hi Stefano!
The patch is almost OK. The only thing is, I think, you should also put "if (op->waiting_for_op) {continue;}" code above into "if (self)" too. Look at the comment above: if we don't have "self", than we are not in the list and nobody will wait for us. This means that we should wait for all.
So, with my suggested fix, you'll simply roll-back d44dae1a7c for the case of self==NULL, which is an additional point of safety.
Still, I wonder, where we check for conflicting requests when create usual MirrorOp. We don't call mirror_wait_on_conflicts() in mirror_perform...
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()
2021-09-10 10:37 ` Vladimir Sementsov-Ogievskiy
@ 2021-09-10 11:42 ` Stefano Garzarella
2021-09-10 12:33 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Garzarella @ 2021-09-10 11:42 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Kevin Wolf, Hanna Reitz, John Snow, qemu-devel, qemu-block
On Fri, Sep 10, 2021 at 01:37:40PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>10.09.2021 11:56, Stefano Garzarella wrote:
>>In mirror_iteration() we call mirror_wait_on_conflicts() with
>>`self` parameter set to NULL.
>>
>>Starting from commit d44dae1a7c we dereference `self` pointer in
>>mirror_wait_on_conflicts() without checks if it is not NULL.
>>
>>Backtrace:
>> Program terminated with signal SIGSEGV, Segmentation fault.
>> #0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
>> at ../block/mirror.c:172
>> 172 self->waiting_for_op = op;
>> [Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]
>> (gdb) bt
>> #0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
>> at ../block/mirror.c:172
>> #1 0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized out>) at ../block/mirror.c:491
>> #2 0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at ../job.c:917
>> #3 0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>)
>> at ../util/coroutine-ucontext.c:173
>> #4 0x00007f0909975820 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91
>> from /usr/lib64/libc.so.6
>>
>>Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404
>>Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in mirror_wait_on_conflicts")
>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>---
>>I'm not familiar with this code so maybe we can fix the bug differently.
>>
>>Running iotests and the test in bugzilla, everything seems okay.
>>
>>Thanks,
>>Stefano
>>---
>> block/mirror.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>>diff --git a/block/mirror.c b/block/mirror.c
>>index 98fc66eabf..6c834d6a7b 100644
>>--- a/block/mirror.c
>>+++ b/block/mirror.c
>>@@ -169,9 +169,16 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
>> continue;
>> }
>>- self->waiting_for_op = op;
>>+ if (self) {
>>+ self->waiting_for_op = op;
>>+ }
>>+
>> qemu_co_queue_wait(&op->waiting_requests, NULL);
>>- self->waiting_for_op = NULL;
>>+
>>+ if (self) {
>>+ self->waiting_for_op = NULL;
>>+ }
>>+
>> break;
>> }
>> }
>>
>
>Hi Stefano!
>
>The patch is almost OK. The only thing is, I think, you should also put
>"if (op->waiting_for_op) {continue;}" code above into "if (self)" too.
>Look at the comment above: if we don't have "self", than we are not in
>the list and nobody will wait for us. This means that we should wait
>for all.
>
>So, with my suggested fix, you'll simply roll-back d44dae1a7c for the
>case of self==NULL, which is an additional point of safety.
>
Right, I'll send a v2 with this change.
>
>Still, I wonder, where we check for conflicting requests when create
>usual MirrorOp. We don't call mirror_wait_on_conflicts() in
>mirror_perform...
IIUC in mirror_iteration() we call mirror_wait_on_conflicts() at the
beginning, then in the cycle we call mirror_perform() where we create a
new MirrorOp and we add it in the `ops_in_flight` list.
At this point, should we call mirror_wait_on_conflicts()?
I need to understand the code better to follow you... :-)
Thanks,
Stefano
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()
2021-09-10 11:42 ` Stefano Garzarella
@ 2021-09-10 12:33 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 4+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-09-10 12:33 UTC (permalink / raw)
To: Stefano Garzarella
Cc: qemu-devel, John Snow, Hanna Reitz, Kevin Wolf, qemu-block
10.09.2021 14:42, Stefano Garzarella wrote:
> On Fri, Sep 10, 2021 at 01:37:40PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 10.09.2021 11:56, Stefano Garzarella wrote:
>>> In mirror_iteration() we call mirror_wait_on_conflicts() with
>>> `self` parameter set to NULL.
>>>
>>> Starting from commit d44dae1a7c we dereference `self` pointer in
>>> mirror_wait_on_conflicts() without checks if it is not NULL.
>>>
>>> Backtrace:
>>> Program terminated with signal SIGSEGV, Segmentation fault.
>>> #0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
>>> at ../block/mirror.c:172
>>> 172 self->waiting_for_op = op;
>>> [Current thread is 1 (Thread 0x7f0908931ec0 (LWP 380249))]
>>> (gdb) bt
>>> #0 mirror_wait_on_conflicts (self=0x0, s=<optimized out>, offset=<optimized out>, bytes=<optimized out>)
>>> at ../block/mirror.c:172
>>> #1 0x00005610c5d9d631 in mirror_run (job=0x5610c76a2c00, errp=<optimized out>) at ../block/mirror.c:491
>>> #2 0x00005610c5d58726 in job_co_entry (opaque=0x5610c76a2c00) at ../job.c:917
>>> #3 0x00005610c5f046c6 in coroutine_trampoline (i0=<optimized out>, i1=<optimized out>)
>>> at ../util/coroutine-ucontext.c:173
>>> #4 0x00007f0909975820 in ?? () at ../sysdeps/unix/sysv/linux/x86_64/__start_context.S:91
>>> from /usr/lib64/libc.so.6
>>>
>>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=2001404
>>> Fixes: d44dae1a7c ("block/mirror: fix active mirror dead-lock in mirror_wait_on_conflicts")
>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> ---
>>> I'm not familiar with this code so maybe we can fix the bug differently.
>>>
>>> Running iotests and the test in bugzilla, everything seems okay.
>>>
>>> Thanks,
>>> Stefano
>>> ---
>>> block/mirror.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/mirror.c b/block/mirror.c
>>> index 98fc66eabf..6c834d6a7b 100644
>>> --- a/block/mirror.c
>>> +++ b/block/mirror.c
>>> @@ -169,9 +169,16 @@ static void coroutine_fn mirror_wait_on_conflicts(MirrorOp *self,
>>> continue;
>>> }
>>> - self->waiting_for_op = op;
>>> + if (self) {
>>> + self->waiting_for_op = op;
>>> + }
>>> +
>>> qemu_co_queue_wait(&op->waiting_requests, NULL);
>>> - self->waiting_for_op = NULL;
>>> +
>>> + if (self) {
>>> + self->waiting_for_op = NULL;
>>> + }
>>> +
>>> break;
>>> }
>>> }
>>>
>>
>> Hi Stefano!
>>
>> The patch is almost OK. The only thing is, I think, you should also put "if (op->waiting_for_op) {continue;}" code above into "if (self)" too. Look at the comment above: if we don't have "self", than we are not in the list and nobody will wait for us. This means that we should wait for all.
>>
>> So, with my suggested fix, you'll simply roll-back d44dae1a7c for the case of self==NULL, which is an additional point of safety.
>>
>
> Right, I'll send a v2 with this change.
>
>>
>> Still, I wonder, where we check for conflicting requests when create usual MirrorOp. We don't call mirror_wait_on_conflicts() in mirror_perform...
>
> IIUC in mirror_iteration() we call mirror_wait_on_conflicts() at the beginning, then in the cycle we call mirror_perform() where we create a new MirrorOp and we add it in the `ops_in_flight` list.
>
> At this point, should we call mirror_wait_on_conflicts()?
Maybe. Deeper audit is needed to understand do we need to fix something and how to do it properly. Maybe I'll dig into it a bit later.. Mirror code is so overcomplicated.
>
> I need to understand the code better to follow you... :-)
My knowing of mirror code is not good too, unfortunately.
>
> Thanks,
> Stefano
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-10 12:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-10 8:56 [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts() Stefano Garzarella
2021-09-10 10:37 ` Vladimir Sementsov-Ogievskiy
2021-09-10 11:42 ` Stefano Garzarella
2021-09-10 12:33 ` Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.