[PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()

[PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()

[Stefano Garzarella]

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

Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()

[Vladimir Sementsov-Ogievskiy]

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

Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()

[Stefano Garzarella]

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

Re: [PATCH] block/mirror: fix NULL pointer dereference in mirror_wait_on_conflicts()

[Vladimir Sementsov-Ogievskiy]

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